Patchwork [v3] QMP: add snapshot_blkdev command

login
register
mail settings
Submitter Luiz Capitulino
Date July 11, 2011, 8:24 p.m.
Message ID <20110711172456.5f61adbb@doriath>
Download mbox | patch
Permalink /patch/104283/
State New
Headers show

Comments

Luiz Capitulino - July 11, 2011, 8:24 p.m.
On Mon, 11 Jul 2011 20:01:09 +0200
Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> 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 <Jes.Sorensen@redhat.com>
> ---
>  qmp-commands.hx |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92c5c3a..2b9f6af 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -694,6 +694,41 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "blockdev-snapshot-sync",
> +        .args_type  = "device:B,snapshot-file:s?,format:s?",

You changed from an underline to a hyphen as I asked, but the implementation
still expects an underline. I fixed that myself to avoid multiple submissions
because of a small thing (also fixed the command name in the subject).

The patch I merged follows for reference. Please, test your patches before
submitting next time.


Subject: [PATCH] QMP: add snapshot-blkdev-sync command

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.

Also change the 'snapshot_file' argument to 'snapshot-file' in
the human monitor, so that it matches QMP.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@gmail.com>
---
 blockdev.c      |    4 ++--
 hmp-commands.hx |    2 +-
 qmp-commands.hx |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)
Jes Sorensen - July 11, 2011, 8:28 p.m.
On 07/11/11 22:24, Luiz Capitulino wrote:
> On Mon, 11 Jul 2011 20:01:09 +0200
> Jes.Sorensen@redhat.com wrote:
> 
>> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> > 
>> > 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 <Jes.Sorensen@redhat.com>
>> > ---
>> >  qmp-commands.hx |   35 +++++++++++++++++++++++++++++++++++
>> >  1 files changed, 35 insertions(+), 0 deletions(-)
>> > 
>> > diff --git a/qmp-commands.hx b/qmp-commands.hx
>> > index 92c5c3a..2b9f6af 100644
>> > --- a/qmp-commands.hx
>> > +++ b/qmp-commands.hx
>> > @@ -694,6 +694,41 @@ Example:
>> >  EQMP
>> >  
>> >      {
>> > +        .name       = "blockdev-snapshot-sync",
>> > +        .args_type  = "device:B,snapshot-file:s?,format:s?",
> You changed from an underline to a hyphen as I asked, but the implementation
> still expects an underline. I fixed that myself to avoid multiple submissions
> because of a small thing (also fixed the command name in the subject).
> 
> The patch I merged follows for reference. Please, test your patches before
> submitting next time.
> 
> 

Sorry that is no go, you just broke the hmp implementation - you cannot
change the hmp behavior like that.

Jes
Luiz Capitulino - July 11, 2011, 8:35 p.m.
On Mon, 11 Jul 2011 22:28:57 +0200
Jes Sorensen <Jes.Sorensen@redhat.com> wrote:

> On 07/11/11 22:24, Luiz Capitulino wrote:
> > On Mon, 11 Jul 2011 20:01:09 +0200
> > Jes.Sorensen@redhat.com wrote:
> > 
> >> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> > 
> >> > 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 <Jes.Sorensen@redhat.com>
> >> > ---
> >> >  qmp-commands.hx |   35 +++++++++++++++++++++++++++++++++++
> >> >  1 files changed, 35 insertions(+), 0 deletions(-)
> >> > 
> >> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> > index 92c5c3a..2b9f6af 100644
> >> > --- a/qmp-commands.hx
> >> > +++ b/qmp-commands.hx
> >> > @@ -694,6 +694,41 @@ Example:
> >> >  EQMP
> >> >  
> >> >      {
> >> > +        .name       = "blockdev-snapshot-sync",
> >> > +        .args_type  = "device:B,snapshot-file:s?,format:s?",
> > You changed from an underline to a hyphen as I asked, but the implementation
> > still expects an underline. I fixed that myself to avoid multiple submissions
> > because of a small thing (also fixed the command name in the subject).
> > 
> > The patch I merged follows for reference. Please, test your patches before
> > submitting next time.
> > 
> > 
> 
> Sorry that is no go, you just broke the hmp implementation - you cannot
> change the hmp behavior like that.

HMP uses positional arguments, so changing argument names makes no
difference. And, apart from some exceptions, it's not an stable interface,
anyway...
Jes Sorensen - July 12, 2011, 9:26 a.m.
On 07/11/11 22:35, Luiz Capitulino wrote:
>> Sorry that is no go, you just broke the hmp implementation - you cannot
>> > change the hmp behavior like that.
> HMP uses positional arguments, so changing argument names makes no
> difference. And, apart from some exceptions, it's not an stable interface,
> anyway...
> 

I guess you're right about the naming not affecting the hmp interface.
However hmp is far more usable to end users than qmp, so yes it does
matter not to change the interface at random.

Jes
Luiz Capitulino - July 12, 2011, 1:26 p.m.
On Tue, 12 Jul 2011 11:26:09 +0200
Jes Sorensen <Jes.Sorensen@redhat.com> wrote:

> On 07/11/11 22:35, Luiz Capitulino wrote:
> >> Sorry that is no go, you just broke the hmp implementation - you cannot
> >> > change the hmp behavior like that.
> > HMP uses positional arguments, so changing argument names makes no
> > difference. And, apart from some exceptions, it's not an stable interface,
> > anyway...
> > 
> 
> I guess you're right about the naming not affecting the hmp interface.
> However hmp is far more usable to end users than qmp, so yes it does
> matter not to change the interface at random.

Right, but we don't do it at random. Actually, it's not something that
happens often and we always consider the impact. However, hmp doesn't
have stability guarantees as qmp has.

In this specific case, no hmp user visible change has been made.

Patch

diff --git a/blockdev.c b/blockdev.c
index 7d579d6..1a96d3c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -572,7 +572,7 @@  void do_commit(Monitor *mon, const QDict *qdict)
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *device = qdict_get_str(qdict, "device");
-    const char *filename = qdict_get_try_str(qdict, "snapshot_file");
+    const char *filename = qdict_get_try_str(qdict, "snapshot-file");
     const char *format = qdict_get_try_str(qdict, "format");
     BlockDriverState *bs;
     BlockDriver *drv, *old_drv, *proto_drv;
@@ -581,7 +581,7 @@  int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
     char old_filename[1024];
 
     if (!filename) {
-        qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
+        qerror_report(QERR_MISSING_PARAMETER, "snapshot-file");
         ret = -1;
         goto out;
     }
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6ad8806..c857827 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -840,7 +840,7 @@  ETEXI
 
     {
         .name       = "snapshot_blkdev",
-        .args_type  = "device:B,snapshot_file:s?,format:s?",
+        .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"
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 92c5c3a..5d44edf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -694,6 +694,40 @@  Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-sync",
+        .args_type  = "device:B,snapshot-file:s?,format:s?",
+        .params     = "device [new-image-file] [format]",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_snapshot_blkdev,
+    },
+
+SQMP
+blockdev-snapshot-sync
+----------------------
+
+Synchronous snapshot of a block device. snapshot-file specifies the
+target of the new image. If the file exists, or if it is a device, the
+snapshot will be created in the existing file/device. If does not
+exist, a new file will be created. format specifies the format of the
+snapshot image, default is qcow2.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "snapshot-file": name of new image file (json-string)
+- "format": format of new image (json-string, optional)
+
+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",