From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43390) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz12J-0004UC-ER for qemu-devel@nongnu.org; Wed, 18 Nov 2015 06:36:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zz12G-0003my-Ph for qemu-devel@nongnu.org; Wed, 18 Nov 2015 06:36:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51237) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zz12G-0003ms-I0 for qemu-devel@nongnu.org; Wed, 18 Nov 2015 06:36:24 -0500 From: Juan Quintela In-Reply-To: <87ziyd0wdm.fsf@blackfin.pond.sub.org> (Markus Armbruster's message of "Tue, 17 Nov 2015 11:10:29 +0100") References: <1447687950-29350-1-git-send-email-den@openvz.org> <1447687950-29350-3-git-send-email-den@openvz.org> <87ziyd0wdm.fsf@blackfin.pond.sub.org> Date: Wed, 18 Nov 2015 12:36:08 +0100 Message-ID: <87mvubmtef.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Amit Shah , "Denis V. Lunev" , qemu-devel@nongnu.org Markus Armbruster wrote: > "Denis V. Lunev" writes: > >> Signed-off-by: Denis V. Lunev >> CC: Juan Quintela >> CC: Amit Shah >> CC: Markus Armbruster >> CC: Eric Blake >> --- >> 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.