| Message ID | 1449240275-26196-3-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
On 12/04/2015 07:44 AM, Denis V. Lunev wrote: > 'name' attribute is made mandatory in distinction with HMP command. > > The patch also moves hmp_savevm implementation into hmp.c. This function > is just a simple wrapper now and does not have knowledge about > migration internals. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Juan Quintela <quintela@redhat.com> > CC: Amit Shah <amit.shah@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > hmp.c | 12 ++++++++++++ > migration/savevm.c | 13 +------------ > qapi-schema.json | 13 +++++++++++++ > qmp-commands.hx | 25 +++++++++++++++++++++++++ > 4 files changed, 51 insertions(+), 12 deletions(-) > > diff --git a/hmp.c b/hmp.c > index 2140605..c9c7100 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -32,6 +32,7 @@ > #include "ui/console.h" > #include "block/qapi.h" > #include "qemu-io.h" > +#include "sysemu/sysemu.h" What is this header needed for? > > #ifdef CONFIG_SPICE > #include <spice/enums.h> > @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict) > > qapi_free_RockerOfDpaGroupList(list); > } > + > +void hmp_savevm(Monitor *mon, const QDict *qdict) > +{ > + Error *local_err = NULL; I've been favoring 'err' rather than 'local_err', but that's not a show-stopper since we still have both spellings mixed in the tree. > + > + qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err); qdict_get_try_str() can return NULL;, but qmp_savevm() requires a non-NULL string. You'll need to invent a name in HMP if the user didn't give you one. > +++ b/migration/savevm.c > @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f) > return ret; > } > > -static void do_savevm(const char *name, Error **errp) > +void qmp_savevm(const char *name, Error **errp) > { This function still has some 'if (name)' conditionals; but according to the qapi contract, name should never be NULL. You need to hoist the name creation aspect to the caller (hmp_savevm), and in qmp_savevm you can then assert(name). > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; > @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp) > } > } > > -void hmp_savevm(Monitor *mon, const QDict *qdict) > -{ > - Error *local_err = NULL; > - > - do_savevm(qdict_get_try_str(qdict, "name"), &local_err); Hmm. You are doing code motion of code you just added in 1/5; wouldn't it be better if 1/5 stuck it in the right file to begin with? And my argument about needing the name logic in HMP (not QMP) means that maybe the split in 1/5 isn't ideal, after all. > +++ b/qapi-schema.json > @@ -3971,3 +3971,16 @@ > ## > { 'enum': 'ReplayMode', > 'data': [ 'none', 'record', 'play' ] } > + > +## > +# @savevm > +# > +# Save a VM snapshot. Without a name new snapshot is created", Stale sentence, since a name is required. > +# > +# @name: identifier of a snapshot to be created > +# > +# Returns: Nothing on success > +# > +# Since 2.6 > +## > +{ 'command': 'savevm', 'data': {'name': 'str'} } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 9d8b42f..f216c5e 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -4739,3 +4739,28 @@ Example: > {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840, > "pop-vlan": 1, "id": 251658240} > ]} > + > +EQMP > + > +SQMP > +savevm > +------------------ Line too long. Make it match the length of the line above. > + > +Save a VM snapshot. If no tag or id are provided, a new snapshot is created Stale sentence, since you are requiring a name. Why does QMP not allow specifying an id? Should it?
On 12/24/2015 12:40 AM, Eric Blake wrote: > On 12/04/2015 07:44 AM, Denis V. Lunev wrote: >> 'name' attribute is made mandatory in distinction with HMP command. >> >> The patch also moves hmp_savevm implementation into hmp.c. This function >> is just a simple wrapper now and does not have knowledge about >> migration internals. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Juan Quintela <quintela@redhat.com> >> CC: Amit Shah <amit.shah@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> --- >> hmp.c | 12 ++++++++++++ >> migration/savevm.c | 13 +------------ >> qapi-schema.json | 13 +++++++++++++ >> qmp-commands.hx | 25 +++++++++++++++++++++++++ >> 4 files changed, 51 insertions(+), 12 deletions(-) >> >> diff --git a/hmp.c b/hmp.c >> index 2140605..c9c7100 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -32,6 +32,7 @@ >> #include "ui/console.h" >> #include "block/qapi.h" >> #include "qemu-io.h" >> +#include "sysemu/sysemu.h" > What is this header needed for? > >> >> #ifdef CONFIG_SPICE >> #include <spice/enums.h> >> @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict) >> >> qapi_free_RockerOfDpaGroupList(list); >> } >> + >> +void hmp_savevm(Monitor *mon, const QDict *qdict) >> +{ >> + Error *local_err = NULL; > I've been favoring 'err' rather than 'local_err', but that's not a > show-stopper since we still have both spellings mixed in the tree. > >> + >> + qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err); > qdict_get_try_str() can return NULL;, but qmp_savevm() requires a > non-NULL string. You'll need to invent a name in HMP if the user didn't > give you one. > >> +++ b/migration/savevm.c >> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f) >> return ret; >> } >> >> -static void do_savevm(const char *name, Error **errp) >> +void qmp_savevm(const char *name, Error **errp) >> { > This function still has some 'if (name)' conditionals; but according to > the qapi contract, name should never be NULL. You need to hoist the name > creation aspect to the caller (hmp_savevm), and in qmp_savevm you can > then assert(name). > >> BlockDriverState *bs, *bs1; >> QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; >> @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp) >> } >> } >> >> -void hmp_savevm(Monitor *mon, const QDict *qdict) >> -{ >> - Error *local_err = NULL; >> - >> - do_savevm(qdict_get_try_str(qdict, "name"), &local_err); > Hmm. You are doing code motion of code you just added in 1/5; wouldn't > it be better if 1/5 stuck it in the right file to begin with? And my > argument about needing the name logic in HMP (not QMP) means that maybe > the split in 1/5 isn't ideal, after all. I'll need a header change in 1/5 and subsequent removal of the item here This was the motto of doing thing this way. >> +++ b/qapi-schema.json >> @@ -3971,3 +3971,16 @@ >> ## >> { 'enum': 'ReplayMode', >> 'data': [ 'none', 'record', 'play' ] } >> + >> +## >> +# @savevm >> +# >> +# Save a VM snapshot. Without a name new snapshot is created", > Stale sentence, since a name is required. > >> +# >> +# @name: identifier of a snapshot to be created >> +# >> +# Returns: Nothing on success >> +# >> +# Since 2.6 >> +## >> +{ 'command': 'savevm', 'data': {'name': 'str'} } >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 9d8b42f..f216c5e 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -4739,3 +4739,28 @@ Example: >> {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840, >> "pop-vlan": 1, "id": 251658240} >> ]} >> + >> +EQMP >> + >> +SQMP >> +savevm >> +------------------ > Line too long. Make it match the length of the line above. > >> + >> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created > Stale sentence, since you are requiring a name. > > Why does QMP not allow specifying an id? Should it? > ok. this is a something to think on.
On 12/24/2015 12:40 AM, Eric Blake wrote: > On 12/04/2015 07:44 AM, Denis V. Lunev wrote: >> 'name' attribute is made mandatory in distinction with HMP command. >> >> The patch also moves hmp_savevm implementation into hmp.c. This function >> is just a simple wrapper now and does not have knowledge about >> migration internals. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Juan Quintela <quintela@redhat.com> >> CC: Amit Shah <amit.shah@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> --- >> hmp.c | 12 ++++++++++++ >> migration/savevm.c | 13 +------------ >> qapi-schema.json | 13 +++++++++++++ >> qmp-commands.hx | 25 +++++++++++++++++++++++++ >> 4 files changed, 51 insertions(+), 12 deletions(-) >> >> diff --git a/hmp.c b/hmp.c >> index 2140605..c9c7100 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -32,6 +32,7 @@ >> #include "ui/console.h" >> #include "block/qapi.h" >> #include "qemu-io.h" >> +#include "sysemu/sysemu.h" > What is this header needed for? > it contains prototype of hmp_savevm. The following warning appears without it: irbis ~/src/qemu $ make -j8 CC hmp.o hmp.c:2390:6: error: no previous prototype for ‘hmp_savevm’ [-Werror=missing-prototypes] void hmp_savevm(Monitor *mon, const QDict *qdict) ^ cc1: all warnings being treated as errors /home/den/src/qemu/rules.mak:57: recipe for target 'hmp.o' failed make: *** [hmp.o] Error 1 irbis ~/src/qemu $
diff --git a/hmp.c b/hmp.c index 2140605..c9c7100 100644 --- a/hmp.c +++ b/hmp.c @@ -32,6 +32,7 @@ #include "ui/console.h" #include "block/qapi.h" #include "qemu-io.h" +#include "sysemu/sysemu.h" #ifdef CONFIG_SPICE #include <spice/enums.h> @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict) qapi_free_RockerOfDpaGroupList(list); } + +void hmp_savevm(Monitor *mon, const QDict *qdict) +{ + Error *local_err = NULL; + + qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err); + + if (local_err != NULL) { + error_report_err(local_err); + } +} diff --git a/migration/savevm.c b/migration/savevm.c index b7278ac..65e0081 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f) return ret; } -static void do_savevm(const char *name, Error **errp) +void qmp_savevm(const char *name, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp) } } -void hmp_savevm(Monitor *mon, const QDict *qdict) -{ - Error *local_err = NULL; - - do_savevm(qdict_get_try_str(qdict, "name"), &local_err); - - if (local_err != NULL) { - error_report_err(local_err); - } -} - void qmp_xen_save_devices_state(const char *filename, Error **errp) { QEMUFile *f; diff --git a/qapi-schema.json b/qapi-schema.json index 8b1a423..0114132 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3971,3 +3971,16 @@ ## { 'enum': 'ReplayMode', 'data': [ 'none', 'record', 'play' ] } + +## +# @savevm +# +# Save a VM snapshot. Without a name new snapshot is created", +# +# @name: identifier of a snapshot to be created +# +# Returns: Nothing on success +# +# Since 2.6 +## +{ 'command': 'savevm', 'data': {'name': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 9d8b42f..f216c5e 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -4739,3 +4739,28 @@ Example: {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840, "pop-vlan": 1, "id": 251658240} ]} + +EQMP + +SQMP +savevm +------------------ + +Save a VM snapshot. If no tag or id are provided, a new snapshot is created + +Arguments: + +- "name": snapshot name + +Example: + +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } } +<- { "return": {} } + +EQMP + + { + .name = "savevm", + .args_type = "name:s?", + .mhandler.cmd_new = qmp_marshal_savevm, + },
'name' attribute is made mandatory in distinction with HMP command. The patch also moves hmp_savevm implementation into hmp.c. This function is just a simple wrapper now and does not have knowledge about migration internals. Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Juan Quintela <quintela@redhat.com> CC: Amit Shah <amit.shah@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Eric Blake <eblake@redhat.com> --- hmp.c | 12 ++++++++++++ migration/savevm.c | 13 +------------ qapi-schema.json | 13 +++++++++++++ qmp-commands.hx | 25 +++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 12 deletions(-)