Re: [PATCH v4 2/3] block: consolidate blocksize properties consistency checks

[Kevin Wolf]

Am 20.05.2020 um 10:57 hat Philippe Mathieu-Daudé geschrieben:
> Hi Roman,
> 
> On 5/20/20 10:06 AM, Roman Kagan wrote:
> > Several block device properties related to blocksize configuration must
> > be in certain relationship WRT each other: physical block must be no
> > smaller than logical block; min_io_size, opt_io_size, and
> > discard_granularity must be a multiple of a logical block.
> > 
> > To ensure these requirements are met, add corresponding consistency
> > checks to blkconf_blocksizes, adjusting its signature to communicate
> > possible error to the caller.  Also remove the now redundant consistency
> > checks from the specific devices.
> > 
> > Signed-off-by: Roman Kagan <address@hidden>
> > ---
> > v4: new patch
> > 
> >   include/hw/block/block.h   |  2 +-
> >   hw/block/block.c           | 29 ++++++++++++++++++++++++++++-
> >   hw/block/fdc.c             |  5 ++++-
> >   hw/block/nvme.c            |  5 ++++-
> >   hw/block/virtio-blk.c      |  7 +------
> >   hw/ide/qdev.c              |  5 ++++-
> >   hw/scsi/scsi-disk.c        | 10 +++-------
> >   hw/usb/dev-storage.c       |  5 ++++-
> >   tests/qemu-iotests/172.out |  2 +-
> >   9 files changed, 50 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index d7246f3862..784953a237 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -87,7 +87,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void 
> > *buf, hwaddr size,
> >   bool blkconf_geometry(BlockConf *conf, int *trans,
> >                         unsigned cyls_max, unsigned heads_max, unsigned 
> > secs_max,
> >                         Error **errp);
> > -void blkconf_blocksizes(BlockConf *conf);
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp);
> >   bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
> >                                      bool resizable, Error **errp);
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index bf56c7612b..5f8ebff59c 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -61,7 +61,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void 
> > *buf, hwaddr size,
> >       return true;
> >   }
> > -void blkconf_blocksizes(BlockConf *conf)
> > +bool blkconf_blocksizes(BlockConf *conf, Error **errp)
> >   {
> >       BlockBackend *blk = conf->blk;
> >       BlockSizes blocksizes;
> > @@ -83,6 +83,33 @@ void blkconf_blocksizes(BlockConf *conf)
> >               conf->logical_block_size = BDRV_SECTOR_SIZE;
> >           }
> >       }
> > +
> > +    if (conf->logical_block_size > conf->physical_block_size) {
> > +        error_setg(errp,
> > +                   "logical_block_size > physical_block_size not 
> > supported");
> 
> "not supported" or "invalid"?

I'm not sure about strictly invalid, though it's certainly a weird case.
But there is enough weird stuff in real hardware...

"not supported" is correct either case, so I think the message is fine.

> > +        return false;
> > +    }
> > +
> > +    if (conf->min_io_size % conf->logical_block_size) {
> 
> It seems the block code usually do:
> 
>        if (!QEMU_IS_ALIGNED(conf->min_io_size, conf->logical_block_size)) {
> 
> > +        error_setg(errp,
> > +                   "min_io_size must be a multple of logical_block_size");
> 
> Typo "multple" -> "multiple".
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->opt_io_size % conf->logical_block_size) {
> > +        error_setg(errp,
> > +                   "opt_io_size must be a multple of logical_block_size");
> 
> Ditto.
> 
> > +        return false;
> > +    }
> > +
> > +    if (conf->discard_granularity != -1 &&
> > +        conf->discard_granularity % conf->logical_block_size) {
> > +        error_setg(errp, "discard_granularity must be "
> > +                   "a multple of logical_block_size");
> 
> Again.
> 
> > +        return false;
> > +    }
> > +
> > +    return true;
> 
> Usually we return true for error, isn't it?

I expect int functions to return 0 for success and -errno for failure,
but bool functions to return true for success and false for failure.
I'm not sure if this varies across the code base, but it is the general
pattern in the block subsystem at least.

I agree with your comments about QEMU_IS_ALIGNED() (both for min_io_size
and opt_io_size) and the typos, though.

Kevin