Re: [Qemu-devel] [PATCH v7 4/5] block: add a 'blockdev-snapshot' QMP command

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6413 bytes --]

On 12.10.2015 11:16, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
> 
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
> 
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
> 
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c           | 165 ++++++++++++++++++++++++++++++++-------------------
>  qapi-schema.json     |   2 +
>  qapi/block-core.json |  28 +++++++++
>  qmp-commands.hx      |  38 ++++++++++++
>  4 files changed, 172 insertions(+), 61 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 12741a0..b5470c9 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -1521,58 +1533,48 @@ typedef struct ExternalSnapshotState {

[...]

>      }
>  
>      /* start processing */
> -    state->old_bs = bdrv_lookup_bs(has_device ? device : NULL,
> -                                   has_node_name ? node_name : NULL,
> -                                   &local_err);
> -    if (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_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
> -        error_setg(errp, "New snapshot node name already in use");

There's a difference from v6 here...

> +    state->old_bs = bdrv_lookup_bs(device, node_name, errp);
> +    if (!state->old_bs) {
>          return;
>      }
>  
> @@ -1602,35 +1604,70 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>          return;
>      }
>  
> -    flags = state->old_bs->open_flags;
> +    if (action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
> +        BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
> +        const char *format = s->has_format ? s->format : "qcow2";
> +        enum NewImageMode mode;
> +        const char *snapshot_node_name =
> +            s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
>  
> -    /* create new image w/backing file */
> -    if (mode != NEW_IMAGE_MODE_EXISTING) {
> -        bdrv_img_create(new_image_file, format,
> -                        state->old_bs->filename,
> -                        state->old_bs->drv->format_name,
> -                        NULL, -1, flags, &local_err, false);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        if (node_name && !snapshot_node_name) {
> +            error_setg(errp, "New snapshot node name missing");
>              return;
>          }
> -    }
>  
> -    options = qdict_new();
> -    if (has_snapshot_node_name) {
> -        qdict_put(options, "node-name",
> -                  qstring_from_str(snapshot_node_name));
> +        if (snapshot_node_name &&
> +            bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
> +            error_setg(errp, "New snapshot node name already in use");

...and here, but how to resolve the conflict resulting from the newly
added patch 1 was obvious, so my R-b stands, of course.

Anyway, this is not why I'm replying, that's further down:

> +            return;
> +        }
> +
> +        flags = state->old_bs->open_flags;
> +
> +        /* create new image w/backing file */
> +        mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +        if (mode != NEW_IMAGE_MODE_EXISTING) {
> +            bdrv_img_create(new_image_file, format,
> +                            state->old_bs->filename,
> +                            state->old_bs->drv->format_name,
> +                            NULL, -1, flags, &local_err, false);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +        }
> +
> +        options = qdict_new();
> +        if (s->has_snapshot_node_name) {
> +            qdict_put(options, "node-name",
> +                      qstring_from_str(snapshot_node_name));
> +        }
> +        qdict_put(options, "driver", qstring_from_str(format));
> +
> +        flags |= BDRV_O_NO_BACKING;
>      }
> -    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);
> +    ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
> +                    flags, errp);
>      /* We will manually add the backing_hd field to the bs later */
>      if (ret != 0) {
> -        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (state->new_bs->blk != NULL) {
> +        error_setg(errp, "The snapshot is already in use by %s",
> +                   blk_name(state->new_bs->blk));
> +        return;
> +    }
> +
> +    if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> +                           errp)) {
> +        return;
> +    }
> +
> +    if (state->new_bs->backing_hd != NULL) {
> +        error_setg(errp, "The snapshot already has a backing image");
>      }

It's here: In case Kevin's series is applied before this one (which I'm
assuming since you were brave enough to base this series on my BB
series), this needs to be s/backing_hd/backing/. I'm just saying this so
you know you can keep my R-b then.

Max

>  }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]