Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm

[Pavel Hrdina <phrdina@redhat.com>]

On 08/15/2012 09:49 PM, Eric Blake wrote:
> On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
> I'm focusing my review more on the public interface (since that's what
> affects libvirt), and therefore glanced through 1 through 12 but did not
> pay close attention to them.
>
>>   hmp-commands.hx  |  2 +-
>>   hmp.c            | 10 ++++++++++
>>   hmp.h            |  1 +
>>   qapi-schema.json | 19 +++++++++++++++++++
>>   qmp-commands.hx  | 29 +++++++++++++++++++++++++++++
>>   savevm.c         | 25 +++++++++----------------
>>   sysemu.h         |  1 -
>>   7 files changed, 69 insertions(+), 18 deletions(-)
>>
>> +
>> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *name = qdict_get_try_str(qdict, "name");
> In the cover letter, you said "and for QMP you have to always provide
> name parameter" - but this says 'name' is optional and can still be
> empty (id only).
I said in cover letter that the last two patches introduce this 
functionality.
>> +++ b/qapi-schema.json
>> @@ -2356,3 +2356,22 @@
>>   # Since: 1.2.0
>>   ##
>>   { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
>> +
>> +##
>> +# @vm-snapshot-save:
>> +#
>> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,
> 'tag' is a misnomer, since you list @name as the parameter name.
>
>> +# it is used as human readable identifier. If there is already a snapshot
>> +# with the same tag or ID, it is replaced.
>> +#
>> +# The VM is automatically stopped and resumed and saving a snapshot can take
>> +# a long time.
>> +#
>> +# @name: tag or id of new or existing snapshot
> Here, you document @name as required,
>
>> +#
>> +# Returns: Nothing on success
>> +#          If an error occurs, GenericError with error message
>> +#
>> +# Since: 1.2
> We missed 1.2 hard freeze.  This better be 1.3.
>
>> +##
>> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
> but here, you listed it as '*name' meaning optional.  I'm okay with
> leaving name optional, provided that it means that you end up allocating
> the next unique id.  But if that's the case, then returning nothing is
> wrong; this command needs to return { 'id':'int', '*name':'str' }, so
> that the user knows what snapshot just got allocated.
If you apply whole patch-series, then you have to for QMP always provide 
@name.
As I said above, this behaviour is introduced by last two patches.
>> +SQMP
>> +vm-snapshot-save
>> +------
>> +
>> +Create a snapshot of the whole virtual machine. If 'tag' is provided,
>> +it is used as human readable identifier. If there is already a snapshot
>> +with the same tag or ID, it is replaced.
>> +
>> +The VM is automatically stopped and resumed and saving a snapshot can take
>> +a long time.
> I don't like that this command can take a long time.  Just today on
> #virt IRC, someone was complaining that 'savevm' HMP cannot be canceled.
>   If we're going to create a new interface, it would be nicer to create a
> command that starts the save process and returns immediately, as well as
> commands to track progress, allow an early abort, and send an event on
> completion.  The HMP 'savevm' interface can issue multiple QMP commands
> under the hood to continue with it's blocking behavior, but as long as
> we are fixing things, I think the QMP interface should be more powerful.
Well, I could try to do it...with this I'll probably introduce new 
command vm-snapshot-save-cancel or vm-snaphsot-cancel as we have migrate 
and migrate-cancel.
>> +
>> +Arguments:
>> +
>> +- "name": tag or id of new or existing snapshot
>> +
>> +Example:
>> +
>> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
>> +<- { "return": {} }
> Again, you need to return the id of the just-created snapshot.
>
>> @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>>       }
>>   
>>       /* Delete old snapshots of the same name */
>> -    if (name && del_existing_snapshots(name, NULL) < 0) {
>> +    if (has_name && del_existing_snapshots(name, errp) < 0) {
> Here's hoping later in the series updates this to make saner decisions.
>   (Maybe I should peruse the entire series before commenting on
> individual patches?)
>