[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 4/5] block: make size-related BlockConf properties accept
From: |
Roman Kagan |
Subject: |
Re: [PATCH v6 4/5] block: make size-related BlockConf properties accept size suffixes |
Date: |
Wed, 27 May 2020 23:53:11 +0300 |
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@yandex-team.ru>
> > ---
> > 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.
- [PATCH v6 1/5] virtio-blk: store opt_io_size with correct size, (continued)
- [PATCH v6 1/5] virtio-blk: store opt_io_size with correct size, Roman Kagan, 2020/05/27
- [PATCH v6 2/5] block: consolidate blocksize properties consistency checks, Roman Kagan, 2020/05/27
- [PATCH v6 3/5] qdev-properties: blocksize: use same limits in code and description, Roman Kagan, 2020/05/27
- [PATCH v6 5/5] block: lift blocksize property limit to 2 MiB, Roman Kagan, 2020/05/27
- [PATCH v6 4/5] block: make size-related BlockConf properties accept size suffixes, Roman Kagan, 2020/05/27