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

[Juan Quintela]

Markus Armbruster <address@hidden> wrote:
> "Denis V. Lunev" <address@hidden> writes:
>
>> Signed-off-by: Denis V. Lunev <address@hidden>
>> CC: Juan Quintela <address@hidden>
>> CC: Amit Shah <address@hidden>
>> CC: Markus Armbruster <address@hidden>
>> CC: Eric Blake <address@hidden>
>> ---
>>  migration/savevm.c |  5 +++++
>>  qapi-schema.json   | 13 +++++++++++++
>>  qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>  3 files changed, 43 insertions(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f83ffd0..565b10a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>      }
>>  }
>>  
>> +void qmp_savevm(bool has_name, const char *name, Error **errp)
>> +{
>> +    do_savevm(has_name ? name : NULL, errp);
>> +}
>> +
>
> Please name do_savevm() qmp_savevm() and drop this wrapper.
>
> We're working on omitting has_FOO for pointer-valued FOO.

Agreed.

>
>>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>>  {
>>      QEMUFile *f;
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b65905f..8cc8b44 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3962,3 +3962,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
>
> Missing #optional tag.
>
> What happens when @name is missing?

Why are we allowing this?  QMP is going to be called by libvirt or
similar, they can choose a name, thanks very much.

> What happens when @name names an existing snapshot?

We remove them.

    /* Delete old snapshots of the same name */
    if (name && del_existing_snapshots(mon, name) < 0) {
        goto the_end;
    }

I think we should give one error.  Let the hmp code remove them.
I would preffer to change the interface to "require" a flag if we want
it to be removed.  But requiring a flag is the equivalent of  requiring

qmp_deletevm
qmp_savevm

or whatever they are called.


> Do we want @name to be optional in QMP?  I dimly remember ambiguity
> problems between names and IDs.  Perhaps it'll become clear later in the
> series.

Again, I think that requiring a name is the best thing to do.  Libvirt,
openstack and friends are good at choosen names, qemu is not.

> The QMP interface needs to make sense on its own, even if that means
> deviating from HMP.  Juan, Amit, do you have opinions on the proper QMP
> interface for internal snapshots?

I agree that the "requirement" of a name is a good idea.  Not deleting
the existing snapshots looks like a good idea also.  It is not difficult
to romeve them previously.

Thanks, Juan.