Re: [Qemu-devel] [PATCH 1/1] QMP: add snapshot_blkdev command

2011-07-11 Thread Jes Sorensen
On 07/11/11 18:35, Luiz Capitulino wrote:
> On Fri,  8 Jul 2011 14:00:13 +0200
> jes.soren...@redhat.com wrote:
> 
>> From: Jes Sorensen 
>>
>> Add QMP bits for snapshot_blkdev command. This is the same as
>> snapshot_blkdev in the human monitor. The command is synchronous.
>>
>> In the future async commands and or a break down of the functionality
>> into multiple commands might be added.
>>
>> Signed-off-by: Jes Sorensen 
>> ---
>>  qmp-commands.hx |   38 ++
>>  1 files changed, 38 insertions(+), 0 deletions(-)
>>
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 92c5c3a..eb135c1 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -694,6 +694,44 @@ Example:
>>  EQMP
>>  
>>  {
>> +.name   = "blockdev-snapshot",
> 
> blockdev-snapshot-sync.

argh, will fix

>> +.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)",
> 
> The 'otherwise' part is a bit confusing. You document something as if it were
> supported then you say it's not supported. I recommend to just not document it
> instead.

Ok

>> +.user_print = monitor_user_noop,
>> +.mhandler.cmd_new = do_snapshot_blkdev,
>> +},
>> +
>> +SQMP
>> +blockdev-snapshot-sync
>> +--
>> +
>> +Synchronous snapshot of block device, using snapshot file as target,
>> +if provided. 
> 
> In QMP only this text is used, the text in '.help' is discarded. Please,
> move all command documentation here.

If .help isn't used, then please remove it from the struct. It is really
pointless to keep carrying both around.

>> +
>> +Arguments:
>> +
>> +- "device": device name to snapshot (json-string)
>> +- "snapshot_file": name of new image file (json-string)
>> +- "format": format of new image (json-string)
>> +
>> +Example:
>> +
>> +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
>> +"snapshot_file":
> 
> We are trying to standardize the use of hyphen in QMP.

Sorry, but I haven't got a clue what you mean here.

Jes



Re: [Qemu-devel] [PATCH 1/1] QMP: add snapshot_blkdev command

2011-07-11 Thread Luiz Capitulino
On Mon, 11 Jul 2011 19:50:45 +0200
Jes Sorensen  wrote:

> On 07/11/11 18:35, Luiz Capitulino wrote:
> > On Fri,  8 Jul 2011 14:00:13 +0200
> > jes.soren...@redhat.com wrote:
> > 
> >> From: Jes Sorensen 
> >>
> >> Add QMP bits for snapshot_blkdev command. This is the same as
> >> snapshot_blkdev in the human monitor. The command is synchronous.
> >>
> >> In the future async commands and or a break down of the functionality
> >> into multiple commands might be added.
> >>
> >> Signed-off-by: Jes Sorensen 
> >> ---
> >>  qmp-commands.hx |   38 ++
> >>  1 files changed, 38 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index 92c5c3a..eb135c1 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -694,6 +694,44 @@ Example:
> >>  EQMP
> >>  
> >>  {
> >> +.name   = "blockdev-snapshot",
> > 
> > blockdev-snapshot-sync.
> 
> argh, will fix
> 
> >> +.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)",
> > 
> > The 'otherwise' part is a bit confusing. You document something as if it 
> > were
> > supported then you say it's not supported. I recommend to just not document 
> > it
> > instead.
> 
> Ok
> 
> >> +.user_print = monitor_user_noop,
> >> +.mhandler.cmd_new = do_snapshot_blkdev,
> >> +},
> >> +
> >> +SQMP
> >> +blockdev-snapshot-sync
> >> +--
> >> +
> >> +Synchronous snapshot of block device, using snapshot file as target,
> >> +if provided. 
> > 
> > In QMP only this text is used, the text in '.help' is discarded. Please,
> > move all command documentation here.
> 
> If .help isn't used, then please remove it from the struct. It is really
> pointless to keep carrying both around.

Yes, sorry for that. It will be completely dropped when we move to the QAPI
(which uses a schema file).

> 
> >> +
> >> +Arguments:
> >> +
> >> +- "device": device name to snapshot (json-string)
> >> +- "snapshot_file": name of new image file (json-string)
> >> +- "format": format of new image (json-string)
> >> +
> >> +Example:
> >> +
> >> +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
> >> +"snapshot_file":
> > 
> > We are trying to standardize the use of hyphen in QMP.
> 
> Sorry, but I haven't got a clue what you mean here.

The argument is called "snapshot_file" but it should be called snapshot-file.



Re: [Qemu-devel] [PATCH 1/1] QMP: add snapshot_blkdev command

2011-07-11 Thread Luiz Capitulino
On Fri,  8 Jul 2011 14:00:13 +0200
jes.soren...@redhat.com wrote:

> From: Jes Sorensen 
> 
> Add QMP bits for snapshot_blkdev command. This is the same as
> snapshot_blkdev in the human monitor. The command is synchronous.
> 
> In the future async commands and or a break down of the functionality
> into multiple commands might be added.
> 
> Signed-off-by: Jes Sorensen 
> ---
>  qmp-commands.hx |   38 ++
>  1 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92c5c3a..eb135c1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -694,6 +694,44 @@ Example:
>  EQMP
>  
>  {
> +.name   = "blockdev-snapshot",

blockdev-snapshot-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)",

The 'otherwise' part is a bit confusing. You document something as if it were
supported then you say it's not supported. I recommend to just not document it
instead.

> +.user_print = monitor_user_noop,
> +.mhandler.cmd_new = do_snapshot_blkdev,
> +},
> +
> +SQMP
> +blockdev-snapshot-sync
> +--
> +
> +Synchronous snapshot of block device, using snapshot file as target,
> +if provided. 

In QMP only this text is used, the text in '.help' is discarded. Please,
move all command documentation here.

> +
> +Arguments:
> +
> +- "device": device name to snapshot (json-string)
> +- "snapshot_file": name of new image file (json-string)
> +- "format": format of new image (json-string)
> +
> +Example:
> +
> +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
> +"snapshot_file":

We are trying to standardize the use of hyphen in QMP.

> +"/some/place/my-image",
> + "format": "qcow2"
> +   } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +{
>  .name   = "balloon",
>  .args_type  = "value:M",
>  .params = "target",




[Qemu-devel] [PATCH 1/1] QMP: add snapshot_blkdev command

2011-07-08 Thread Jes . Sorensen
From: Jes Sorensen 

Add QMP bits for snapshot_blkdev command. This is the same as
snapshot_blkdev in the human monitor. The command is synchronous.

In the future async commands and or a break down of the functionality
into multiple commands might be added.

Signed-off-by: Jes Sorensen 
---
 qmp-commands.hx |   38 ++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 92c5c3a..eb135c1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -694,6 +694,44 @@ Example:
 EQMP
 
 {
+.name   = "blockdev-snapshot",
+.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
+blockdev-snapshot-sync
+--
+
+Synchronous snapshot of block device, using snapshot file as target,
+if provided. 
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "snapshot_file": name of new image file (json-string)
+- "format": format of new image (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
+"snapshot_file":
+"/some/place/my-image",
+   "format": "qcow2"
+   } }
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "balloon",
 .args_type  = "value:M",
 .params = "target",
-- 
1.7.4.4