Re: [Qemu-block] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

Am 22.09.2015 um 15:28 hat Alberto Garcia geschrieben:
> 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 <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: Max Reitz <address@hidden>
> ---
>  blockdev.c           | 163 
> ++++++++++++++++++++++++++++++++-------------------
>  qapi-schema.json     |   2 +
>  qapi/block-core.json |  28 +++++++++
>  qmp-commands.hx      |  38 ++++++++++++
>  4 files changed, 171 insertions(+), 60 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1a5b889..daf72f3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const 
> char *device,
>                         &snapshot, errp);
>  }
>  
> +void qmp_blockdev_snapshot(const char *node, const char *overlay,
> +                           Error **errp)
> +{
> +    BlockdevSnapshot snapshot_data = {
> +        .node = (char *) node,
> +        .overlay = (char *) overlay
> +    };
> +
> +    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
> +                       &snapshot_data, errp);
> +}
> +
>  void qmp_blockdev_snapshot_internal_sync(const char *device,
>                                           const char *name,
>                                           Error **errp)
> @@ -1521,57 +1533,48 @@ typedef struct ExternalSnapshotState {
>  static void external_snapshot_prepare(BlkTransactionState *common,
>                                        Error **errp)
>  {
> -    int flags, ret;
> -    QDict *options;
> +    int flags = 0, ret;
> +    QDict *options = NULL;
>      Error *local_err = NULL;
> -    bool has_device = false;
> +    /* Device and node name of the image to generate the snapshot from */
>      const char *device;
> -    bool has_node_name = false;
>      const char *node_name;
> -    bool has_snapshot_node_name = false;
> -    const char *snapshot_node_name;
> +    /* Reference to the new image (for 'blockdev-snapshot') */
> +    const char *snapshot_ref;
> +    /* File name of the new image (for 'blockdev-snapshot-sync') */
>      const char *new_image_file;
> -    const char *format = "qcow2";
> -    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>      ExternalSnapshotState *state =
>                               DO_UPCAST(ExternalSnapshotState, common, 
> common);
>      TransactionAction *action = common->action;
>  
> -    /* 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;
> -    }
> -    if (action->blockdev_snapshot_sync->has_mode) {
> -        mode = action->blockdev_snapshot_sync->mode;
> +    /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
> +     * purpose but a different set of parameters */
> +    switch (action->kind) {
> +    case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
> +        {
> +            BlockdevSnapshot *s = action->blockdev_snapshot;
> +            device = s->node;
> +            node_name = s->node;
> +            new_image_file = NULL;
> +            snapshot_ref = s->overlay;
> +        }
> +        break;
> +    case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
> +        {
> +            BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
> +            device = s->has_device ? s->device : NULL;
> +            node_name = s->has_node_name ? s->node_name : NULL;
> +            new_image_file = s->snapshot_file;
> +            snapshot_ref = NULL;
> +        }
> +        break;
> +    default:
> +        g_assert_not_reached();
>      }
>  
>      /* 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_find_node(snapshot_node_name)) {
> -        error_setg(errp, "New snapshot node name already existing");
> +    state->old_bs = bdrv_lookup_bs(device, node_name, errp);
> +    if (!state->old_bs) {
>          return;
>      }
>  
> @@ -1601,35 +1604,69 @@ 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_find_node(snapshot_node_name)) {
> +            error_setg(errp, "New snapshot node name already exists");
> +            return;
> +        }

Preexisting, but shouldn't we use bdrv_lookup_bs() here (because devices
and node names share a namespace)?

Otherwise, we could decide to drop the check altogether (because
bdrv_open() already checks this), but then we would already have created
the new image.

> +
> +        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;
> +    }

Is it even possible yet to create a root BDS without a BB?

> +    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");
>      }

The error cases after bdrv_open() should probably bdrv_unref() the node.

Kevin