Markus Armbruster <arm...@redhat.com> wrote:
> "Denis V. Lunev" <d...@openvz.org> writes:
>
>> Signed-off-by: Denis V. Lunev <d...@openvz.org>
>> CC: Juan Quintela <quint...@redhat.com>
>> CC: Amit Shah <amit.s...@redhat.com>
>> CC: Markus Armbruster <arm...@redhat.com>
>> CC: Eric Blake <ebl...@redhat.com>
>> ---
>> 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.
