Re: [PATCH v6 11/14] block/core: add generic infrastructure for x-blockdev-amend qmp command

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2020-05-14 at 17:47 +0200, Max Reitz wrote:
> On 10.05.20 15:40, Maxim Levitsky wrote:
> > blockdev-amend will be used similiar to blockdev-create
> > to allow on the fly changes of the structure of the format based block devices.
> > 
> > Current plan is to first support encryption keyslot management for luks
> > based formats (raw and embedded in qcow2)
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  block/Makefile.objs       |   2 +-
> >  block/amend.c             | 108 ++++++++++++++++++++++++++++++++++++++
> >  include/block/block_int.h |  21 +++++---
> >  qapi/block-core.json      |  42 +++++++++++++++
> >  qapi/job.json             |   4 +-
> >  5 files changed, 169 insertions(+), 8 deletions(-)
> >  create mode 100644 block/amend.c
> 
> [...]
> 
> > diff --git a/block/amend.c b/block/amend.c
> > new file mode 100644
> > index 0000000000..4840c0ffef
> > --- /dev/null
> > +++ b/block/amend.c
> 
> [...]
> 
> > +void qmp_x_blockdev_amend(const char *job_id,
> > +                          const char *node_name,
> > +                          BlockdevAmendOptions *options,
> > +                          bool has_force,
> > +                          bool force,
> > +                          Error **errp)
> > +{
> > +    BlockdevAmendJob *s;
> > +    const char *fmt = BlockdevDriver_str(options->driver);
> > +    BlockDriver *drv = bdrv_find_format(fmt);
> > +    BlockDriverState *bs = bdrv_find_node(node_name);
> > +
> > +    /*
> > +     * If the driver is in the schema, we know that it exists.
> 
> I wonder why create.c then still checks whether drv == NULL.
It because I copy&pasta'ed that code when this fix wasn't there.
I'll copy it to amend.c, since this check doesn't hurt anyway.

I do wonder if we should make a common function for that 
can check that!= NULL and whitelist? Thoughts?


> 
> I wouldn’t count on the schema.  For example, with modularized block
> driver I could imagine that a driver appears in the schema, but loading
> the module fails, so that drv still ends up NULL.
> 
> >         But it may not
> > +     * be whitelisted.
> > +     */
> > +    assert(drv);
> > +    if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, false)) {
> > +        error_setg(errp, "Driver is not whitelisted");
> > +        return;
> > +    }
> 
> [...]
> 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 0a71357b50..fdb0cdbcdd 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -133,12 +133,27 @@ struct BlockDriver {
> >      int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> >                            Error **errp);
> >      void (*bdrv_close)(BlockDriverState *bs);
> > +
> > +
> >      int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> >                                         Error **errp);
> >      int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv,
> >                                              const char *filename,
> >                                              QemuOpts *opts,
> >                                              Error **errp);
> > +
> > +    int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs,
> > +                                      BlockdevAmendOptions *opts,
> > +                                      bool force,
> > +                                      Error **errp);
> > +
> > +    int (*bdrv_amend_options)(BlockDriverState *bs,
> > +                              QemuOpts *opts,
> > +                              BlockDriverAmendStatusCB *status_cb,
> > +                              void *cb_opaque,
> > +                              bool force,
> > +                              Error **errp);
> > +
> >      int (*bdrv_make_empty)(BlockDriverState *bs);
> >  
> >      /*
> > @@ -433,12 +448,6 @@ struct BlockDriver {
> >                                        BdrvCheckResult *result,
> >                                        BdrvCheckMode fix);
> >  
> > -    int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
> > -                              BlockDriverAmendStatusCB *status_cb,
> > -                              void *cb_opaque,
> > -                              bool force,
> > -                              Error **errp);
> > -
> 
> No harm done, but why not just leave it where it was?
I wanted to have both legacy and qmp amend interfaces appear close
in the header file.

> 
> >      void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event);
> >  
> >      /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 943df1926a..74db515414 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4649,6 +4649,48 @@
> >    'data': { 'job-id': 'str',
> >              'options': 'BlockdevCreateOptions' } }
> >  
> > +##
> > +# @BlockdevAmendOptions:
> > +#
> > +# Options for amending an image format
> > +#
> > +# @driver           block driver that is suitable for the image
> 
> Missing colon after @driver.  Also, what does “suitable” mean?
> Shouldn’t it be exactly the node’s driver?  (i.e. “Block driver of the
> node to amend”)
Fixed!

> 
> Max
> 

Best regards,
	Maxim Levitsky