Re: [Qemu-devel] [PATCH V5 7/7] qmp: Allow to take external snapshots on bs graphs node.

[Fam Zheng <famz@redhat.com>]

On Thu, 12/12 16:34, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  blockdev.c       | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  hmp.c            |  4 +++-
>  qapi-schema.json | 13 ++++++++++---
>  qmp-commands.hx  | 11 ++++++++++-
>  4 files changed, 71 insertions(+), 12 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 374d03d..1246544 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -940,14 +940,22 @@ static void blockdev_do_action(int kind, void *data, Error **errp)
>      qmp_transaction(&list, errp);
>  }
>  
> -void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
> +void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
> +                                bool has_node_name, const char *node_name,
> +                                const char *snapshot_file,
> +                                bool has_snapshot_node_name,
> +                                const char *snapshot_node_name,
>                                  bool has_format, const char *format,
> -                                bool has_mode, enum NewImageMode mode,
> -                                Error **errp)
> +                                bool has_mode, NewImageMode mode, Error **errp)
>  {
>      BlockdevSnapshot snapshot = {
> +        .has_device = has_device,
>          .device = (char *) device,
> +        .has_node_name = has_node_name,
> +        .node_name = (char *) node_name,
>          .snapshot_file = (char *) snapshot_file,
> +        .has_snapshot_node_name = has_snapshot_node_name,
> +        .snapshot_node_name = (char *) snapshot_node_name,
>          .has_format = has_format,
>          .format = (char *) format,
>          .has_mode = has_mode,
> @@ -1185,8 +1193,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>  {
>      BlockDriver *drv;
>      int flags, ret;
> +    QDict *options = NULL;
>      Error *local_err = NULL;
> +    bool has_device = false;
>      const char *device;
> +    bool has_node_name = false;
> +    const char *node_name;
> +    bool has_snapshot_node_name = false;
> +    const char *snapshot_node_name;
>      const char *new_image_file;
>      const char *format = "qcow2";
>      enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> @@ -1197,7 +1211,14 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>      /* get parameters */
>      g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
>  
> +    has_device = action->blockdev_snapshot_sync->has_device;
>      device = action->blockdev_snapshot_sync->device;
> +    has_node_name = action->blockdev_snapshot_sync->has_node_name;
> +    node_name = action->blockdev_snapshot_sync->node_name;
> +    has_snapshot_node_name =
> +        action->blockdev_snapshot_sync->has_snapshot_node_name;
> +    snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
> +
>      new_image_file = action->blockdev_snapshot_sync->snapshot_file;
>      if (action->blockdev_snapshot_sync->has_format) {
>          format = action->blockdev_snapshot_sync->format;
> @@ -1213,9 +1234,21 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>          return;
>      }
>  
> -    state->old_bs = bdrv_find(device);
> -    if (!state->old_bs) {
> -        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +    state->old_bs = bdrv_lookup_bs(has_device, device,
> +                                   has_node_name, node_name,
> +                                   &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (has_node_name && !has_snapshot_node_name) {
> +        error_setg(errp, "New snapshot node name missing");
> +        return;
> +    }
> +
> +    if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> +        error_setg(errp, "New snapshot node name already existing");
>          return;
>      }
>  
> @@ -1255,15 +1288,23 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>          }
>      }
>  
> +    if (has_snapshot_node_name) {
> +        options = qdict_new();
> +        qdict_put(options, "node-name",
> +                  qstring_from_str(snapshot_node_name));
> +    }
> +
>      /* We will manually add the backing_hd field to the bs later */
>      state->new_bs = bdrv_new("");
>      /* TODO Inherit bs->options or only take explicit options with an
>       * extended QMP command? */
> -    ret = bdrv_open(state->new_bs, new_image_file, NULL,
> +    ret = bdrv_open(state->new_bs, new_image_file, options,
>                      flags | BDRV_O_NO_BACKING, drv, &local_err);
>      if (ret != 0) {
>          error_propagate(errp, local_err);
>      }
> +
> +    QDECREF(options);
>  }
>  
>  static void external_snapshot_commit(BlkTransactionState *common)
> diff --git a/hmp.c b/hmp.c
> index 906ddb7..47dcf0c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -971,7 +971,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>      }
>  
>      mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> -    qmp_blockdev_snapshot_sync(device, filename, !!format, format,
> +    qmp_blockdev_snapshot_sync(true, device, false, NULL,
> +                               filename, false, NULL,
> +                               !!format, format,
>                                 true, mode, &errp);
>      hmp_handle_error(mon, &errp);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3977619..d7afb69 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1759,18 +1759,25 @@
>  ##
>  # @BlockdevSnapshot
>  #
> -# @device:  the name of the device to generate the snapshot from.
> +# Either @device or @node-name must be set but not both.
> +#
> +# @device: #optional the name of the device to generate the snapshot from.
> +#
> +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0)
>  #
>  # @snapshot-file: the target of the new image. A new file will be created.
>  #
> +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0)
> +#
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.
>  #
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #        'absolute-paths'.
>  ##
>  { 'type': 'BlockdevSnapshot',
> -  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode' } }
> +  'data': { '*device': 'str', '*node-name': 'str',
> +            'snapshot-file': 'str', '*snapshot-node-name': 'str',
> +            '*format': 'str', '*mode': 'NewImageMode' } }
>  
>  ##
>  # @BlockdevSnapshotInternal
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5696b08..b62b0f5 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1038,7 +1038,9 @@ actions array:
>      - "data": a dictionary.  The contents depend on the value
>        of "type".  When "type" is "blockdev-snapshot-sync":
>        - "device": device name to snapshot (json-string)
> +      - "node-name": graph node name to snapshot (json-string)
>        - "snapshot-file": name of new image file (json-string)
> +      - "snapshot-node-name": graph node name of the new snapshot (json-string)
>        - "format": format of new image (json-string, optional)
>        - "mode": whether and how QEMU should create the snapshot file
>          (NewImageMode, optional, default "absolute-paths")
> @@ -1053,6 +1055,11 @@ Example:
>           { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd0",
>                                           "snapshot-file": "/some/place/my-image",
>                                           "format": "qcow2" } },
> +         { 'type': 'blockdev-snapshot-sync', 'data' : { "node-name": "myfile",
> +                                         "snapshot-file": "/some/place/my-image2",
> +                                         "snapshot-node-name": "node3432",
> +                                         "mode": "existing",
> +                                         "format": "qcow2" } },
>           { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
>                                           "snapshot-file": "/some/place/my-image2",
>                                           "mode": "existing",
> @@ -1066,7 +1073,7 @@ EQMP
>  
>      {
>          .name       = "blockdev-snapshot-sync",
> -        .args_type  = "device:B,snapshot-file:s,format:s?,mode:s?",
> +        .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
>          .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
>      },
>  
> @@ -1083,7 +1090,9 @@ snapshot image, default is qcow2.
>  Arguments:
>  
>  - "device": device name to snapshot (json-string)
> +- "node-name": graph node name to snapshot (json-string)
>  - "snapshot-file": name of new image file (json-string)
> +- "snapshot-node-name": graph node name of the new snapshot (json-string)
>  - "mode": whether and how QEMU should create the snapshot file
>    (NewImageMode, optional, default "absolute-paths")
>  - "format": format of new image (json-string, optional)
> -- 
> 1.8.3.2
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>