On 09/04/2015 06:37 AM, 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.
I like it. Obviously, we have to get the pre-requisites in first, to
allow opening a BDS without a BB, but it is nicer overall.
>
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> ---
> blockdev.c | 154
> ++++++++++++++++++++++++++++++++-------------------
> qapi-schema.json | 1 +
> qapi/block-core.json | 26 +++++++++
> qmp-commands.hx | 28 ++++++++++
> 4 files changed, 153 insertions(+), 56 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 6b787c1..db6e3bf 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 *device, const char *snapshot,
Is 'node' a better name than 'device' here?
> Error **errp)
> @@ -1521,60 +1533,52 @@ typedef struct ExternalSnapshotState {
> static void external_snapshot_prepare(BlkTransactionState *common,
> Error **errp)
> {
> + /* '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->device;
> + node_name = s->device;
Again, I think this should be setting s->node_name, not s->device.
> + new_image_file = NULL;
> + snapshot_ref = s->snapshot;
> + }
> + 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);
> + state->old_bs = bdrv_lookup_bs(device, node_name, &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");
> - return;
> - }
> -
> /* Acquire AioContext now so any threads operating on old_bs stop */
> state->aio_context = bdrv_get_aio_context(state->old_bs);
> aio_context_acquire(state->aio_context);
> @@ -1601,35 +1605,67 @@ static void
> external_snapshot_prepare(BlkTransactionState *common,
> return;
> }
>
> - flags = state->old_bs->open_flags;
> -
> - /* 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) {
> + if (snapshot_ref) {
> + if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
> error_propagate(errp, local_err);
> return;
> }
> }
Shouldn't you also check that snapshot_ref does not currently have a
backing BDS (as it is the act of creating the snapshot that sets the
current device as the backing of the snapshot_ref BDS before altering
the BB to point to snapshot_ref as its new BDS)?
> +++ b/qapi-schema.json
> @@ -1496,6 +1496,7 @@
> ##
> { 'union': 'TransactionAction',
> 'data': {
> + 'blockdev-snapshot': 'BlockdevSnapshot',
Missing 'since 2.5' documentation.
> 'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
> 'drive-backup': 'DriveBackup',
> 'blockdev-backup': 'BlockdevBackup',
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ec50f06..adfae23 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -705,6 +705,19 @@
> '*format': 'str', '*mode': 'NewImageMode' } }
>
> ##
> +# @BlockdevSnapshot
> +#
> +# @device: device or node name to generate the snapshot from.
Would naming this 'node' make more sense?
> +#
> +# @snapshot: reference to the existing block device that will be used
> +# for the snapshot.
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'BlockdevSnapshot',
> + 'data': { 'device': 'str', 'snapshot': 'str' } }
> +
> +##
> # @DriveBackup
> #
> # @device: the name of the device which should be copied.
> @@ -800,6 +813,19 @@
> { 'command': 'blockdev-snapshot-sync',
> 'data': 'BlockdevSnapshotSync' }
>
> +
> +##
> +# @blockdev-snapshot
> +#
> +# Generates a snapshot of a block device.
> +#
> +# For the arguments, see the documentation of BlockdevSnapshot.
> +#
> +# Since 2.5
> +##
> +{ 'command': 'blockdev-snapshot',
> + 'data': 'BlockdevSnapshot' }
> +
Overall, I like the syntax.
> +SQMP
> +blockdev-snapshot
> +-----------------
> +Since 2.5
> +
> +Create a snapshot of a block device. 'device' and 'snapshot' both
> +refer to existing block devices. The former is the one to generate the
> +snapshot from, and the latter is the target of the snapshot.
Is there any better terminology? Maybe:
The act of creating a snapshot installs 'device' as the backing image of
'snapshot'; additionally, if 'device' is associated with a block device,
the block device changes to using 'snapshot' as its new active image.
Hmm - I wonder if that means we should have an optional boolean
parameter that allows us to avoid the automatic pivot. After all, with
'blockdev-snapshot-sync', you can specify 'device' and omit 'node-name'
to update the device's active layer, or you can omit 'device' and
specify 'node-name' to create another qcow2 file but NOT install it as
the active layer, regardless of which 'node-name' that serves as the
starting point. So when 'node-name' is the BDS node that 'device' is
currently visiting, you have control over whether 'device' auto-updates
to the new BDS.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
