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

[Peter Lieven]

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?

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
  address@hidden | http://www.kamp.de

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

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