Re: [Qemu-devel] [PATCH v2 1/1] Add QMP bits for blockdev-snapshot-sync.

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 18 Apr 2011 16:27:01 +0200
Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This is quivalent to snapshot_blkdev in the human monitor, with _sync
> added to the command name to make it explicit that the command is
> synchronous and leave space for a future async version.

I'm not sure appending "_sync" is such a good convention, most commands
are sync today and they don't have it. I'd prefer to call it snapshot_blkdev
and note in the documentation how it works.

On the other hand, I'm not sure how Anthony is going to model async
commands, so maybe he has a better suggestion.

> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  qmp-commands.hx |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fbd98ee..b8f537c 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -667,6 +667,32 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "blockdev-snapshot-sync",
> +        .args_type  = "device:B,snapshot_file:s?,format:s?",
> +        .params     = "device [new-image-file] [format]",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_snapshot_blkdev,
> +    },
> +
> +SQMP

This doesn't follow QMP doc convention, which is:

command name
------------

(Explain how the command works, like you do below)

Arguments

Example

> +Synchronous snapshot of block device, using snapshot file as target
> +if provided. 

It's not optional in HMP:

 (qemu) snapshot_blkdev ide0-hd0
 Parameter 'snapshot_file' is missing
 (qemu)

And the command argument is called "snapshot_file" not "new-image-file"
as written in the HMP help text.

> +
> +If a new image file is specified, the new image file will become the
> +new root image. If format is specified, the snapshot file will be
> +created in that format. Otherwise the snapshot will be internal!
> +(currently unsupported).

Sorry for the stupid question, but what's a "new root image"? Also, all
these assumptions seem human features to me, as it can save some typing
and I can poke around to see where the snapshots are stored.

All arguments should be mandatory in QMP, IMO.

Finally, what's the expect behavior when -snapshot is used? I'm getting
this:

 (qemu) snapshot_blkdev ide0-hd0 snap-test
 Could not open '/tmp/vl.6w8YXA'
 (qemu)

At first, I don't see why we shouldn't generate the live snapshot, but anyway,
any special behavior like this should be noted in the section called Notes
in the command's documentation.

> +
> +Errors:
> +If creating the new snapshot image fails, QEMU will continue running
> +on the original image. If switching to the newly created image fails,
> +it will be attempted to fall back to the original image and return
> +QERR_OPEN_FILE_FAILED with the snapshot filename. If re-opening
> +the original image fails, QERR_OPEN_FILE_FAILED will be returned with
> +the original image filename.
> +EQMP
> +
> +    {
>          .name       = "balloon",
>          .args_type  = "value:M",
>          .params     = "target",