Patchwork QMP: add snapshot_blkdev_sync command

login
register
mail settings
Submitter Jes Sorensen
Date March 9, 2011, 3:37 p.m.
Message ID <1299685061-24234-1-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/86123/
State New
Headers show

Comments

Jes Sorensen - March 9, 2011, 3:37 p.m.
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(-)
Anthony Liguori - March 9, 2011, 3:46 p.m.
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",
Kevin Wolf - March 9, 2011, 4:01 p.m.
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
Anthony Liguori - March 9, 2011, 5:03 p.m.
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
>
Jes Sorensen - March 10, 2011, 9:10 a.m.
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
Kevin Wolf - March 10, 2011, 9:21 a.m.
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

Patch

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",