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

[Kevin Wolf <kwolf@redhat.com>]

Am 09.03.2011 16:46, schrieb Anthony Liguori:
> 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).

Doesn't this command work on a -blockdev style thing, i.e.
BlockDriverState or DriveInfo? I don't think we have any commands that
refer to qdev devices that happen to be block devices. You could
probably argue that some of them should...

Kevin