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

[Kevin Wolf]

Am 09.03.2011 18:03, schrieb Anthony Liguori:
> On 03/09/2011 10:01 AM, Kevin Wolf wrote:
>> Am 09.03.2011 16:46, schrieb Anthony Liguori:
>>> On 03/09/2011 09:37 AM, address@hidden wrote:
>>>> From: Jes Sorensen<address@hidden>
>>>>
>>>> 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<address@hidden>
>>>> ---
>>>>    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...
> 
> 'device' is a device name though, right?  Or is it a name associated 
> with a BlockDriverState that currently happens to be a qdev name?
> 
> Would I be able to eventually pass a qdev path here?
> 
> If the answer is that this is a bdrv_name, then should we at least use 
> blockdev instead of blkdev?

I think it's a drive name, like 'ide-hd0' or what they were called. I'm
not completely sure if this is the same namespace as blockdev_add would
use, but at first sight it would make some sense. Markus?

Kevin