[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP com
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command |
Date: |
Fri, 11 Sep 2015 11:58:11 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
On 09/10/2015 07:39 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.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
> blockdev.c | 163
> ++++++++++++++++++++++++++++++++-------------------
> qapi-schema.json | 2 +
> qapi/block-core.json | 26 ++++++++
> qmp-commands.hx | 29 +++++++++
> 4 files changed, 160 insertions(+), 60 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 6b787c1..78cfb79 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,
> + Error **errp)
> +{
> + BlockdevSnapshot snapshot_data = {
> + .device = (char *) device,
> + .snapshot = (char *) snapshot
> + };
Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to
make this function have the saner signature of
void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);
rather than having to rebuild the struct yourself (with an annoying cast
to lose const) :)
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html
But no need to hold this series up waiting for the qapi review queue to
flush. We can simplify later.
>
> - 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 existing");
Pre-existing, but s/existing/exists/ while you are reindenting this.
> +++ b/qapi/block-core.json
> @@ -705,6 +705,19 @@
> '*format': 'str', '*mode': 'NewImageMode' } }
>
> ##
> +# @BlockdevSnapshot
> +#
> +# @device: device or node name to generate the snapshot from.
> +#
> +# @snapshot: reference to the existing block device that will be used
> +# for the snapshot.
Maybe mention that it must NOT have a current backing file, and point to
the "backing":"" trick to get it that way.
> +Create a snapshot, by installing '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.
Still didn't answer the question from the earlier review of whether the
blockdev-snapshot-sync behavior of specifying the node name of an active
layer in order to not pivot the block device to the snapshot still makes
sense to support in the blockdev-snapshot case. But we could always add
an optional boolean flag later if someone comes up for a use case where
they'd need to create a snapshot of the active layer without the block
device pivoting, so I don't think it should hold up this patch.
> +
> +Arguments:
> +
> +- "device": snapshot source (json-string)
> +- "snapshot": snapshot target (json-string)
> +
> +Example:
> +
> +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
> + "snapshot": "node1534" }
> }
> +<- { "return": {} }
Maybe enhance the example to show the preliminary blockdev-add that
created node1534?
I've pointed out some potential wording improvements, but think they are
minor enough that you can have:
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command, Alberto Garcia, 2015/09/10
- [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync, Alberto Garcia, 2015/09/10
- [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command, Alberto Garcia, 2015/09/10
- [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Alberto Garcia, 2015/09/10
- Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Max Reitz, 2015/09/11
- Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Kevin Wolf, 2015/09/11
- Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Max Reitz, 2015/09/11
- Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Alberto Garcia, 2015/09/14
- Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Kevin Wolf, 2015/09/14
- Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Alberto Garcia, 2015/09/14
Re: [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat, Eric Blake, 2015/09/11