Re: [PATCH v6 4/5] block: make size-related BlockConf properties accept size suffixes

[Roman Kagan <rvkagan@xxxxxxxxxxxxxx>]

On Wed, May 27, 2020 at 09:50:39AM -0500, Eric Blake wrote:
> On 5/27/20 7:45 AM, Roman Kagan wrote:
> > Several BlockConf properties represent respective sizes in bytes so it
> > makes sense to accept size suffixes for them.
> > 
> > Turn them all into uint32_t and use size-suffix-capable setters/getters
> > on them; introduce DEFINE_PROP_SIZE32 and adjust DEFINE_PROP_BLOCKSIZE
> > for that. (Making them uint64_t and reusing DEFINE_PROP_SIZE isn't
> > justified because guests expect at most 32bit values.)
> > 
> > Also, since min_io_size is exposed to the guest by scsi and virtio-blk
> > devices as an uint16_t in units of logical blocks, introduce an
> > additional check in blkconf_blocksizes to prevent its silent truncation.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@xxxxxxxxxxxxxx>
> > ---
> > v5 -> v6:
> > - add prop_size32 instead of going with 64bit
> 
> Would it be worth adding prop_size32 as its own patch, before using it here?

I've no strong opinion on this.  Should I better split it out when
respinning?

> But I'll review this as-is.
> 
> > +++ b/hw/block/block.c
> > @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> >           return false;
> >       }
> > +    /*
> > +     * all devices which support min_io_size (scsi and virtio-blk) expose 
> > it to
> > +     * the guest as a uint16_t in units of logical blocks
> > +     */
> > +    if (conf->min_io_size > conf->logical_block_size * UINT16_MAX) {
> 
> This risks overflow.  Better would be:
> 
> if (conf->min_io_size / conf->logical_block-size > UINT16_MAX)

D'oh.  I kept it in mind and did it right initially in v4, but then
mixed it up.  Thanks for catching!

> 
> > +        error_setg(errp,
> > +                   "min_io_size must not exceed " stringify(UINT16_MAX)
> > +                   " logical blocks");
> > +        return false;
> > +    }
> > +
> >       if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) {
> >           error_setg(errp,
> >                      "opt_io_size must be a multiple of 
> > logical_block_size");
> 
> > +++ b/tests/qemu-iotests/172.out
> > @@ -24,11 +24,11 @@ Testing:
> >                 dev: floppy, id ""
> >                   unit = 0 (0x0)
> >                   drive = "floppy0"
> > -                logical_block_size = 512 (0x200)
> > -                physical_block_size = 512 (0x200)
> > -                min_io_size = 0 (0x0)
> > -                opt_io_size = 0 (0x0)
> > -                discard_granularity = 4294967295 (0xffffffff)
> > +                logical_block_size = 512 (512 B)
> > +                physical_block_size = 512 (512 B)
> > +                min_io_size = 0 (0 B)
> > +                opt_io_size = 0 (0 B)
> > +                discard_granularity = 4294967295 (4 GiB)
> 
> Although 4 GiB is not quite the same as 4294967295, the exact byte value
> next to the approximate size is not too bad.  The mechanical fallout from
> the change from int to size is fine to me.

Thanks,
Roman.