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 <be...@igalia.com>
> Cc: Eric Blake <ebl...@redhat.com>
> Cc: Max Reitz <mre...@redhat.com>
> ---
> 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
