Re: [Qemu-block] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Max Reitz]

On 31.08.2015 12:00, Alberto Garcia wrote:
> Snapshots created using blockdev-snapshot-sync are currently opened
> using their default options, not even inheriting those from the images
> they are based on.
> 
> This patch extends the command by adding an 'options' parameter that
> takes a BlockdevOptions structure. Since some of the options that can
> be specified overlap with the parameters of blockdev-snapshot-sync,
> the following checks are made:
> 
> - The 'driver' field must match the format specified for the snapshot.
> - The 'node-name' field and the 'snapshot-node-name' parameters cannot
>   be specified at the same time.
> - The 'file' field can only contain an empty string, since it overlaps
>   with the 'snapshot-file' parameter.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  blockdev.c           | 59 
> ++++++++++++++++++++++++++++++++++++++++++++++------
>  hmp.c                |  2 +-
>  qapi/block-core.json |  9 +++++++-
>  qmp-commands.hx      |  3 ++-
>  4 files changed, 64 insertions(+), 9 deletions(-)

Design question: Would it make sense to instead add a "reference" mode
to blockdev-snapshot-sync where you can specify a BDS's node-name
instead of snapshot-file to use an existing BDS as the new top layer,
ideally an empty one?

What we'd then need is a QMP command for creating images. But as far as
I know we'll need that anyway sooner or later...


Comments on the patch itself below.

> diff --git a/blockdev.c b/blockdev.c
> index 4a1fc2e..7e904f3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1070,7 +1070,9 @@ void qmp_blockdev_snapshot_sync(bool has_device, const 
> char *device,
>                                  bool has_snapshot_node_name,
>                                  const char *snapshot_node_name,
>                                  bool has_format, const char *format,
> -                                bool has_mode, NewImageMode mode, Error 
> **errp)
> +                                bool has_mode, NewImageMode mode,
> +                                bool has_options, BlockdevOptions *options,
> +                                Error **errp)
>  {
>      BlockdevSnapshot snapshot = {
>          .has_device = has_device,
> @@ -1084,6 +1086,8 @@ void qmp_blockdev_snapshot_sync(bool has_device, const 
> char *device,
>          .format = (char *) format,
>          .has_mode = has_mode,
>          .mode = mode,
> +        .has_options = has_options,
> +        .options = options,
>      };
>      blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
>                         &snapshot, errp);
> @@ -1504,6 +1508,48 @@ static void 
> external_snapshot_prepare(BlkTransactionState *common,
>  
>      flags = state->old_bs->open_flags;
>  
> +    if (action->blockdev_snapshot_sync->has_options) {
> +        QObject *obj;
> +        QmpOutputVisitor *ov = qmp_output_visitor_new();
> +        visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> +                                   &action->blockdev_snapshot_sync->options,
> +                                   NULL, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            qmp_output_visitor_cleanup(ov);
> +            return;
> +        }
> +
> +        obj = qmp_output_get_qobject(ov);
> +        options = qobject_to_qdict(obj);
> +        qmp_output_visitor_cleanup(ov);
> +
> +        if (strcmp(format, qdict_get_str(options, "driver"))) {

With has_format == false, format will be set to "qcow2" by default. So,
if the user does not specify the format explicitly, the "driver" field
has to be set to "qcow2".

I'd rather make specifying @format and @options exclusive, and if
@options has been specified, its "driver" field should override the
"format" default.

> +            error_setg(errp, "Can't specify two different image formats");
> +            goto fail;
> +        }
> +
> +        if (has_snapshot_node_name && qdict_haskey(options, "node-name")) {
> +            error_setg(errp, "Can't specify a node name twice");
> +            goto fail;
> +        }
> +
> +        obj = qdict_get(options, "file");
> +        if (obj != NULL) {
> +            QString *str = qobject_to_qstring(obj);
> +            if (!str || strcmp(qstring_get_str(str), "")) {
> +                error_setg(errp, "The 'file' field must be empty");
> +                goto fail;
> +            }
> +            qdict_del(options, "file");
> +        }

And the "backing" field must be NULL.

> +
> +        qdict_flatten(options);
> +    } else {
> +        options = qdict_new();
> +        qdict_put(options, "driver", qstring_from_str(format));
> +    }
> +
>      /* create new image w/backing file */
>      if (mode != NEW_IMAGE_MODE_EXISTING) {
>          bdrv_img_create(new_image_file, format,
> @@ -1512,19 +1558,15 @@ static void 
> external_snapshot_prepare(BlkTransactionState *common,
>                          NULL, -1, flags, &local_err, false);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            return;
> +            goto fail;
>          }
>      }
>  
> -    options = qdict_new();
>      if (has_snapshot_node_name) {
>          qdict_put(options, "node-name",
>                    qstring_from_str(snapshot_node_name));
>      }
> -    qdict_put(options, "driver", qstring_from_str(format));
>  
> -    /* TODO Inherit bs->options or only take explicit options with an
> -     * extended QMP command? */
>      assert(state->new_bs == NULL);
>      ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>                      flags | BDRV_O_NO_BACKING, &local_err);
> @@ -1532,6 +1574,11 @@ static void 
> external_snapshot_prepare(BlkTransactionState *common,
>      if (ret != 0) {
>          error_propagate(errp, local_err);
>      }
> +
> +    return;
> +
> +fail:
> +    QDECREF(options);
>  }
>  
>  static void external_snapshot_commit(BlkTransactionState *common)
> diff --git a/hmp.c b/hmp.c
> index dcc66f1..180a7f6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1115,7 +1115,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict 
> *qdict)
>      qmp_blockdev_snapshot_sync(true, device, false, NULL,
>                                 filename, false, NULL,
>                                 !!format, format,
> -                               true, mode, &err);
> +                               true, mode, false, NULL, &err);
>      hmp_handle_error(mon, &err);
>  }
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7b2efb8..5eb111e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -697,11 +697,18 @@
>  #
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #        'absolute-paths'.
> +#
> +# @options: #optional options for the new device, with the following
> +#           restrictions for the fields: 'driver' must match the value
> +#           of @format,

As said above, I'd rather make specifying both @options and @format
exclusive.

Maybe there is even some QAPI magic to enforce that (and for
'node-name', too), I don't know...

Max

>                          'node-name' and @snapshot-node-name cannot be
> +#           specified at the same time, and 'file' can only contain an
> +#           empty string (Since 2.5)
>  ##
>  { 'struct': 'BlockdevSnapshot',
>    'data': { '*device': 'str', '*node-name': 'str',
>              'snapshot-file': 'str', '*snapshot-node-name': 'str',
> -            '*format': 'str', '*mode': 'NewImageMode' } }
> +            '*format': 'str', '*mode': 'NewImageMode',
> +            '*options': 'BlockdevOptions' } }
>  
>  ##
>  # @DriveBackup
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba630b1..bf45e31 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1394,7 +1394,7 @@ EQMP
>  
>      {
>          .name       = "blockdev-snapshot-sync",
> -        .args_type  = 
> "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
> +        .args_type  = 
> "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?,options:q?",
>          .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
>      },
>  
> @@ -1417,6 +1417,7 @@ Arguments:
>  - "mode": whether and how QEMU should create the snapshot file
>    (NewImageMode, optional, default "absolute-paths")
>  - "format": format of new image (json-string, optional)
> +- "options": options for the new image (json-dict)
>  
>  Example:
>  
>

signature.asc
Description: OpenPGP digital signature