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

[Max Reitz <mreitz@redhat.com>]

On 2014-10-27 at 09:35, Peter Lieven wrote:
> 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?

Seems good enough. I'd just like a comment in that function why you're 
doing that (in order to limit both alignments and "normal" lengths 
properly).

I'm counting on nobody noticing that INT_MAX may be something different 
than 2^x - 1. *g*

Max

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