| Message ID | 20110711172456.5f61adbb@doriath |
|---|---|
| State | New |
| Headers | show |
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
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...
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
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.
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",