Re: [Qemu-devel] [PATCHv5 5/6] block/iscsi: limit to INT_MAX throughout iscsi_refresh_limits

[Max Reitz <mreitz@redhat.com>]

On 2014-10-25 at 18:18, Peter Lieven wrote:
> As Max pointed out there is a hidden cast from int64_t to int.
> So use the newly introduced nb_sectors_lun2qemu for all
> limits.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/iscsi.c |   20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 1ae4add..85131b7 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>   
>       if (iscsilun->lbp.lbpu) {
>           if (iscsilun->bl.max_unmap < 0xffffffff) {
> -            bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
> -                                                 iscsilun);
> +            bs->bl.max_discard = nb_sectors_lun2qemu(iscsilun->bl.max_unmap,
> +                                                     iscsilun);
>           }
> -        bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
> -                                                   iscsilun);
> +        bs->bl.discard_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran,
> +                                                       iscsilun);

This looks wrong. I think an alignment should always be a power of two. 
The function however may return the unaligned INT_MAX. I think it should 
be capped to something like INT_MAX / 2 + 1 or just simply written out 
0x40000000.

>       }
>   
>       if (iscsilun->bl.max_ws_len < 0xffffffff) {
> -        bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
> -                                                  iscsilun);
> +        bs->bl.max_write_zeroes = nb_sectors_lun2qemu(iscsilun->bl.max_ws_len,
> +                                                      iscsilun);
>       }
>       if (iscsilun->lbp.lbpws) {
> -        bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
> -                                                        iscsilun);
> +        bs->bl.write_zeroes_alignment = nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran,
> +                                                            iscsilun);

Same here.

>       }
> -    bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len,
> -                                                 iscsilun);
> +    bs->bl.opt_transfer_length = nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len,
> +                                                     iscsilun);
>   }
>   
>   /* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in

Max