Re: [Qemu-devel] [PATCH] QMP: add snapshot_blkdev_sync command

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/09/2011 09:37 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Add QMP bits for snapshot_blkdev_sync command. This is the same as
> snapshot_blkdev in the human monitor, but added _sync to the name to
> make it explicit that the command is synchronous and leave space for a
> future async version.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   qmp-commands.hx |   19 +++++++++++++++++++
>   1 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d3cc31..e32187e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -667,6 +667,25 @@ Example:
>   EQMP
>
>       {
> +        .name       = "snapshot_blkdev_sync",
> +        .args_type  = "device:B,snapshot_file:s?,format:s?",
> +        .params     = "device [new-image-file] [format]",
> +        .help       = "initiates a live snapshot\n\t\t\t"
> +                      "of device. If a new image file is specified, the\n\t\t\t"
> +                      "new image file will become the new root image.\n\t\t\t"
> +                      "If format is specified, the snapshot file will\n\t\t\t"
> +                      "be created in that format. Otherwise the\n\t\t\t"
> +                      "snapshot will be internal! (currently unsupported)",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_snapshot_blkdev,
> +    },
> +
> +SQMP
> +Synchronous snapshot of block device, using snapshot file as target
> +if provided.

Please document the error semantics.

The documentation in .help is discarded for QMP.  You should put the 
docs in the SQMP section.

Also, QMP should use '-' instead of '_'.  We should also try to follow 
the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync'

I'm not sure blkdev is the right prefix.  Kevin, what are your thoughts 
here?  Does 'blkdev' make sense for any command operating on a block 
device (that is, a qdev device that happens to have a block drive, not 
the same thing as -blockdev that we've discussed in the past).

Regards,

Anthony Liguori

> +EQMP
> +
> +    {
>           .name       = "balloon",
>           .args_type  = "value:M",
>           .params     = "target",