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 <mlevi...@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berra...@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
