qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pavel Hrdina
Subject: Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm
Date: Thu, 16 Aug 2012 12:16:23 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0

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<address@hidden>
---
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?)






reply via email to

[Prev in Thread] Current Thread [Next in Thread]