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 <be...@igalia.com> > Cc: Eric Blake <ebl...@redhat.com> > Reviewed-by: Max Reitz <mre...@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
> }
>
signature.asc
Description: OpenPGP digital signature
