From: Eric Blake <eblake@redhat.com>
To: Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
qemu-block@nongnu.org, Max Reitz <mreitz@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command
Date: Fri, 11 Sep 2015 11:58:11 -0600 [thread overview]
Message-ID: <55F31633.6010800@redhat.com> (raw)
In-Reply-To: <40f3f51fb44da70929f7055e95582351d1aa3f34.1441890725.git.berto@igalia.com>
[-- Attachment #1: Type: text/plain, Size: 4447 bytes --]
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 <berto@igalia.com>
> ---
> 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 <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
next prev parent reply other threads:[~2015-09-11 17:58 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-10 13:39 [Qemu-devel] [PATCH v3 0/4] Add 'blockdev-snapshot' command Alberto Garcia
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync Alberto Garcia
2015-09-11 17:15 ` Max Reitz
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat Alberto Garcia
2015-09-11 17:21 ` Max Reitz
2015-09-11 17:28 ` Kevin Wolf
2015-09-11 17:33 ` Max Reitz
2015-09-14 5:54 ` Alberto Garcia
2015-09-14 8:45 ` Kevin Wolf
2015-09-14 14:20 ` Alberto Garcia
2015-09-11 17:28 ` Eric Blake
2015-09-11 17:30 ` [Qemu-devel] [Qemu-block] " Eric Blake
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command Alberto Garcia
2015-09-11 17:58 ` Eric Blake [this message]
2015-09-14 14:08 ` Alberto Garcia
2015-09-11 18:11 ` Max Reitz
2015-09-10 13:39 ` [Qemu-devel] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command Alberto Garcia
2015-09-11 18:02 ` Eric Blake
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55F31633.6010800@redhat.com \
--to=eblake@redhat.com \
--cc=berto@igalia.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.