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

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

[Jes.Sorensen]

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. 
+EQMP
+
+    {
         .name       = "balloon",
         .args_type  = "value:M",
         .params     = "target",
-- 
1.7.4

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

[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).

Regards,

Anthony Liguori

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

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

[Kevin Wolf]

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

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

[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, 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...

'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?

Regards,

Anthony Liguori



> Kevin
>

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

[Jes Sorensen]

On 03/09/11 18:03, Anthony Liguori wrote:
> On 03/09/2011 10:01 AM, Kevin Wolf wrote:
>>> 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?

It is a device in the sense of what you get from 'info block' in the
human monitor.

> 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 used blkdev in the human monitor command to reduce the length of the
command name.

Anyway, if you can decide what you want to call the command, I'll rename
it accordingly.

Cheers,
Jes

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, 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...
> 
> '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