Re: [Qemu-devel] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command

[Eric Blake <eblake@redhat.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 --]