Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

["Denis V. Lunev" <den@openvz.org>]

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.