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

[Max Reitz]

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 <address@hidden>
> > > ---
> > >   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