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

[Eric Blake <eblake@xxxxxxxxxx>]

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

> +        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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org