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

[Peter Lieven <pl@kamp.de>]

On 27.10.2014 09:32, Max Reitz wrote:
> 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.

Good point, I would cap all limits to the highest power of two and use INT_MAX / 2 + 1 directly in nb_sectors_lun2qemu?

Peter

>
>>       }
>>         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


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................