[Qemu-devel] [PATCH v5 0/8] QMP wrappers for VM snapshot operations

[Qemu-devel] [PATCH v5 0/8] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

EFI based VM with pflash storage for NVRAM could not be snapshoted as
libvirt configures storage as 'raw' and writable. OK, this is a libvirt
problem.

Another problem is that libvirt can not detect this failure at all
as it uses HMP for this operation. This create snapshot/delete snapshot
sequence passes silently.

The patchset adds QMP wrappers for the purpose and specifically changes
behavior of the savevm/loadvm:
- savevm now allows optional argument to allow to specify block device
  to save VM state to
- loadvm locate block device which really contains VM state

Eric, Marcus, I don't want to merge addition of the 'device' parameter
into patch 5. The present patch split looks better to me.

Signed-off-by: "Denis V. Lunev" <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>

Changes from v4:
- patches allows to save VM state to arbitrary BDS added

Changes from v3:
- wrong patch 1 is replaced

Changes from v2:
- patches 1/2 are resplit to move processing HMP specific handling
  of snapshot name generation to exclusive HMP code
- removed all '.' at the end of error_setg strings
- fixed too long lines with '-' in qmp-commands.hx
- error_setg_errno errno passing is fixed (-ret)
- fixed logical error in hmp_loadvm (vm_start on error)
- NOT switched to error_prepend code (it is not yet merged). Can we do this
  later? This will make my life easear merging code to our downstream.

Changes from v1:
- cosmetic fixes suggested by Markus. I pray I have added all of them
- patch 5 is rewritten completely. Original one was deadbeaf

Denis V. Lunev (8):
  migration: split hmp_savevm to migrate_savevm and hmp_savevm wrapper
  qmp: create qmp_savevm command
  qmp: create qmp_delvm command
  migration: improve error reporting for load_vmstate
  qmp: create QMP implementation of loadvm command
  migration, block: better select BDS for VM state loading
  migration, qmp: add optional argument to specify BDS to save VM state
    to
  block: allow to skip block driver in selection of one for VM state
    saving

 block.c                       |   7 +++
 block/snapshot.c              | 111 ++++++++++++++++++++++++++++++++++++++-
 hmp.c                         |  36 +++++++++++++
 include/block/block_int.h     |   3 ++
 include/block/snapshot.h      |   3 +-
 include/migration/migration.h |   1 +
 include/sysemu/sysemu.h       |   2 +-
 migration/savevm.c            | 118 ++++++++++++++++--------------------------
 monitor.c                     |   9 ++--
 qapi-schema.json              |  40 ++++++++++++++
 qmp-commands.hx               |  73 ++++++++++++++++++++++++++
 vl.c                          |   4 +-
 12 files changed, 325 insertions(+), 82 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/8] migration: split hmp_savevm to migrate_savevm and hmp_savevm wrapper

[Denis V. Lunev]

This would be useful in the next step when QMP version of this call will
be introduced. The patch also moves snapshot name generation to the
hmp specific code as QMP version of this code will require the name
on the protocol level.

Addition of migration_savevm to migration/migration.h is temporary. It
will be removed in the next patch with QMP level change.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
---
 hmp.c                         | 26 ++++++++++++++++++++++++
 include/migration/migration.h |  3 +++
 migration/savevm.c            | 46 +++++++++++++++++--------------------------
 3 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/hmp.c b/hmp.c
index c2b2c16..e7bab75 100644
--- a/hmp.c
+++ b/hmp.c
@@ -18,6 +18,7 @@
 #include "net/eth.h"
 #include "sysemu/char.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
 #include "qemu/option.h"
 #include "qemu/timer.h"
 #include "qmp-commands.h"
@@ -32,6 +33,7 @@
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
+#include "migration/migration.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -2386,3 +2388,27 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+    const char *name = qdict_get_try_str(qdict, "name");
+    char name_buf[64];
+
+    if (name == NULL) {
+        qemu_timeval tv;
+        struct tm tm;
+
+        /* cast below needed for OpenBSD where tv_sec is still 'long' */
+        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        strftime(name_buf, sizeof(name_buf), "vm-%Y%m%d%H%M%S", &tm);
+
+        name = name_buf;
+    }
+
+    migrate_savevm(name, &local_err);
+
+    if (local_err != NULL) {
+        error_report_err(local_err);
+    }
+}
diff --git a/include/migration/migration.h b/include/migration/migration.h
index d9494b8..73c8bb1 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -277,6 +277,8 @@ int migrate_compress_threads(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 
+void migrate_savevm(const char *name, Error **errp);
+
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_message(MigrationIncomingState *mis,
                              enum mig_rp_message_type message_type,
@@ -321,4 +323,5 @@ int ram_save_queue_pages(MigrationState *ms, const char *rbname,
 PostcopyState postcopy_state_get(void);
 /* Set the state and return the old state */
 PostcopyState postcopy_state_set(PostcopyState new_state);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..308302a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void hmp_savevm(Monitor *mon, const QDict *qdict)
+void migrate_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -1914,29 +1914,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     int saved_vm_running;
     uint64_t vm_state_size;
     qemu_timeval tv;
-    struct tm tm;
-    const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;
     AioContext *aio_context;
 
     if (!bdrv_all_can_snapshot(&bs)) {
-        monitor_printf(mon, "Device '%s' is writable but does not "
-                       "support snapshots.\n", bdrv_get_device_name(bs));
+        error_setg(errp,
+                   "Device '%s' is writable but does not support snapshots",
+                   bdrv_get_device_name(bs));
         return;
     }
 
     /* Delete old snapshots of the same name */
-    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+    if (bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
+                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
         error_free(local_err);
         return;
     }
 
     bs = bdrv_all_find_vmstate_bs();
     if (bs == NULL) {
-        monitor_printf(mon, "No block device can accept snapshots\n");
+        error_setg(errp, "No block device can accept snapshots");
         return;
     }
     aio_context = bdrv_get_aio_context(bs);
@@ -1945,7 +1943,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 
     ret = global_state_store();
     if (ret) {
-        monitor_printf(mon, "Error saving global state\n");
+        error_setg(errp, "Error saving global state");
         return;
     }
     vm_stop(RUN_STATE_SAVE_VM);
@@ -1960,39 +1958,31 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     sn->date_nsec = tv.tv_usec * 1000;
     sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
-    if (name) {
-        ret = bdrv_snapshot_find(bs, old_sn, name);
-        if (ret >= 0) {
-            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
-            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
-        } else {
-            pstrcpy(sn->name, sizeof(sn->name), name);
-        }
+    ret = bdrv_snapshot_find(bs, old_sn, name);
+    if (ret >= 0) {
+        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
+        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
     } else {
-        /* cast below needed for OpenBSD where tv_sec is still 'long' */
-        localtime_r((const time_t *)&tv.tv_sec, &tm);
-        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
+        pstrcpy(sn->name, sizeof(sn->name), name);
     }
 
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-        monitor_printf(mon, "Could not open VM state file\n");
+        error_setg(errp, "Could not open VM state file");
         goto the_end;
     }
-    ret = qemu_savevm_state(f, &local_err);
+    ret = qemu_savevm_state(f, errp);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
-        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
-        error_free(local_err);
         goto the_end;
     }
 
     ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
     if (ret < 0) {
-        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                       bdrv_get_device_name(bs));
+        error_setg(errp, "Error while creating snapshot on '%s'",
+                   bdrv_get_device_name(bs));
     }
 
  the_end:
-- 
2.5.0

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

[Denis V. Lunev]

'name' attribute is made mandatory in distinction with HMP command.

The patch also moves hmp_savevm implementation into hmp.c. This function
is just a simple wrapper now and does not have knowledge about
migration internals.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hmp.c                         |  3 +--
 include/migration/migration.h |  2 --
 migration/savevm.c            |  2 +-
 qapi-schema.json              | 13 +++++++++++++
 qmp-commands.hx               | 25 +++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/hmp.c b/hmp.c
index e7bab75..8d6eda6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -33,7 +33,6 @@
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
-#include "migration/migration.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -2406,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         name = name_buf;
     }
 
-    migrate_savevm(name, &local_err);
+    qmp_savevm(name, &local_err);
 
     if (local_err != NULL) {
         error_report_err(local_err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 73c8bb1..58c04a9 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -277,8 +277,6 @@ int migrate_compress_threads(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 
-void migrate_savevm(const char *name, Error **errp);
-
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_message(MigrationIncomingState *mis,
                              enum mig_rp_message_type message_type,
diff --git a/migration/savevm.c b/migration/savevm.c
index 308302a..0dbb15f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void migrate_savevm(const char *name, Error **errp)
+void qmp_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
diff --git a/qapi-schema.json b/qapi-schema.json
index 2e31733..09d1a1a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4054,3 +4054,16 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @savevm
+#
+# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
+#
+# @name: identifier of a snapshot to be created
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'savevm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db072a6..b7851e1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4795,3 +4795,28 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+SQMP
+savevm
+------
+
+Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "savevm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_savevm,
+    },
-- 
2.5.0

[Qemu-devel] [PATCH 3/8] qmp: create qmp_delvm command

[Denis V. Lunev]

The patch also moves hmp_delvm implementation into hmp.c. This function
is just a simple wrapper now and does not have knowledge about
migration internals.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hmp.c              | 11 +++++++++++
 migration/savevm.c | 16 +++++++---------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 23 +++++++++++++++++++++++
 4 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/hmp.c b/hmp.c
index 8d6eda6..f65bbe7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2411,3 +2411,14 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         error_report_err(local_err);
     }
 }
+
+void hmp_delvm(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    qmp_delvm(qdict_get_str(qdict, "name"), &local_err);
+
+    if (local_err != NULL) {
+        error_report_err(local_err);
+    }
+}
diff --git a/migration/savevm.c b/migration/savevm.c
index 0dbb15f..1262b57 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2092,17 +2092,15 @@ int load_vmstate(const char *name)
     return 0;
 }
 
-void hmp_delvm(Monitor *mon, const QDict *qdict)
+void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs;
-    Error *err;
-    const char *name = qdict_get_str(qdict, "name");
-
-    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs), error_get_pretty(err));
-        error_free(err);
+    Error *local_err = NULL;
+
+    if (bdrv_all_delete_snapshot(name, &bs, &local_err) < 0) {
+        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
+                   bdrv_get_device_name(bs), error_get_pretty(local_err));
+        error_free(local_err);
     }
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 09d1a1a..94a2764 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4067,3 +4067,16 @@
 # Since 2.6
 ##
 { 'command': 'savevm', 'data': {'name': 'str'} }
+
+##
+# @delvm
+#
+# Delete a VM snapshot
+#
+# @name: identifier of a snapshot to be deleted
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'delvm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b7851e1..5e6f573 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4820,3 +4820,26 @@ EQMP
         .args_type  = "name:s",
         .mhandler.cmd_new = qmp_marshal_savevm,
     },
+
+SQMP
+delvm
+-----
+
+Delete a VM snapshot
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "delvm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "delvm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_delvm,
+    },
-- 
2.5.0

[Qemu-devel] [PATCH 4/8] migration: improve error reporting for load_vmstate

[Denis V. Lunev]

The patch adds Error ** parameter to load_vmstate call and fills error
inside. The caller after that properly reports error either through
monitor or via local stderr facility during VM start.

This helper will be useful too for qmp_loadvm implementation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 include/sysemu/sysemu.h |  2 +-
 migration/savevm.c      | 28 ++++++++++++++++------------
 monitor.c               |  5 ++++-
 vl.c                    |  4 +++-
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 3bb8897..d9ccf45 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -78,7 +78,7 @@ void qemu_remove_exit_notifier(Notifier *notify);
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 
 void hmp_savevm(Monitor *mon, const QDict *qdict);
-int load_vmstate(const char *name);
+int load_vmstate(const char *name, Error **errp);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 1262b57..3de917e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2019,7 +2019,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
     }
 }
 
-int load_vmstate(const char *name)
+int load_vmstate(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs_vm_state;
     QEMUSnapshotInfo sn;
@@ -2028,20 +2028,22 @@ int load_vmstate(const char *name)
     AioContext *aio_context;
 
     if (!bdrv_all_can_snapshot(&bs)) {
-        error_report("Device '%s' is writable but does not support snapshots.",
-                     bdrv_get_device_name(bs));
+        error_setg(errp,
+                   "Device '%s' is writable but does not support snapshots",
+                   bdrv_get_device_name(bs));
         return -ENOTSUP;
     }
     ret = bdrv_all_find_snapshot(name, &bs);
     if (ret < 0) {
-        error_report("Device '%s' does not have the requested snapshot '%s'",
-                     bdrv_get_device_name(bs), name);
+        error_setg(errp,
+                   "Device '%s' does not have the requested snapshot '%s'",
+                   bdrv_get_device_name(bs), name);
         return ret;
     }
 
     bs_vm_state = bdrv_all_find_vmstate_bs();
     if (!bs_vm_state) {
-        error_report("No block device supports snapshots");
+        error_setg(errp, "No block device supports snapshots");
         return -ENOTSUP;
     }
     aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2051,10 +2053,11 @@ int load_vmstate(const char *name)
     ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
     aio_context_release(aio_context);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Snapshot '%s' not found", name);
         return ret;
     } else if (sn.vm_state_size == 0) {
-        error_report("This is a disk-only snapshot. Revert to it offline "
-            "using qemu-img.");
+        error_setg(errp, "Snapshot '%s' is a disk-only snapshot and must be "
+                   "reverted offline using qemu-img", name);
         return -EINVAL;
     }
 
@@ -2063,15 +2066,16 @@ int load_vmstate(const char *name)
 
     ret = bdrv_all_goto_snapshot(name, &bs);
     if (ret < 0) {
-        error_report("Error %d while activating snapshot '%s' on '%s'",
-                     ret, name, bdrv_get_device_name(bs));
+        error_setg_errno(errp, -ret,
+                         "Error while activating snapshot '%s' on '%s'",
+                         name, bdrv_get_device_name(bs));
         return ret;
     }
 
     /* restore the VM state */
     f = qemu_fopen_bdrv(bs_vm_state, 0);
     if (!f) {
-        error_report("Could not open VM state file");
+        error_setg(errp, "Could not open VM state file");
         return -EINVAL;
     }
 
@@ -2085,7 +2089,7 @@ int load_vmstate(const char *name)
 
     migration_incoming_state_destroy();
     if (ret < 0) {
-        error_report("Error %d while loading VM state", ret);
+        error_setg_errno(errp, -ret, "Error while loading VM state");
         return ret;
     }
 
diff --git a/monitor.c b/monitor.c
index e7e7ae2..2ddf6c1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1739,10 +1739,13 @@ static void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
     int saved_vm_running  = runstate_is_running();
     const char *name = qdict_get_str(qdict, "name");
+    Error *local_err = NULL;
 
     vm_stop(RUN_STATE_RESTORE_VM);
 
-    if (load_vmstate(name) == 0 && saved_vm_running) {
+    if (load_vmstate(name, &local_err) < 0) {
+        error_report_err(local_err);
+    } else if (saved_vm_running) {
         vm_start();
     }
 }
diff --git a/vl.c b/vl.c
index 5aaea77..0172e42 100644
--- a/vl.c
+++ b/vl.c
@@ -4648,8 +4648,10 @@ int main(int argc, char **argv, char **envp)
     qemu_system_reset(VMRESET_SILENT);
     register_global_state();
     if (loadvm) {
-        if (load_vmstate(loadvm) < 0) {
+        Error *local_err = NULL;
+        if (load_vmstate(loadvm, &local_err) < 0) {
             autostart = 0;
+            error_report_err(local_err);
         }
     }
 
-- 
2.5.0

[Qemu-devel] [PATCH 5/8] qmp: create QMP implementation of loadvm command

[Denis V. Lunev]

Unfortunately load_vmstate has a return code (int) and this code is checked
in the other places. Thus we could not just rename it to qmp_loadvm as
returns void.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 migration/savevm.c | 10 ++++++++++
 monitor.c          |  8 ++------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 23 +++++++++++++++++++++++
 4 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 3de917e..6b34017 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2096,6 +2096,16 @@ int load_vmstate(const char *name, Error **errp)
     return 0;
 }
 
+void qmp_loadvm(const char *name, Error **errp)
+{
+    int saved_vm_running  = runstate_is_running();
+    vm_stop(RUN_STATE_RESTORE_VM);
+
+    if (load_vmstate(name, errp) == 0 && saved_vm_running) {
+        vm_start();
+    }
+}
+
 void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs;
diff --git a/monitor.c b/monitor.c
index 2ddf6c1..c1e998c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1737,16 +1737,12 @@ void qmp_closefd(const char *fdname, Error **errp)
 
 static void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
-    int saved_vm_running  = runstate_is_running();
     const char *name = qdict_get_str(qdict, "name");
     Error *local_err = NULL;
 
-    vm_stop(RUN_STATE_RESTORE_VM);
-
-    if (load_vmstate(name, &local_err) < 0) {
+    qmp_loadvm(name, &local_err);
+    if (local_err != NULL) {
         error_report_err(local_err);
-    } else if (saved_vm_running) {
-        vm_start();
     }
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 94a2764..7d48948 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4080,3 +4080,16 @@
 # Since 2.6
 ##
 { 'command': 'delvm', 'data': {'name': 'str'} }
+
+##
+# @loadvm
+#
+# Load a VM snapshot
+#
+# @name: identifier of a snapshot to be loaded
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'loadvm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5e6f573..9cd1bfe 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4843,3 +4843,26 @@ EQMP
         .args_type  = "name:s",
         .mhandler.cmd_new = qmp_marshal_delvm,
     },
+
+SQMP
+loadvm
+------
+
+Load a VM snapshot
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "loadvm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "loadvm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_loadvm,
+    },
-- 
2.5.0

[Qemu-devel] [PATCH 6/8] migration, block: better select BDS for VM state loading

[Denis V. Lunev]

This patch does 2 things:
- it merges all snapshot validity checks for load_vmstate into one function
- it now selects BDS to load VM state by availability of the state in the
  snapshot

This commit is preparatory to allow to select BDS to save snapshot for QMP.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 block/snapshot.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 33 ++------------------------
 3 files changed, 64 insertions(+), 31 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 2d86b88..77affbc 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -489,3 +489,64 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
     }
     return bs;
 }
+
+
+static bool validate_bs(BlockDriverState *bs, BlockDriverState **vmstate_bs,
+                        const char *name, Error **errp)
+{
+    QEMUSnapshotInfo sn;
+
+    if (!bdrv_can_snapshot(bs)) {
+        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+            return true;
+        }
+        error_setg(errp,
+                   "Device '%s' is writable but does not support snapshots",
+                   bdrv_get_device_name(bs));
+        return false;
+    }
+
+    if (bdrv_snapshot_find(bs, &sn, name) < 0) {
+        error_setg(errp,
+                   "Device '%s' does not have the requested snapshot '%s'",
+                   bdrv_get_device_name(bs), name);
+        return false;
+    }
+
+    if (sn.vm_state_size == 0) {
+        return true;
+    }
+    if (*vmstate_bs != NULL) {
+        error_setg(errp, "Devices '%s' and '%s' both has vmstate with the "
+                   "requested snapshot '%s'", bdrv_get_device_name(bs),
+                   bdrv_get_device_name(*vmstate_bs), name);
+        return false;
+    }
+    *vmstate_bs = bs;
+
+    return true;
+}
+
+BlockDriverState *bdrv_all_validate_snapshot(const char *name, Error **errp)
+{
+    BlockDriverState *bs = NULL;
+    BlockDriverState *vmstate_bs = NULL;
+
+    while ((bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+        bool ok;
+
+        aio_context_acquire(ctx);
+        ok = validate_bs(bs, &vmstate_bs, name, errp);
+        aio_context_release(ctx);
+
+        if (!ok) {
+            return NULL;
+        }
+    }
+
+    if (vmstate_bs == NULL) {
+        error_setg(errp, "VM state for the snapshot '%s' not found", name);
+    }
+    return vmstate_bs;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index c6910da6..a8506e9 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -92,5 +92,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              BlockDriverState **first_bad_bs);
 
 BlockDriverState *bdrv_all_find_vmstate_bs(void);
+BlockDriverState *bdrv_all_validate_snapshot(const char *name, Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 6b34017..fed8664 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2022,45 +2022,16 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
 int load_vmstate(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs_vm_state;
-    QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
     AioContext *aio_context;
 
-    if (!bdrv_all_can_snapshot(&bs)) {
-        error_setg(errp,
-                   "Device '%s' is writable but does not support snapshots",
-                   bdrv_get_device_name(bs));
-        return -ENOTSUP;
-    }
-    ret = bdrv_all_find_snapshot(name, &bs);
-    if (ret < 0) {
-        error_setg(errp,
-                   "Device '%s' does not have the requested snapshot '%s'",
-                   bdrv_get_device_name(bs), name);
-        return ret;
-    }
-
-    bs_vm_state = bdrv_all_find_vmstate_bs();
-    if (!bs_vm_state) {
-        error_setg(errp, "No block device supports snapshots");
+    bs_vm_state = bdrv_all_validate_snapshot(name, errp);
+    if (bs == NULL) {
         return -ENOTSUP;
     }
     aio_context = bdrv_get_aio_context(bs_vm_state);
 
-    /* Don't even try to load empty VM states */
-    aio_context_acquire(aio_context);
-    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
-    aio_context_release(aio_context);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "Snapshot '%s' not found", name);
-        return ret;
-    } else if (sn.vm_state_size == 0) {
-        error_setg(errp, "Snapshot '%s' is a disk-only snapshot and must be "
-                   "reverted offline using qemu-img", name);
-        return -EINVAL;
-    }
-
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all();
 
-- 
2.5.0

[Qemu-devel] [PATCH 7/8] migration, qmp: add optional argument to specify BDS to save VM state to

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 block/snapshot.c         | 40 ++++++++++++++++++++++++++++++++++++++--
 hmp.c                    |  2 +-
 include/block/snapshot.h |  2 +-
 migration/savevm.c       | 11 ++++++-----
 qapi-schema.json         |  3 ++-
 qmp-commands.hx          |  6 ++++--
 6 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 77affbc..237a015 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,6 +25,7 @@
 #include "block/snapshot.h"
 #include "block/block_int.h"
 #include "qapi/qmp/qerror.h"
+#include "sysemu/block-backend.h"
 
 QemuOptsList internal_snapshot_opts = {
     .name = "snapshot",
@@ -475,18 +476,53 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
     return err;
 }
 
-BlockDriverState *bdrv_all_find_vmstate_bs(void)
+
+BlockDriverState *bdrv_all_find_vmstate_bs(const char *device, Error **errp)
 {
     bool not_found = true;
     BlockDriverState *bs = NULL;
+    AioContext *ctx;
+
+    if (device != NULL) {
+        BlockBackend *blk = blk_by_name(device);
+        bool can_snapshot;
+
+        if (blk == NULL) {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", device);
+            return NULL;
+        }
+        bs = blk_bs(blk);
+        if (bs == NULL) {
+            error_setg(errp, "Device '%s' does not have block driver attached",
+                       device);
+            return NULL;
+        }
+
+        ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        can_snapshot =  bdrv_can_snapshot(bs);
+        aio_context_release(ctx);
+
+        if (!can_snapshot) {
+            error_setg(errp, "Device '%s' does not support snapshots", device);
+            return NULL;
+        }
+        return bs;
+    }
 
     while (not_found && (bs = bdrv_next(bs))) {
-        AioContext *ctx = bdrv_get_aio_context(bs);
+        ctx = bdrv_get_aio_context(bs);
 
         aio_context_acquire(ctx);
         not_found = !bdrv_can_snapshot(bs);
         aio_context_release(ctx);
     }
+
+    if (bs == NULL) {
+        error_setg(errp, "No block device can accept snapshots");
+    }
     return bs;
 }
 
diff --git a/hmp.c b/hmp.c
index f65bbe7..2c6a9a0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2405,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         name = name_buf;
     }
 
-    qmp_savevm(name, &local_err);
+    qmp_savevm(name, false, NULL, &local_err);
 
     if (local_err != NULL) {
         error_report_err(local_err);
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index a8506e9..a2caca9 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -91,7 +91,7 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              uint64_t vm_state_size,
                              BlockDriverState **first_bad_bs);
 
-BlockDriverState *bdrv_all_find_vmstate_bs(void);
+BlockDriverState *bdrv_all_find_vmstate_bs(const char *device, Error **errp);
 BlockDriverState *bdrv_all_validate_snapshot(const char *name, Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index fed8664..16ff92a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,8 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void qmp_savevm(const char *name, Error **errp)
+void qmp_savevm(const char *name, bool has_device, const char *device,
+                Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -1932,9 +1933,8 @@ void qmp_savevm(const char *name, Error **errp)
         return;
     }
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(has_device ? device : NULL, errp);
     if (bs == NULL) {
-        error_setg(errp, "No block device can accept snapshots");
         return;
     }
     aio_context = bdrv_get_aio_context(bs);
@@ -2097,10 +2097,11 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     int total;
     int *available_snapshots;
     AioContext *aio_context;
+    Error *local_err = NULL;
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(NULL, &local_err);
     if (!bs) {
-        monitor_printf(mon, "No available block device supports snapshots\n");
+        error_report_err(local_err);
         return;
     }
     aio_context = bdrv_get_aio_context(bs);
diff --git a/qapi-schema.json b/qapi-schema.json
index 7d48948..cd6d97f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4061,12 +4061,13 @@
 # Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
 #
 # @name: identifier of a snapshot to be created
+# @device: #optional device name to save VM state to
 #
 # Returns: Nothing on success
 #
 # Since 2.6
 ##
-{ 'command': 'savevm', 'data': {'name': 'str'} }
+{ 'command': 'savevm', 'data': {'name': 'str', '*device': 'str'} }
 
 ##
 # @delvm
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9cd1bfe..9133f2c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4807,17 +4807,19 @@ Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
 Arguments:
 
 - "name": snapshot name
+- "device": block device name
 
 Example:
 
--> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
+-> { "execute": "savevm",
+     "arguments": { "name": "snapshot1", "device": "ide1-hd0" } }
 <- { "return": {} }
 
 EQMP
 
     {
         .name       = "savevm",
-        .args_type  = "name:s",
+        .args_type  = "name:s,device:s?",
         .mhandler.cmd_new = qmp_marshal_savevm,
     },
 
-- 
2.5.0

[Qemu-devel] [PATCH 8/8] block: allow to skip block driver in selection of one for VM state saving

[Denis V. Lunev]

Some block drives like PFLASH ones in OVFM setup are not suitable for
VM state saving. Allow option to ban them in this selection.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   |  7 +++++++
 block/snapshot.c          | 10 ++++++++++
 include/block/block_int.h |  3 +++
 3 files changed, 20 insertions(+)

diff --git a/block.c b/block.c
index 01655de..c36393f 100644
--- a/block.c
+++ b/block.c
@@ -895,6 +895,12 @@ static QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Ignore flush requests",
         },
+        {
+            .name = "vmstate",
+            .type = QEMU_OPT_BOOL,
+            .help = "Allow to select to save VM state",
+            .def_value_str = "on",
+        },
         { /* end of list */ }
     },
 };
@@ -957,6 +963,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     bs->zero_beyond_eof = true;
     open_flags = bdrv_open_flags(bs, flags);
     bs->read_only = !(open_flags & BDRV_O_RDWR);
+    bs->enable_vmstate = qemu_opt_get_bool(opts, "vmstate", true);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
         error_setg(errp,
diff --git a/block/snapshot.c b/block/snapshot.c
index 237a015..eb73c76 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -498,6 +498,12 @@ BlockDriverState *bdrv_all_find_vmstate_bs(const char *device, Error **errp)
                        device);
             return NULL;
         }
+        if (!bs->enable_vmstate) {
+            error_setg(errp,
+                       "It is not allowed to save VM state to the device '%s'",
+                       device);
+            return NULL;
+        }
 
         ctx = bdrv_get_aio_context(bs);
 
@@ -515,6 +521,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(const char *device, Error **errp)
     while (not_found && (bs = bdrv_next(bs))) {
         ctx = bdrv_get_aio_context(bs);
 
+        if (!bs->enable_vmstate) {
+            continue;
+        }
+
         aio_context_acquire(ctx);
         not_found = !bdrv_can_snapshot(bs);
         aio_context_release(ctx);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 256609d..a30b576 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -438,6 +438,9 @@ struct BlockDriverState {
     /* do we need to tell the quest if we have a volatile write cache? */
     int enable_write_cache;
 
+    /* allow to save VM state on snapshot here */
+    bool enable_vmstate;
+
     /* the following member gives a name to every node on the bs graph. */
     char node_name[32];
     /* element of the list of named nodes building the graph */
-- 
2.5.0

[Qemu-devel] [PATCH v6 8/8] block: allow to skip block driver in selection of one for VM state saving

[Denis V. Lunev]

Some block drives like PFLASH ones in OVFM setup are not suitable for
VM state saving. Allow option to ban them in this selection.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
---
Changes from v5:
- added to vmstate option to blkdev JSON

 block.c                   |  7 +++++++
 block/snapshot.c          | 10 ++++++++++
 include/block/block_int.h |  3 +++
 qapi/block-core.json      |  5 ++++-
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 01655de..08c1db6 100644
--- a/block.c
+++ b/block.c
@@ -895,6 +895,12 @@ static QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Ignore flush requests",
         },
+        {
+            .name = "vmstate",
+            .type = QEMU_OPT_BOOL,
+            .help = "Allow to to save VM state to this block device",
+            .def_value_str = "on",
+        },
         { /* end of list */ }
     },
 };
@@ -957,6 +963,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     bs->zero_beyond_eof = true;
     open_flags = bdrv_open_flags(bs, flags);
     bs->read_only = !(open_flags & BDRV_O_RDWR);
+    bs->enable_vmstate = qemu_opt_get_bool(opts, "vmstate", true);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
         error_setg(errp,
diff --git a/block/snapshot.c b/block/snapshot.c
index 237a015..eb73c76 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -498,6 +498,12 @@ BlockDriverState *bdrv_all_find_vmstate_bs(const char *device, Error **errp)
                        device);
             return NULL;
         }
+        if (!bs->enable_vmstate) {
+            error_setg(errp,
+                       "It is not allowed to save VM state to the device '%s'",
+                       device);
+            return NULL;
+        }
 
         ctx = bdrv_get_aio_context(bs);
 
@@ -515,6 +521,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(const char *device, Error **errp)
     while (not_found && (bs = bdrv_next(bs))) {
         ctx = bdrv_get_aio_context(bs);
 
+        if (!bs->enable_vmstate) {
+            continue;
+        }
+
         aio_context_acquire(ctx);
         not_found = !bdrv_can_snapshot(bs);
         aio_context_release(ctx);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 256609d..a30b576 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -438,6 +438,9 @@ struct BlockDriverState {
     /* do we need to tell the quest if we have a volatile write cache? */
     int enable_write_cache;
 
+    /* allow to save VM state on snapshot here */
+    bool enable_vmstate;
+
     /* the following member gives a name to every node on the bs graph. */
     char node_name[32];
     /* element of the list of named nodes building the graph */
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0a915ed..d04453f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1583,6 +1583,8 @@
 #                   statistics, in seconds (default: none) (Since 2.5)
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
+# @vmstate: #optional allow to save VM state to this device (Since 2.6)
+#           (default: on)
 #
 # Since: 1.7
 ##
@@ -1599,7 +1601,8 @@
             '*stats-account-invalid': 'bool',
             '*stats-account-failed': 'bool',
             '*stats-intervals': ['int'],
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*vmstate': 'bool' } }
 
 ##
 # @BlockdevOptionsFile
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/8] migration: split hmp_savevm to migrate_savevm and hmp_savevm wrapper

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> This would be useful in the next step when QMP version of this call will
> be introduced. The patch also moves snapshot name generation to the
> hmp specific code as QMP version of this code will require the name
> on the protocol level.
>
> Addition of migration_savevm to migration/migration.h is temporary. It
> will be removed in the next patch with QMP level change.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
>  hmp.c                         | 26 ++++++++++++++++++++++++
>  include/migration/migration.h |  3 +++
>  migration/savevm.c            | 46 +++++++++++++++++--------------------------
>  3 files changed, 47 insertions(+), 28 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index c2b2c16..e7bab75 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -18,6 +18,7 @@
>  #include "net/eth.h"
>  #include "sysemu/char.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/sysemu.h"
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
>  #include "qmp-commands.h"
> @@ -32,6 +33,7 @@
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> +#include "migration/migration.h"
>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -2386,3 +2388,27 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
>  
>      qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    char name_buf[64];
> +
> +    if (name == NULL) {

I'd prefer !name.

> +        qemu_timeval tv;
> +        struct tm tm;
> +
> +        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> +        localtime_r((const time_t *)&tv.tv_sec, &tm);

I'm afraid this uses tv.tv_sec uninitialized.  Not sure how this
survived testing :)

Aside: the ugly cast comes from commit d7d9b52.  It works when the
system conforms to POSIX (tv_sec is time_t), or when tv_sec's type is
essentially the same as time_t (supposedly the case in old OpenBSD).  As
far as I can tell, OpenBSD was fixed in 2013.

> +        strftime(name_buf, sizeof(name_buf), "vm-%Y%m%d%H%M%S", &tm);
> +
> +        name = name_buf;
> +    }
> +
> +    migrate_savevm(name, &local_err);
> +
> +    if (local_err != NULL) {
> +        error_report_err(local_err);
> +    }
> +}
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index d9494b8..73c8bb1 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -277,6 +277,8 @@ int migrate_compress_threads(void);
>  int migrate_decompress_threads(void);
>  bool migrate_use_events(void);
>  
> +void migrate_savevm(const char *name, Error **errp);
> +
>  /* Sending on the return path - generic and then for each message type */
>  void migrate_send_rp_message(MigrationIncomingState *mis,
>                               enum mig_rp_message_type message_type,
> @@ -321,4 +323,5 @@ int ram_save_queue_pages(MigrationState *ms, const char *rbname,
>  PostcopyState postcopy_state_get(void);
>  /* Set the state and return the old state */
>  PostcopyState postcopy_state_set(PostcopyState new_state);
> +
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0ad1b93..308302a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> +void migrate_savevm(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1914,29 +1914,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      int saved_vm_running;
>      uint64_t vm_state_size;
>      qemu_timeval tv;
> -    struct tm tm;
> -    const char *name = qdict_get_try_str(qdict, "name");
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
>      if (!bdrv_all_can_snapshot(&bs)) {
> -        monitor_printf(mon, "Device '%s' is writable but does not "
> -                       "support snapshots.\n", bdrv_get_device_name(bs));
> +        error_setg(errp,
> +                   "Device '%s' is writable but does not support snapshots",
> +                   bdrv_get_device_name(bs));
>          return;
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> -        monitor_printf(mon,
> -                       "Error while deleting snapshot on device '%s': %s\n",
> -                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
> +    if (bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));

Before the patch, we delete snapshots only when name comes from the
user, not when we made it up.

After the patch, we delete always.

What happens before the patch when we make up a name, a snapshot with
this name exists, but we don't delete it?

Should deleting old snapshots move to the HMP command as well?

>          error_free(local_err);
>          return;
>      }
>  
>      bs = bdrv_all_find_vmstate_bs();
>      if (bs == NULL) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> +        error_setg(errp, "No block device can accept snapshots");
>          return;
>      }
>      aio_context = bdrv_get_aio_context(bs);
> @@ -1945,7 +1943,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>  
>      ret = global_state_store();
>      if (ret) {
> -        monitor_printf(mon, "Error saving global state\n");
> +        error_setg(errp, "Error saving global state");
>          return;
>      }
>      vm_stop(RUN_STATE_SAVE_VM);
> @@ -1960,39 +1958,31 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      sn->date_nsec = tv.tv_usec * 1000;
>      sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  
> -    if (name) {
> -        ret = bdrv_snapshot_find(bs, old_sn, name);
> -        if (ret >= 0) {
> -            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> -        } else {
> -            pstrcpy(sn->name, sizeof(sn->name), name);
> -        }
> +    ret = bdrv_snapshot_find(bs, old_sn, name);
> +    if (ret >= 0) {
> +        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> +        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>      } else {
> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> -        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> +        pstrcpy(sn->name, sizeof(sn->name), name);
>      }

Another place where we now use the code for "name from user" for made-up
name as well.  Not sure about the impact.

>  
>      /* save the VM state */
>      f = qemu_fopen_bdrv(bs, 1);
>      if (!f) {
> -        monitor_printf(mon, "Could not open VM state file\n");
> +        error_setg(errp, "Could not open VM state file");
>          goto the_end;
>      }
> -    ret = qemu_savevm_state(f, &local_err);
> +    ret = qemu_savevm_state(f, errp);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> -        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> -        error_free(local_err);
>          goto the_end;
>      }
>  
>      ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
>      if (ret < 0) {
> -        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> -                       bdrv_get_device_name(bs));
> +        error_setg(errp, "Error while creating snapshot on '%s'",
> +                   bdrv_get_device_name(bs));
>      }
>  
>   the_end:

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

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> 'name' attribute is made mandatory in distinction with HMP command.
>
> The patch also moves hmp_savevm implementation into hmp.c. This function
> is just a simple wrapper now and does not have knowledge about
> migration internals.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  hmp.c                         |  3 +--
>  include/migration/migration.h |  2 --
>  migration/savevm.c            |  2 +-
>  qapi-schema.json              | 13 +++++++++++++
>  qmp-commands.hx               | 25 +++++++++++++++++++++++++
>  5 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index e7bab75..8d6eda6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -33,7 +33,6 @@
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> -#include "migration/migration.h"
>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -2406,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>          name = name_buf;
>      }
>  
> -    migrate_savevm(name, &local_err);
> +    qmp_savevm(name, &local_err);
>  
>      if (local_err != NULL) {
>          error_report_err(local_err);
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 73c8bb1..58c04a9 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -277,8 +277,6 @@ int migrate_compress_threads(void);
>  int migrate_decompress_threads(void);
>  bool migrate_use_events(void);
>  
> -void migrate_savevm(const char *name, Error **errp);
> -
>  /* Sending on the return path - generic and then for each message type */
>  void migrate_send_rp_message(MigrationIncomingState *mis,
>                               enum mig_rp_message_type message_type,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 308302a..0dbb15f 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -void migrate_savevm(const char *name, Error **errp)
> +void qmp_savevm(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 2e31733..09d1a1a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4054,3 +4054,16 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @savevm
> +#
> +# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
> +#
> +# @name: identifier of a snapshot to be created
> +#
> +# Returns: Nothing on success
> +#
> +# Since 2.6
> +##
> +{ 'command': 'savevm', 'data': {'name': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index db072a6..b7851e1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4795,3 +4795,28 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +SQMP
> +savevm
> +------
> +
> +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
> +
> +Arguments:
> +
> +- "name": snapshot name
> +
> +Example:
> +
> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "savevm",
> +        .args_type  = "name:s",
> +        .mhandler.cmd_new = qmp_marshal_savevm,
> +    },

A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
(QEMUSnapshotInfo member id_str).

HMP's name arguments are overloaded: they're matched both against tag
and ID.  Unwisely chosen tags can create ambiguity.  Example:

    (qemu) savevm 2
    (qemu) savevm 
    (qemu) info snapshots
    ID        TAG                 VM SIZE                DATE       VM CLOCK
    1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
    2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000

Care to guess which one we get when we ask for "2"?

I think we want separate, unoverloaded arguments for QMP.

Re: [Qemu-devel] [PATCH 1/8] migration: split hmp_savevm to migrate_savevm and hmp_savevm wrapper

[Denis V. Lunev]

On 01/18/2016 06:47 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> This would be useful in the next step when QMP version of this call will
>> be introduced. The patch also moves snapshot name generation to the
>> hmp specific code as QMP version of this code will require the name
>> on the protocol level.
>>
>> Addition of migration_savevm to migration/migration.h is temporary. It
>> will be removed in the next patch with QMP level change.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> ---
>>   hmp.c                         | 26 ++++++++++++++++++++++++
>>   include/migration/migration.h |  3 +++
>>   migration/savevm.c            | 46 +++++++++++++++++--------------------------
>>   3 files changed, 47 insertions(+), 28 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index c2b2c16..e7bab75 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -18,6 +18,7 @@
>>   #include "net/eth.h"
>>   #include "sysemu/char.h"
>>   #include "sysemu/block-backend.h"
>> +#include "sysemu/sysemu.h"
>>   #include "qemu/option.h"
>>   #include "qemu/timer.h"
>>   #include "qmp-commands.h"
>> @@ -32,6 +33,7 @@
>>   #include "ui/console.h"
>>   #include "block/qapi.h"
>>   #include "qemu-io.h"
>> +#include "migration/migration.h"
>>   
>>   #ifdef CONFIG_SPICE
>>   #include <spice/enums.h>
>> @@ -2386,3 +2388,27 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
>>   
>>       qapi_free_RockerOfDpaGroupList(list);
>>   }
>> +
>> +void hmp_savevm(Monitor *mon, const QDict *qdict)
>> +{
>> +    Error *local_err = NULL;
>> +    const char *name = qdict_get_try_str(qdict, "name");
>> +    char name_buf[64];
>> +
>> +    if (name == NULL) {
> I'd prefer !name.
>
>> +        qemu_timeval tv;
>> +        struct tm tm;
>> +
>> +        /* cast below needed for OpenBSD where tv_sec is still 'long' */
>> +        localtime_r((const time_t *)&tv.tv_sec, &tm);
> I'm afraid this uses tv.tv_sec uninitialized.  Not sure how this
> survived testing :)
the name is generated :) somehow....
OK, this should be definitely fixed

> Aside: the ugly cast comes from commit d7d9b52.  It works when the
> system conforms to POSIX (tv_sec is time_t), or when tv_sec's type is
> essentially the same as time_t (supposedly the case in old OpenBSD).  As
> far as I can tell, OpenBSD was fixed in 2013.
this is copied from the original code. I do not think
that this should be fixed in this commit. We could
make additional commit removing this ugly cast.
I don't want to have this patch reverted if something
goes wrong during merge.

>> +        strftime(name_buf, sizeof(name_buf), "vm-%Y%m%d%H%M%S", &tm);
>> +
>> +        name = name_buf;
>> +    }
>> +
>> +    migrate_savevm(name, &local_err);
>> +
>> +    if (local_err != NULL) {
>> +        error_report_err(local_err);
>> +    }
>> +}
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index d9494b8..73c8bb1 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -277,6 +277,8 @@ int migrate_compress_threads(void);
>>   int migrate_decompress_threads(void);
>>   bool migrate_use_events(void);
>>   
>> +void migrate_savevm(const char *name, Error **errp);
>> +
>>   /* Sending on the return path - generic and then for each message type */
>>   void migrate_send_rp_message(MigrationIncomingState *mis,
>>                                enum mig_rp_message_type message_type,
>> @@ -321,4 +323,5 @@ int ram_save_queue_pages(MigrationState *ms, const char *rbname,
>>   PostcopyState postcopy_state_get(void);
>>   /* Set the state and return the old state */
>>   PostcopyState postcopy_state_set(PostcopyState new_state);
>> +
>>   #endif
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 0ad1b93..308302a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>       return ret;
>>   }
>>   
>> -void hmp_savevm(Monitor *mon, const QDict *qdict)
>> +void migrate_savevm(const char *name, Error **errp)
>>   {
>>       BlockDriverState *bs, *bs1;
>>       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
>> @@ -1914,29 +1914,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>       int saved_vm_running;
>>       uint64_t vm_state_size;
>>       qemu_timeval tv;
>> -    struct tm tm;
>> -    const char *name = qdict_get_try_str(qdict, "name");
>>       Error *local_err = NULL;
>>       AioContext *aio_context;
>>   
>>       if (!bdrv_all_can_snapshot(&bs)) {
>> -        monitor_printf(mon, "Device '%s' is writable but does not "
>> -                       "support snapshots.\n", bdrv_get_device_name(bs));
>> +        error_setg(errp,
>> +                   "Device '%s' is writable but does not support snapshots",
>> +                   bdrv_get_device_name(bs));
>>           return;
>>       }
>>   
>>       /* Delete old snapshots of the same name */
>> -    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
>> -        monitor_printf(mon,
>> -                       "Error while deleting snapshot on device '%s': %s\n",
>> -                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
>> +    if (bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
>> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
>> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
> Before the patch, we delete snapshots only when name comes from the
> user, not when we made it up.
>
> After the patch, we delete always.
>
> What happens before the patch when we make up a name, a snapshot with
> this name exists, but we don't delete it?
I think that we will have problems later on in f.e. qcow2_snapshot_create

> Should deleting old snapshots move to the HMP command as well?

actually I don't want to. In this case we will have to add here the
code to check that there are no snapshots with this name on
all states. This will place us into the ugly loop:
- hmp_info_snapshots reports snapshot names which are available
   on ALL drives only
- thus if the snapshot resides on one image out of four we will not
   be able to detect this in advace

I think that we should remove here and this is correct.

>>           error_free(local_err);
>>           return;
>>       }
>>   
>>       bs = bdrv_all_find_vmstate_bs();
>>       if (bs == NULL) {
>> -        monitor_printf(mon, "No block device can accept snapshots\n");
>> +        error_setg(errp, "No block device can accept snapshots");
>>           return;
>>       }
>>       aio_context = bdrv_get_aio_context(bs);
>> @@ -1945,7 +1943,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>   
>>       ret = global_state_store();
>>       if (ret) {
>> -        monitor_printf(mon, "Error saving global state\n");
>> +        error_setg(errp, "Error saving global state");
>>           return;
>>       }
>>       vm_stop(RUN_STATE_SAVE_VM);
>> @@ -1960,39 +1958,31 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>       sn->date_nsec = tv.tv_usec * 1000;
>>       sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>   
>> -    if (name) {
>> -        ret = bdrv_snapshot_find(bs, old_sn, name);
>> -        if (ret >= 0) {
>> -            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>> -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>> -        } else {
>> -            pstrcpy(sn->name, sizeof(sn->name), name);
>> -        }
>> +    ret = bdrv_snapshot_find(bs, old_sn, name);
>> +    if (ret >= 0) {
>> +        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>> +        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>>       } else {
>> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
>> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
>> -        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
>> +        pstrcpy(sn->name, sizeof(sn->name), name);
>>       }
> Another place where we now use the code for "name from user" for made-up
> name as well.  Not sure about the impact.
this resides in HMP only. I hope that original functionality is preserved.

>>   
>>       /* save the VM state */
>>       f = qemu_fopen_bdrv(bs, 1);
>>       if (!f) {
>> -        monitor_printf(mon, "Could not open VM state file\n");
>> +        error_setg(errp, "Could not open VM state file");
>>           goto the_end;
>>       }
>> -    ret = qemu_savevm_state(f, &local_err);
>> +    ret = qemu_savevm_state(f, errp);
>>       vm_state_size = qemu_ftell(f);
>>       qemu_fclose(f);
>>       if (ret < 0) {
>> -        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
>> -        error_free(local_err);
>>           goto the_end;
>>       }
>>   
>>       ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
>>       if (ret < 0) {
>> -        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
>> -                       bdrv_get_device_name(bs));
>> +        error_setg(errp, "Error while creating snapshot on '%s'",
>> +                   bdrv_get_device_name(bs));
>>       }
>>   
>>    the_end:

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

[Denis V. Lunev]

On 01/18/2016 06:58 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> 'name' attribute is made mandatory in distinction with HMP command.
>>
>> The patch also moves hmp_savevm implementation into hmp.c. This function
>> is just a simple wrapper now and does not have knowledge about
>> migration internals.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>   hmp.c                         |  3 +--
>>   include/migration/migration.h |  2 --
>>   migration/savevm.c            |  2 +-
>>   qapi-schema.json              | 13 +++++++++++++
>>   qmp-commands.hx               | 25 +++++++++++++++++++++++++
>>   5 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index e7bab75..8d6eda6 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -33,7 +33,6 @@
>>   #include "ui/console.h"
>>   #include "block/qapi.h"
>>   #include "qemu-io.h"
>> -#include "migration/migration.h"
>>   
>>   #ifdef CONFIG_SPICE
>>   #include <spice/enums.h>
>> @@ -2406,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>           name = name_buf;
>>       }
>>   
>> -    migrate_savevm(name, &local_err);
>> +    qmp_savevm(name, &local_err);
>>   
>>       if (local_err != NULL) {
>>           error_report_err(local_err);
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 73c8bb1..58c04a9 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -277,8 +277,6 @@ int migrate_compress_threads(void);
>>   int migrate_decompress_threads(void);
>>   bool migrate_use_events(void);
>>   
>> -void migrate_savevm(const char *name, Error **errp);
>> -
>>   /* Sending on the return path - generic and then for each message type */
>>   void migrate_send_rp_message(MigrationIncomingState *mis,
>>                                enum mig_rp_message_type message_type,
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 308302a..0dbb15f 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>       return ret;
>>   }
>>   
>> -void migrate_savevm(const char *name, Error **errp)
>> +void qmp_savevm(const char *name, Error **errp)
>>   {
>>       BlockDriverState *bs, *bs1;
>>       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 2e31733..09d1a1a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -4054,3 +4054,16 @@
>>   ##
>>   { 'enum': 'ReplayMode',
>>     'data': [ 'none', 'record', 'play' ] }
>> +
>> +##
>> +# @savevm
>> +#
>> +# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>> +#
>> +# @name: identifier of a snapshot to be created
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since 2.6
>> +##
>> +{ 'command': 'savevm', 'data': {'name': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index db072a6..b7851e1 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4795,3 +4795,28 @@ Example:
>>                    {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>                     "pop-vlan": 1, "id": 251658240}
>>      ]}
>> +
>> +EQMP
>> +
>> +SQMP
>> +savevm
>> +------
>> +
>> +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>> +
>> +Arguments:
>> +
>> +- "name": snapshot name
>> +
>> +Example:
>> +
>> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>> +        .name       = "savevm",
>> +        .args_type  = "name:s",
>> +        .mhandler.cmd_new = qmp_marshal_savevm,
>> +    },
> A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
> (QEMUSnapshotInfo member id_str).
>
> HMP's name arguments are overloaded: they're matched both against tag
> and ID.  Unwisely chosen tags can create ambiguity.  Example:
>
>      (qemu) savevm 2
>      (qemu) savevm
>      (qemu) info snapshots
>      ID        TAG                 VM SIZE                DATE       VM CLOCK
>      1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
>      2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000
>
> Care to guess which one we get when we ask for "2"?
>
> I think we want separate, unoverloaded arguments for QMP.
I think there is no need to. Name is now absolutely mandatory.
Thus for new snapshots we will have 'name' specified and we
will be bound to name only.

'id' will be used for old VMs and this is convenience
layer to make old 'id' only snaphosts accessible
through new interface in the same way as old.

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

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> On 01/18/2016 06:58 PM, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>>
>>> 'name' attribute is made mandatory in distinction with HMP command.
>>>
>>> The patch also moves hmp_savevm implementation into hmp.c. This function
>>> is just a simple wrapper now and does not have knowledge about
>>> migration internals.
[...]
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 2e31733..09d1a1a 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -4054,3 +4054,16 @@
>>>   ##
>>>   { 'enum': 'ReplayMode',
>>>     'data': [ 'none', 'record', 'play' ] }
>>> +
>>> +##
>>> +# @savevm
>>> +#
>>> +# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>> +#
>>> +# @name: identifier of a snapshot to be created
>>> +#
>>> +# Returns: Nothing on success
>>> +#
>>> +# Since 2.6
>>> +##
>>> +{ 'command': 'savevm', 'data': {'name': 'str'} }
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index db072a6..b7851e1 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -4795,3 +4795,28 @@ Example:
>>>                    {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>>                     "pop-vlan": 1, "id": 251658240}
>>>      ]}
>>> +
>>> +EQMP
>>> +
>>> +SQMP
>>> +savevm
>>> +------
>>> +
>>> +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>> +
>>> +Arguments:
>>> +
>>> +- "name": snapshot name
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
>>> +<- { "return": {} }
>>> +
>>> +EQMP
>>> +
>>> +    {
>>> +        .name       = "savevm",
>>> +        .args_type  = "name:s",
>>> +        .mhandler.cmd_new = qmp_marshal_savevm,
>>> +    },
>> A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
>> (QEMUSnapshotInfo member id_str).
>>
>> HMP's name arguments are overloaded: they're matched both against tag
>> and ID.  Unwisely chosen tags can create ambiguity.  Example:
>>
>>      (qemu) savevm 2
>>      (qemu) savevm
>>      (qemu) info snapshots
>>      ID        TAG                 VM SIZE                DATE       VM CLOCK
>>      1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
>>      2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000
>>
>> Care to guess which one we get when we ask for "2"?
>>
>> I think we want separate, unoverloaded arguments for QMP.
> I think there is no need to. Name is now absolutely mandatory.
> Thus for new snapshots we will have 'name' specified and we
> will be bound to name only.
>
> 'id' will be used for old VMs and this is convenience
> layer to make old 'id' only snaphosts accessible
> through new interface in the same way as old.

The overloaded interface you propose is more complex than it seems.  You
hide the complexity by not documenting its workings.  Not even to the
(insufficient!) degree the HMP interface documents how its overloaded
name parameters work.

Merely copying over the HMP documentation won't cut it.  The bar for new
QMP interfaces is a fair bit higher than "no worse than HMP".  The new
interface must reasonably sane for *QMP*, and sufficiently documented.

If we can't make a sane QMP interface, I'd rather have no QMP interface.
However, I believe we *can* make a sane QMP interface if we put in the
design work.

The design work must start with a review of what we're trying to
accomplish, and how to fit it into the rest of the system.  Here's my
attempt.  Since my knowledge on snapshots is rather superficial, I'm
cc'ing Kevin for additional snapshot expertise.  Kevin, please correct
me when I talk nonsense.  I'm further cc'ing Eric and Peter for the
management layer perspective.

A point-in-time snapshot of a system consists of a snapshot of its
machine state and snapshots of its storage.  All the snapshots need to
be made at the same point in time for the result to be consistent.

Snapshots of read-only storage carry no information and are commonly
omitted.

Isolated storage snapshots can make sense, but snapshotting the machine
state without also snapshotting the machine's storage doesn't sound
useful to me.

Both storage and machine state snapshots come in two flavours: internal
and external.

External ones can be made with any block backend, but internal storage
snapshots work only with certain formats, notably qcow2.  QMP supports
both kinds of storage snapshots.

Both kinds of storage snapshots need exclusive access while they work.
They're relatively quick, but the delay could be noticable for large
internal snapshots, and perhaps for external snapshots on really slow
storage.

Internal machine state snapshots are currently only available via HMP's
savevm, which integrates internal machine state and storage snapshots.
This is non-live, i.e. the guest is stopped while the snapshot gets
saved.  I figure we could make it live if we really wanted to.  Another
instance of the emerging background job concept.

On the implementation level, QCOW2 can't currently store a machine state
snapshot without also storing a storage snapshot.  I guess we could
change this if we really wanted to.

External machine state snapshots are basically migrate to file.
Supported by QMP.

Live migration to file is possible, but currently wastes space, because
memory dirtied during migration gets saved multiple times.  Could be
fixed either by making migration update previously saved memory instead
of appending (beware, random I/O), or by compacting the file afterwards.

Non-live migration to file doesn't waste space that way.

To take multiple *consistent* snapshots, you have to bundle them up in a
transaction.  Transactions currently support only *storage* snapshots,
though.

Work-around for external machine state snapshot: migrate to file
(possibly live), leaving the guest sopped on completion, take storage
snapshots, resume guest.

You can combine internal and external storage snapshots with an external
machine state snapshot to get a mixed system snapshot.

You currently can't do that with an internal machine state snapshot: the
only way to take one is HMP savevm, which insists on internally
snapshotting all writable storage, and doesn't transact together with
external storage snapshots.

Except for the case "purely internal snapshot with just one writable
storage device", a system snapshot consists of multiple parts stored in
separate files.  Tying the parts together is a management problem.  QEMU
provides rudimentary management for purely internal snapshots, but it's
flawed: missing storage isn't detected, and additional storage can creep
in if snapshot tags or IDs happen to match.  I guess managing the parts
is better left to the management layer.

I figure a fully general QMP interface would let you perform a system
snapshot by combining storage snapshots of either kind with either kind
of machine state snapshot.

We already have most of the building blocks: we can take external and
internal storage snapshots, and combine them in transactions.

What's missing is transactionable machine state snapshots.

We know how to work around it for external machine state snapshots (see
above), but a transaction-based solution might be nicer.

Any solution for internal machine state snapshots in QMP should at least
try to fit into this.  Some restrictions are okay.  For instance, we
probably need to restrict internal machine state snapshots to piggyback
on an internal storage snapshot for now, so we don't have to dig up
QCOW2 just to get QMP support.

We can talk about more convenient interfaces for common special cases,
but I feel we need to design for the general case.  We don't have to
implement the completely general case right away, though.  As long as we
know where we want to go, incremental steps towards that goal are fine.

Can we create a transactionable QMP command to take an internal machine
state snapshot?

This would be like HMP savevm with the following differences:

* Separate parameters for tag and ID.  I'll have none of this
  overloading nonsense in QMP.

* Specify the destination block device.  I'll have none of this "pick a
  device in some magic, undocumented way" in QMP.

* Leave alone the other block devices.  Adding transaction actions to
  snapshot all the writable block devices to get a full system snapshot
  is the user's responsibility.

Limitations:

* No live internal machine snapshot, yet.

* The storage device taking the internal snapshot must also be
  internally snapshot for now.  In fact, the command does both
  (tolerable wart).

Open questions:

* Do we want the QMP command to delete existing snapshots with
  conflicting tag / ID, like HMP savevm does?  Or do we want it to fail
  the transaction?

* Do we need transactions for switching to a system snapshot, too?

Opinions?

Denis, I understand doing this right will be more work than simply
rebadging HMP's savevm etc for QMP, and probably a good deal more than
you anticipated.  Sorry about that.  I'm trying to accomodate you as far
as possible without screwing up QMP.  Not screwing up QMP too much is my
job as maintainer.

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

[Denis V. Lunev]

On 01/19/2016 09:11 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> On 01/18/2016 06:58 PM, Markus Armbruster wrote:
>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>
>>>> 'name' attribute is made mandatory in distinction with HMP command.
>>>>
>>>> The patch also moves hmp_savevm implementation into hmp.c. This function
>>>> is just a simple wrapper now and does not have knowledge about
>>>> migration internals.
> [...]
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 2e31733..09d1a1a 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -4054,3 +4054,16 @@
>>>>    ##
>>>>    { 'enum': 'ReplayMode',
>>>>      'data': [ 'none', 'record', 'play' ] }
>>>> +
>>>> +##
>>>> +# @savevm
>>>> +#
>>>> +# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>>> +#
>>>> +# @name: identifier of a snapshot to be created
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since 2.6
>>>> +##
>>>> +{ 'command': 'savevm', 'data': {'name': 'str'} }
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index db072a6..b7851e1 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -4795,3 +4795,28 @@ Example:
>>>>                     {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>>>                      "pop-vlan": 1, "id": 251658240}
>>>>       ]}
>>>> +
>>>> +EQMP
>>>> +
>>>> +SQMP
>>>> +savevm
>>>> +------
>>>> +
>>>> +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "name": snapshot name
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>> +        .name       = "savevm",
>>>> +        .args_type  = "name:s",
>>>> +        .mhandler.cmd_new = qmp_marshal_savevm,
>>>> +    },
>>> A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
>>> (QEMUSnapshotInfo member id_str).
>>>
>>> HMP's name arguments are overloaded: they're matched both against tag
>>> and ID.  Unwisely chosen tags can create ambiguity.  Example:
>>>
>>>       (qemu) savevm 2
>>>       (qemu) savevm
>>>       (qemu) info snapshots
>>>       ID        TAG                 VM SIZE                DATE       VM CLOCK
>>>       1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
>>>       2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000
>>>
>>> Care to guess which one we get when we ask for "2"?
>>>
>>> I think we want separate, unoverloaded arguments for QMP.
>> I think there is no need to. Name is now absolutely mandatory.
>> Thus for new snapshots we will have 'name' specified and we
>> will be bound to name only.
>>
>> 'id' will be used for old VMs and this is convenience
>> layer to make old 'id' only snaphosts accessible
>> through new interface in the same way as old.
> The overloaded interface you propose is more complex than it seems.  You
> hide the complexity by not documenting its workings.  Not even to the
> (insufficient!) degree the HMP interface documents how its overloaded
> name parameters work.
>
> Merely copying over the HMP documentation won't cut it.  The bar for new
> QMP interfaces is a fair bit higher than "no worse than HMP".  The new
> interface must reasonably sane for *QMP*, and sufficiently documented.
>
> If we can't make a sane QMP interface, I'd rather have no QMP interface.
> However, I believe we *can* make a sane QMP interface if we put in the
> design work.
>
> The design work must start with a review of what we're trying to
> accomplish, and how to fit it into the rest of the system.  Here's my
> attempt.  Since my knowledge on snapshots is rather superficial, I'm
> cc'ing Kevin for additional snapshot expertise.  Kevin, please correct
> me when I talk nonsense.  I'm further cc'ing Eric and Peter for the
> management layer perspective.
>
> A point-in-time snapshot of a system consists of a snapshot of its
> machine state and snapshots of its storage.  All the snapshots need to
> be made at the same point in time for the result to be consistent.
>
> Snapshots of read-only storage carry no information and are commonly
> omitted.
>
> Isolated storage snapshots can make sense, but snapshotting the machine
> state without also snapshotting the machine's storage doesn't sound
> useful to me.
>
> Both storage and machine state snapshots come in two flavours: internal
> and external.
>
> External ones can be made with any block backend, but internal storage
> snapshots work only with certain formats, notably qcow2.  QMP supports
> both kinds of storage snapshots.
>
> Both kinds of storage snapshots need exclusive access while they work.
> They're relatively quick, but the delay could be noticable for large
> internal snapshots, and perhaps for external snapshots on really slow
> storage.
>
> Internal machine state snapshots are currently only available via HMP's
> savevm, which integrates internal machine state and storage snapshots.
> This is non-live, i.e. the guest is stopped while the snapshot gets
> saved.  I figure we could make it live if we really wanted to.  Another
> instance of the emerging background job concept.
>
> On the implementation level, QCOW2 can't currently store a machine state
> snapshot without also storing a storage snapshot.  I guess we could
> change this if we really wanted to.
>
> External machine state snapshots are basically migrate to file.
> Supported by QMP.
>
> Live migration to file is possible, but currently wastes space, because
> memory dirtied during migration gets saved multiple times.  Could be
> fixed either by making migration update previously saved memory instead
> of appending (beware, random I/O), or by compacting the file afterwards.
>
> Non-live migration to file doesn't waste space that way.
>
> To take multiple *consistent* snapshots, you have to bundle them up in a
> transaction.  Transactions currently support only *storage* snapshots,
> though.
>
> Work-around for external machine state snapshot: migrate to file
> (possibly live), leaving the guest sopped on completion, take storage
> snapshots, resume guest.
>
> You can combine internal and external storage snapshots with an external
> machine state snapshot to get a mixed system snapshot.
>
> You currently can't do that with an internal machine state snapshot: the
> only way to take one is HMP savevm, which insists on internally
> snapshotting all writable storage, and doesn't transact together with
> external storage snapshots.
>
> Except for the case "purely internal snapshot with just one writable
> storage device", a system snapshot consists of multiple parts stored in
> separate files.  Tying the parts together is a management problem.  QEMU
> provides rudimentary management for purely internal snapshots, but it's
> flawed: missing storage isn't detected, and additional storage can creep
> in if snapshot tags or IDs happen to match.  I guess managing the parts
> is better left to the management layer.
>
> I figure a fully general QMP interface would let you perform a system
> snapshot by combining storage snapshots of either kind with either kind
> of machine state snapshot.
>
> We already have most of the building blocks: we can take external and
> internal storage snapshots, and combine them in transactions.
>
> What's missing is transactionable machine state snapshots.
>
> We know how to work around it for external machine state snapshots (see
> above), but a transaction-based solution might be nicer.
>
> Any solution for internal machine state snapshots in QMP should at least
> try to fit into this.  Some restrictions are okay.  For instance, we
> probably need to restrict internal machine state snapshots to piggyback
> on an internal storage snapshot for now, so we don't have to dig up
> QCOW2 just to get QMP support.
>
> We can talk about more convenient interfaces for common special cases,
> but I feel we need to design for the general case.  We don't have to
> implement the completely general case right away, though.  As long as we
> know where we want to go, incremental steps towards that goal are fine.
>
> Can we create a transactionable QMP command to take an internal machine
> state snapshot?
>
> This would be like HMP savevm with the following differences:
>
> * Separate parameters for tag and ID.  I'll have none of this
>    overloading nonsense in QMP.
why do we need both 'name' and 'id' values in the future at all?
Why single descriptive parameter is not enough? From my point
of view all this overloading is non-sense and in future one
name parameter should be kept.

> * Specify the destination block device.  I'll have none of this "pick a
>    device in some magic, undocumented way" in QMP.
agreed

> * Leave alone the other block devices.  Adding transaction actions to
>    snapshot all the writable block devices to get a full system snapshot
>    is the user's responsibility.
this requires clarification:
- do you mean that the list of the devices should come from the 
management layer?
If so this is error prone at the restoration stage. From my point of 
view restoration
must check that all RW devices are restored properly and there are no missed
ones. Do you propose to put this into libvirt? From my point of view 
this flexibility
is not that necessary.

There is one important thing missed in your description.
Machine snapshot MUST handle configuration change. This means
that at the moment of snapshot VM can have different amount of
disks and other peripherals and this should be properly handled.


> Limitations:
>
> * No live internal machine snapshot, yet.
>
> * The storage device taking the internal snapshot must also be
>    internally snapshot for now.  In fact, the command does both
>    (tolerable wart).
>
> Open questions:
>
> * Do we want the QMP command to delete existing snapshots with
>    conflicting tag / ID, like HMP savevm does?  Or do we want it to fail
>    the transaction?
>
> * Do we need transactions for switching to a system snapshot, too?
>
> Opinions?
>
> Denis, I understand doing this right will be more work than simply
> rebadging HMP's savevm etc for QMP, and probably a good deal more than
> you anticipated.  Sorry about that.  I'm trying to accomodate you as far
> as possible without screwing up QMP.  Not screwing up QMP too much is my
> job as maintainer.
this is not a problem if we will be able to do things with less
latency. From my side I am a bit frustrated that this discussion
comes with v5 of patches after two months of waiting to be
merged assuming that most of previous discussions were about
some mistakes in the code.

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

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> On 01/19/2016 09:11 PM, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>>
>>> On 01/18/2016 06:58 PM, Markus Armbruster wrote:
>>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>>
>>>>> 'name' attribute is made mandatory in distinction with HMP command.
>>>>>
>>>>> The patch also moves hmp_savevm implementation into hmp.c. This function
>>>>> is just a simple wrapper now and does not have knowledge about
>>>>> migration internals.
>> [...]
>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>> index 2e31733..09d1a1a 100644
>>>>> --- a/qapi-schema.json
>>>>> +++ b/qapi-schema.json
>>>>> @@ -4054,3 +4054,16 @@
>>>>>    ##
>>>>>    { 'enum': 'ReplayMode',
>>>>>      'data': [ 'none', 'record', 'play' ] }
>>>>> +
>>>>> +##
>>>>> +# @savevm
>>>>> +#
>>>>> +# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>>>> +#
>>>>> +# @name: identifier of a snapshot to be created
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since 2.6
>>>>> +##
>>>>> +{ 'command': 'savevm', 'data': {'name': 'str'} }
>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>> index db072a6..b7851e1 100644
>>>>> --- a/qmp-commands.hx
>>>>> +++ b/qmp-commands.hx
>>>>> @@ -4795,3 +4795,28 @@ Example:
>>>>>                     {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>>>>                      "pop-vlan": 1, "id": 251658240}
>>>>>       ]}
>>>>> +
>>>>> +EQMP
>>>>> +
>>>>> +SQMP
>>>>> +savevm
>>>>> +------
>>>>> +
>>>>> +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>>>> +
>>>>> +Arguments:
>>>>> +
>>>>> +- "name": snapshot name
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
>>>>> +<- { "return": {} }
>>>>> +
>>>>> +EQMP
>>>>> +
>>>>> +    {
>>>>> +        .name       = "savevm",
>>>>> +        .args_type  = "name:s",
>>>>> +        .mhandler.cmd_new = qmp_marshal_savevm,
>>>>> +    },
>>>> A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
>>>> (QEMUSnapshotInfo member id_str).
>>>>
>>>> HMP's name arguments are overloaded: they're matched both against tag
>>>> and ID.  Unwisely chosen tags can create ambiguity.  Example:
>>>>
>>>>       (qemu) savevm 2
>>>>       (qemu) savevm
>>>>       (qemu) info snapshots
>>>>       ID        TAG                 VM SIZE                DATE       VM CLOCK
>>>>       1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
>>>>       2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000
>>>>
>>>> Care to guess which one we get when we ask for "2"?
>>>>
>>>> I think we want separate, unoverloaded arguments for QMP.
>>> I think there is no need to. Name is now absolutely mandatory.
>>> Thus for new snapshots we will have 'name' specified and we
>>> will be bound to name only.
>>>
>>> 'id' will be used for old VMs and this is convenience
>>> layer to make old 'id' only snaphosts accessible
>>> through new interface in the same way as old.
>> The overloaded interface you propose is more complex than it seems.  You
>> hide the complexity by not documenting its workings.  Not even to the
>> (insufficient!) degree the HMP interface documents how its overloaded
>> name parameters work.
>>
>> Merely copying over the HMP documentation won't cut it.  The bar for new
>> QMP interfaces is a fair bit higher than "no worse than HMP".  The new
>> interface must reasonably sane for *QMP*, and sufficiently documented.
>>
>> If we can't make a sane QMP interface, I'd rather have no QMP interface.
>> However, I believe we *can* make a sane QMP interface if we put in the
>> design work.
>>
>> The design work must start with a review of what we're trying to
>> accomplish, and how to fit it into the rest of the system.  Here's my
>> attempt.  Since my knowledge on snapshots is rather superficial, I'm
>> cc'ing Kevin for additional snapshot expertise.  Kevin, please correct
>> me when I talk nonsense.  I'm further cc'ing Eric and Peter for the
>> management layer perspective.
>>
>> A point-in-time snapshot of a system consists of a snapshot of its
>> machine state and snapshots of its storage.  All the snapshots need to
>> be made at the same point in time for the result to be consistent.
>>
>> Snapshots of read-only storage carry no information and are commonly
>> omitted.
>>
>> Isolated storage snapshots can make sense, but snapshotting the machine
>> state without also snapshotting the machine's storage doesn't sound
>> useful to me.
>>
>> Both storage and machine state snapshots come in two flavours: internal
>> and external.
>>
>> External ones can be made with any block backend, but internal storage
>> snapshots work only with certain formats, notably qcow2.  QMP supports
>> both kinds of storage snapshots.
>>
>> Both kinds of storage snapshots need exclusive access while they work.
>> They're relatively quick, but the delay could be noticable for large
>> internal snapshots, and perhaps for external snapshots on really slow
>> storage.
>>
>> Internal machine state snapshots are currently only available via HMP's
>> savevm, which integrates internal machine state and storage snapshots.
>> This is non-live, i.e. the guest is stopped while the snapshot gets
>> saved.  I figure we could make it live if we really wanted to.  Another
>> instance of the emerging background job concept.
>>
>> On the implementation level, QCOW2 can't currently store a machine state
>> snapshot without also storing a storage snapshot.  I guess we could
>> change this if we really wanted to.
>>
>> External machine state snapshots are basically migrate to file.
>> Supported by QMP.
>>
>> Live migration to file is possible, but currently wastes space, because
>> memory dirtied during migration gets saved multiple times.  Could be
>> fixed either by making migration update previously saved memory instead
>> of appending (beware, random I/O), or by compacting the file afterwards.
>>
>> Non-live migration to file doesn't waste space that way.
>>
>> To take multiple *consistent* snapshots, you have to bundle them up in a
>> transaction.  Transactions currently support only *storage* snapshots,
>> though.
>>
>> Work-around for external machine state snapshot: migrate to file
>> (possibly live), leaving the guest sopped on completion, take storage
>> snapshots, resume guest.
>>
>> You can combine internal and external storage snapshots with an external
>> machine state snapshot to get a mixed system snapshot.
>>
>> You currently can't do that with an internal machine state snapshot: the
>> only way to take one is HMP savevm, which insists on internally
>> snapshotting all writable storage, and doesn't transact together with
>> external storage snapshots.
>>
>> Except for the case "purely internal snapshot with just one writable
>> storage device", a system snapshot consists of multiple parts stored in
>> separate files.  Tying the parts together is a management problem.  QEMU
>> provides rudimentary management for purely internal snapshots, but it's
>> flawed: missing storage isn't detected, and additional storage can creep
>> in if snapshot tags or IDs happen to match.  I guess managing the parts
>> is better left to the management layer.
>>
>> I figure a fully general QMP interface would let you perform a system
>> snapshot by combining storage snapshots of either kind with either kind
>> of machine state snapshot.
>>
>> We already have most of the building blocks: we can take external and
>> internal storage snapshots, and combine them in transactions.
>>
>> What's missing is transactionable machine state snapshots.
>>
>> We know how to work around it for external machine state snapshots (see
>> above), but a transaction-based solution might be nicer.
>>
>> Any solution for internal machine state snapshots in QMP should at least
>> try to fit into this.  Some restrictions are okay.  For instance, we
>> probably need to restrict internal machine state snapshots to piggyback
>> on an internal storage snapshot for now, so we don't have to dig up
>> QCOW2 just to get QMP support.
>>
>> We can talk about more convenient interfaces for common special cases,
>> but I feel we need to design for the general case.  We don't have to
>> implement the completely general case right away, though.  As long as we
>> know where we want to go, incremental steps towards that goal are fine.
>>
>> Can we create a transactionable QMP command to take an internal machine
>> state snapshot?
>>
>> This would be like HMP savevm with the following differences:
>>
>> * Separate parameters for tag and ID.  I'll have none of this
>>    overloading nonsense in QMP.
> why do we need both 'name' and 'id' values in the future at all?
> Why single descriptive parameter is not enough? From my point
> of view all this overloading is non-sense and in future one
> name parameter should be kept.

Ever since we support multiple snapshots (commit faea38e, Aug 2006), a
snapshot has both an ID and a tag.  We can debate whether that's a good
idea, but we can't go back in time and change the image formats.

Monitor commands and qemu-img let you identify a snapshot by ID or by
tag.  They generally use a single, overloaded parameter, and match it
against both.  Exception: QMP blockdev-snapshot-delete-internal-sync has
two unoverloaded parameters, to avoid ambiguity.  Quoting commit
44e3e05:

    This interface use id and name as optional parameters, to handle the
    case that one image contain multiple snapshots with same name which
    may be '', but with different id.
    
    Adding parameter id is for historical compatiability reason, and
    that case is not possible in qemu's new interface for internal
    snapshot at block device level, but still possible in qemu-img.

Are you proposing to ditch the ability to identify snapshots by ID?  I
doubt we can.

Since HMP commands should be build on top of QMP commands, HMP savevm's
underlying QMP commands cannot be less general than HMP savevm.  HMP
savevm can do both ID and tag.  Therefore, the underlying QMP commands
must be able to do that, too.

Once we accept that, the only question is whether to continue to do that
with a single overloaded parameter, or two unambigous ones.  For QMP, my
answer is unambigous.

>> * Specify the destination block device.  I'll have none of this "pick a
>>    device in some magic, undocumented way" in QMP.
> agreed
>
>> * Leave alone the other block devices.  Adding transaction actions to
>>    snapshot all the writable block devices to get a full system snapshot
>>    is the user's responsibility.
> this requires clarification:
> - do you mean that the list of the devices should come from the
> management layer?

See below.

> If so this is error prone at the restoration stage. From my point of
> view restoration
> must check that all RW devices are restored properly and there are no missed
> ones.

Agreed.

>       Do you propose to put this into libvirt? From my point of view
> this flexibility
> is not that necessary.

VM snapshot / restore has much in common with migration: both save and
restore machine state.  Another similarity is that you have to ensure
identical block device contents.  For migration, this is the management
layer's job.  My point is: the management layer is already prepared to
track block device contents.

For the special case all-internal snapshot / restore, QEMU provides
convenience features:

* You can take a consistent, complete, all-internal snapshot with
  savevm.

* You can restore all parts of an all-internal snapshot with loadvm.

* You can delete all parts of an all-internal snapshot with delvm.

Except "all parts" is a lie: if you manage to lose a block device since
savevm, QEMU won't notice.  Ensuring you still have them all is left to
the management layer even there.

Once external snapshots come into play, QEMU doesn't even try.  It has
no idea where to find external snapshots.  It's completely left to the
management layer.

I'm not fundamentally opposed to convenience features in HMP or even
QMP.  But I don't want to start with them.  I want us to get the core
functionality right before we add convenience.

I wrote:

    A point-in-time snapshot of a system consists of a snapshot of its
    machine state and snapshots of its storage.  All the snapshots need
    to be made at the same point in time for the result to be
    consistent.

and

    I figure a fully general QMP interface would let you perform a
    system snapshot by combining storage snapshots of either kind with
    either kind of machine state snapshot.

where "either kind" is external or internal.

If we decide that we'll never need that much flexibility, we can discuss
restrictions, and how to design a restricted interface.

For instance, if we decide that we'll never need to mix external and
internal in the same snapshot, we can discuss how an the all-internal
and all-external interfaces could look like.

I'm not the person to decide how much flexibility we'll need.  I'm happy
to listen to advice.  I'm hoping the management layer guys I cc'ed can
provide some.

> There is one important thing missed in your description.
> Machine snapshot MUST handle configuration change. This means
> that at the moment of snapshot VM can have different amount of
> disks and other peripherals and this should be properly handled.

I'm not sure I understand.

Configuration changes between which two points of time?  savevm and
loadvm?

How should configuraton change be handled?

>> Limitations:
>>
>> * No live internal machine snapshot, yet.
>>
>> * The storage device taking the internal snapshot must also be
>>    internally snapshot for now.  In fact, the command does both
>>    (tolerable wart).
>>
>> Open questions:
>>
>> * Do we want the QMP command to delete existing snapshots with
>>    conflicting tag / ID, like HMP savevm does?  Or do we want it to fail
>>    the transaction?
>>
>> * Do we need transactions for switching to a system snapshot, too?
>>
>> Opinions?
>>
>> Denis, I understand doing this right will be more work than simply
>> rebadging HMP's savevm etc for QMP, and probably a good deal more than
>> you anticipated.  Sorry about that.  I'm trying to accomodate you as far
>> as possible without screwing up QMP.  Not screwing up QMP too much is my
>> job as maintainer.
> this is not a problem if we will be able to do things with less
> latency. From my side I am a bit frustrated that this discussion
> comes with v5 of patches after two months of waiting to be
> merged assuming that most of previous discussions were about
> some mistakes in the code.

You're absolutely right to complain.  We *failed* to review your patches
in a timely manner.  And "we" very much includes "me".

Reasons include:

* I'm the QMP maintainer, but I'm chronically overloaded with review.
  It's been particularly bad since summer, because I'm also the QAPI
  maintainer, and Eric has been keeping me very busy there.

* I did a quick review of v1 (posted Nov 16, reviewed Nov 17), but
  utterly failed to see the bigger picture: external snapshots,
  migration and transactions.  Bad job on my part.

* Unlucky timing on your part: you posted v2 on Dec 4 during the
  pre-release crunch and general end-of-year crunch, and v3+v4 on Jan 8
  right into my after-Christmas mail deluge.  When I found some time for
  your series again, it was at v5 already.

* v1 and v2 got were reviewed by Eric and Juan, but they also failed to
  see the bigger picture.  I don't blame them.  You can competently
  review code with just local knowledge, but proper interface review
  often requires a more global view, which few people have.

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

[Denis V. Lunev]

On 01/22/2016 06:31 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> On 01/19/2016 09:11 PM, Markus Armbruster wrote:
>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>
>>>> On 01/18/2016 06:58 PM, Markus Armbruster wrote:
>>>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>>>
>>>>>> 'name' attribute is made mandatory in distinction with HMP command.
>>>>>>
>>>>>> The patch also moves hmp_savevm implementation into hmp.c. This function
>>>>>> is just a simple wrapper now and does not have knowledge about
>>>>>> migration internals.
>>> [...]
>>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>>> index 2e31733..09d1a1a 100644
>>>>>> --- a/qapi-schema.json
>>>>>> +++ b/qapi-schema.json
>>>>>> @@ -4054,3 +4054,16 @@
>>>>>>     ##
>>>>>>     { 'enum': 'ReplayMode',
>>>>>>       'data': [ 'none', 'record', 'play' ] }
>>>>>> +
>>>>>> +##
>>>>>> +# @savevm
>>>>>> +#
>>>>>> +# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>>>>> +#
>>>>>> +# @name: identifier of a snapshot to be created
>>>>>> +#
>>>>>> +# Returns: Nothing on success
>>>>>> +#
>>>>>> +# Since 2.6
>>>>>> +##
>>>>>> +{ 'command': 'savevm', 'data': {'name': 'str'} }
>>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>>> index db072a6..b7851e1 100644
>>>>>> --- a/qmp-commands.hx
>>>>>> +++ b/qmp-commands.hx
>>>>>> @@ -4795,3 +4795,28 @@ Example:
>>>>>>                      {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>>>>>                       "pop-vlan": 1, "id": 251658240}
>>>>>>        ]}
>>>>>> +
>>>>>> +EQMP
>>>>>> +
>>>>>> +SQMP
>>>>>> +savevm
>>>>>> +------
>>>>>> +
>>>>>> +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
>>>>>> +
>>>>>> +Arguments:
>>>>>> +
>>>>>> +- "name": snapshot name
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
>>>>>> +<- { "return": {} }
>>>>>> +
>>>>>> +EQMP
>>>>>> +
>>>>>> +    {
>>>>>> +        .name       = "savevm",
>>>>>> +        .args_type  = "name:s",
>>>>>> +        .mhandler.cmd_new = qmp_marshal_savevm,
>>>>>> +    },
>>>>> A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
>>>>> (QEMUSnapshotInfo member id_str).
>>>>>
>>>>> HMP's name arguments are overloaded: they're matched both against tag
>>>>> and ID.  Unwisely chosen tags can create ambiguity.  Example:
>>>>>
>>>>>        (qemu) savevm 2
>>>>>        (qemu) savevm
>>>>>        (qemu) info snapshots
>>>>>        ID        TAG                 VM SIZE                DATE       VM CLOCK
>>>>>        1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
>>>>>        2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000
>>>>>
>>>>> Care to guess which one we get when we ask for "2"?
>>>>>
>>>>> I think we want separate, unoverloaded arguments for QMP.
>>>> I think there is no need to. Name is now absolutely mandatory.
>>>> Thus for new snapshots we will have 'name' specified and we
>>>> will be bound to name only.
>>>>
>>>> 'id' will be used for old VMs and this is convenience
>>>> layer to make old 'id' only snaphosts accessible
>>>> through new interface in the same way as old.
>>> The overloaded interface you propose is more complex than it seems.  You
>>> hide the complexity by not documenting its workings.  Not even to the
>>> (insufficient!) degree the HMP interface documents how its overloaded
>>> name parameters work.
>>>
>>> Merely copying over the HMP documentation won't cut it.  The bar for new
>>> QMP interfaces is a fair bit higher than "no worse than HMP".  The new
>>> interface must reasonably sane for *QMP*, and sufficiently documented.
>>>
>>> If we can't make a sane QMP interface, I'd rather have no QMP interface.
>>> However, I believe we *can* make a sane QMP interface if we put in the
>>> design work.
>>>
>>> The design work must start with a review of what we're trying to
>>> accomplish, and how to fit it into the rest of the system.  Here's my
>>> attempt.  Since my knowledge on snapshots is rather superficial, I'm
>>> cc'ing Kevin for additional snapshot expertise.  Kevin, please correct
>>> me when I talk nonsense.  I'm further cc'ing Eric and Peter for the
>>> management layer perspective.
>>>
>>> A point-in-time snapshot of a system consists of a snapshot of its
>>> machine state and snapshots of its storage.  All the snapshots need to
>>> be made at the same point in time for the result to be consistent.
>>>
>>> Snapshots of read-only storage carry no information and are commonly
>>> omitted.
>>>
>>> Isolated storage snapshots can make sense, but snapshotting the machine
>>> state without also snapshotting the machine's storage doesn't sound
>>> useful to me.
>>>
>>> Both storage and machine state snapshots come in two flavours: internal
>>> and external.
>>>
>>> External ones can be made with any block backend, but internal storage
>>> snapshots work only with certain formats, notably qcow2.  QMP supports
>>> both kinds of storage snapshots.
>>>
>>> Both kinds of storage snapshots need exclusive access while they work.
>>> They're relatively quick, but the delay could be noticable for large
>>> internal snapshots, and perhaps for external snapshots on really slow
>>> storage.
>>>
>>> Internal machine state snapshots are currently only available via HMP's
>>> savevm, which integrates internal machine state and storage snapshots.
>>> This is non-live, i.e. the guest is stopped while the snapshot gets
>>> saved.  I figure we could make it live if we really wanted to.  Another
>>> instance of the emerging background job concept.
>>>
>>> On the implementation level, QCOW2 can't currently store a machine state
>>> snapshot without also storing a storage snapshot.  I guess we could
>>> change this if we really wanted to.
>>>
>>> External machine state snapshots are basically migrate to file.
>>> Supported by QMP.
>>>
>>> Live migration to file is possible, but currently wastes space, because
>>> memory dirtied during migration gets saved multiple times.  Could be
>>> fixed either by making migration update previously saved memory instead
>>> of appending (beware, random I/O), or by compacting the file afterwards.
>>>
>>> Non-live migration to file doesn't waste space that way.
>>>
>>> To take multiple *consistent* snapshots, you have to bundle them up in a
>>> transaction.  Transactions currently support only *storage* snapshots,
>>> though.
>>>
>>> Work-around for external machine state snapshot: migrate to file
>>> (possibly live), leaving the guest sopped on completion, take storage
>>> snapshots, resume guest.
>>>
>>> You can combine internal and external storage snapshots with an external
>>> machine state snapshot to get a mixed system snapshot.
>>>
>>> You currently can't do that with an internal machine state snapshot: the
>>> only way to take one is HMP savevm, which insists on internally
>>> snapshotting all writable storage, and doesn't transact together with
>>> external storage snapshots.
>>>
>>> Except for the case "purely internal snapshot with just one writable
>>> storage device", a system snapshot consists of multiple parts stored in
>>> separate files.  Tying the parts together is a management problem.  QEMU
>>> provides rudimentary management for purely internal snapshots, but it's
>>> flawed: missing storage isn't detected, and additional storage can creep
>>> in if snapshot tags or IDs happen to match.  I guess managing the parts
>>> is better left to the management layer.
>>>
>>> I figure a fully general QMP interface would let you perform a system
>>> snapshot by combining storage snapshots of either kind with either kind
>>> of machine state snapshot.
>>>
>>> We already have most of the building blocks: we can take external and
>>> internal storage snapshots, and combine them in transactions.
>>>
>>> What's missing is transactionable machine state snapshots.
>>>
>>> We know how to work around it for external machine state snapshots (see
>>> above), but a transaction-based solution might be nicer.
>>>
>>> Any solution for internal machine state snapshots in QMP should at least
>>> try to fit into this.  Some restrictions are okay.  For instance, we
>>> probably need to restrict internal machine state snapshots to piggyback
>>> on an internal storage snapshot for now, so we don't have to dig up
>>> QCOW2 just to get QMP support.
>>>
>>> We can talk about more convenient interfaces for common special cases,
>>> but I feel we need to design for the general case.  We don't have to
>>> implement the completely general case right away, though.  As long as we
>>> know where we want to go, incremental steps towards that goal are fine.
>>>
>>> Can we create a transactionable QMP command to take an internal machine
>>> state snapshot?
>>>
>>> This would be like HMP savevm with the following differences:
>>>
>>> * Separate parameters for tag and ID.  I'll have none of this
>>>     overloading nonsense in QMP.
>> why do we need both 'name' and 'id' values in the future at all?
>> Why single descriptive parameter is not enough? From my point
>> of view all this overloading is non-sense and in future one
>> name parameter should be kept.
> Ever since we support multiple snapshots (commit faea38e, Aug 2006), a
> snapshot has both an ID and a tag.  We can debate whether that's a good
> idea, but we can't go back in time and change the image formats.
>
> Monitor commands and qemu-img let you identify a snapshot by ID or by
> tag.  They generally use a single, overloaded parameter, and match it
> against both.  Exception: QMP blockdev-snapshot-delete-internal-sync has
> two unoverloaded parameters, to avoid ambiguity.  Quoting commit
> 44e3e05:
>
>      This interface use id and name as optional parameters, to handle the
>      case that one image contain multiple snapshots with same name which
>      may be '', but with different id.
>      
>      Adding parameter id is for historical compatiability reason, and
>      that case is not possible in qemu's new interface for internal
>      snapshot at block device level, but still possible in qemu-img.
>
> Are you proposing to ditch the ability to identify snapshots by ID?  I
> doubt we can.
>
> Since HMP commands should be build on top of QMP commands, HMP savevm's
> underlying QMP commands cannot be less general than HMP savevm.  HMP
> savevm can do both ID and tag.  Therefore, the underlying QMP commands
> must be able to do that, too.
>
> Once we accept that, the only question is whether to continue to do that
> with a single overloaded parameter, or two unambigous ones.  For QMP, my
> answer is unambigous.
thank you for an extensive explanation. This sounds fair for me.


>>> * Specify the destination block device.  I'll have none of this "pick a
>>>     device in some magic, undocumented way" in QMP.
>> agreed
>>
>>> * Leave alone the other block devices.  Adding transaction actions to
>>>     snapshot all the writable block devices to get a full system snapshot
>>>     is the user's responsibility.
>> this requires clarification:
>> - do you mean that the list of the devices should come from the
>> management layer?
> See below.
>
>> If so this is error prone at the restoration stage. From my point of
>> view restoration
>> must check that all RW devices are restored properly and there are no missed
>> ones.
> Agreed.
>
>>        Do you propose to put this into libvirt? From my point of view
>> this flexibility
>> is not that necessary.
> VM snapshot / restore has much in common with migration: both save and
> restore machine state.  Another similarity is that you have to ensure
> identical block device contents.  For migration, this is the management
> layer's job.  My point is: the management layer is already prepared to
> track block device contents.
no. I am speaking about a little different thing.

At the moment (1) VM is configured to have 2 disks, floppy and 2 network 
cards.
Internal snapshot is made with this configuration with running QEMU. 
After that the
user has stopped QEMU, removed 1 disk, floppy and 1 network card and has 
started
QEMU back. Now we are in a bad shape with loadvm: devices configuration is
completely different. This should be handled gracefully on both layers. 
Either QEMU
should be stopped and started back or configuration change should be applied
in the middle.

> For the special case all-internal snapshot / restore, QEMU provides
> convenience features:
>
> * You can take a consistent, complete, all-internal snapshot with
>    savevm.
>
> * You can restore all parts of an all-internal snapshot with loadvm.
>
> * You can delete all parts of an all-internal snapshot with delvm.
>
> Except "all parts" is a lie: if you manage to lose a block device since
> savevm, QEMU won't notice.  Ensuring you still have them all is left to
> the management layer even there.
>
> Once external snapshots come into play, QEMU doesn't even try.  It has
> no idea where to find external snapshots.  It's completely left to the
> management layer.
>
> I'm not fundamentally opposed to convenience features in HMP or even
> QMP.  But I don't want to start with them.  I want us to get the core
> functionality right before we add convenience.
>
> I wrote:
>
>      A point-in-time snapshot of a system consists of a snapshot of its
>      machine state and snapshots of its storage.  All the snapshots need
>      to be made at the same point in time for the result to be
>      consistent.
>
> and
>
>      I figure a fully general QMP interface would let you perform a
>      system snapshot by combining storage snapshots of either kind with
>      either kind of machine state snapshot.
>
> where "either kind" is external or internal.
fair enough with note about configuration. Though we could make a check and
report proper error code to management layer if configurations with the 
current
moment and previous moment are inconsistent to force management layer to
restart.

> If we decide that we'll never need that much flexibility, we can discuss
> restrictions, and how to design a restricted interface.
>
> For instance, if we decide that we'll never need to mix external and
> internal in the same snapshot, we can discuss how an the all-internal
> and all-external interfaces could look like.
>
> I'm not the person to decide how much flexibility we'll need.  I'm happy
> to listen to advice.  I'm hoping the management layer guys I cc'ed can
> provide some.
>
>> There is one important thing missed in your description.
>> Machine snapshot MUST handle configuration change. This means
>> that at the moment of snapshot VM can have different amount of
>> disks and other peripherals and this should be properly handled.
> I'm not sure I understand.
>
> Configuration changes between which two points of time?  savevm and
> loadvm?
>
> How should configuraton change be handled?
I have written a bit about this above

>>> Limitations:
>>>
>>> * No live internal machine snapshot, yet.
>>>
>>> * The storage device taking the internal snapshot must also be
>>>     internally snapshot for now.  In fact, the command does both
>>>     (tolerable wart).
>>>
>>> Open questions:
>>>
>>> * Do we want the QMP command to delete existing snapshots with
>>>     conflicting tag / ID, like HMP savevm does?  Or do we want it to fail
>>>     the transaction?
>>>
>>> * Do we need transactions for switching to a system snapshot, too?
>>>
>>> Opinions?
>>>
>>> Denis, I understand doing this right will be more work than simply
>>> rebadging HMP's savevm etc for QMP, and probably a good deal more than
>>> you anticipated.  Sorry about that.  I'm trying to accomodate you as far
>>> as possible without screwing up QMP.  Not screwing up QMP too much is my
>>> job as maintainer.
>> this is not a problem if we will be able to do things with less
>> latency. From my side I am a bit frustrated that this discussion
>> comes with v5 of patches after two months of waiting to be
>> merged assuming that most of previous discussions were about
>> some mistakes in the code.
> You're absolutely right to complain.  We *failed* to review your patches
> in a timely manner.  And "we" very much includes "me".
>
> Reasons include:
>
> * I'm the QMP maintainer, but I'm chronically overloaded with review.
>    It's been particularly bad since summer, because I'm also the QAPI
>    maintainer, and Eric has been keeping me very busy there.
>
> * I did a quick review of v1 (posted Nov 16, reviewed Nov 17), but
>    utterly failed to see the bigger picture: external snapshots,
>    migration and transactions.  Bad job on my part.
>
> * Unlucky timing on your part: you posted v2 on Dec 4 during the
>    pre-release crunch and general end-of-year crunch, and v3+v4 on Jan 8
>    right into my after-Christmas mail deluge.  When I found some time for
>    your series again, it was at v5 already.
>
> * v1 and v2 got were reviewed by Eric and Juan, but they also failed to
>    see the bigger picture.  I don't blame them.  You can competently
>    review code with just local knowledge, but proper interface review
>    often requires a more global view, which few people have.
no problem actually. Thank you for a prompt response.

I think that we should start next iteration with API review only (no 
code changes)
and after that I'll start working on the code to simplify your work and 
reduce my.

Is it fair enough?

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 8884 bytes --]

On Tue, Jan 19, 2016 at 19:11:05 +0100, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
> 
> > On 01/18/2016 06:58 PM, Markus Armbruster wrote:
> >> "Denis V. Lunev" <den@openvz.org> writes:

...

> 
> The overloaded interface you propose is more complex than it seems.  You
> hide the complexity by not documenting its workings.  Not even to the
> (insufficient!) degree the HMP interface documents how its overloaded
> name parameters work.
> 
> Merely copying over the HMP documentation won't cut it.  The bar for new
> QMP interfaces is a fair bit higher than "no worse than HMP".  The new
> interface must reasonably sane for *QMP*, and sufficiently documented.
> 
> If we can't make a sane QMP interface, I'd rather have no QMP interface.
> However, I believe we *can* make a sane QMP interface if we put in the
> design work.

I have to agree on this one. A broken interface will introduce just more
pain in the long run. Management tools still can fall back to the old
command if they need to (granted that error handling sucks there though
.. [1]) and use it as previously.

[1]
http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=4ac14cde9ae925515009400c2186d7eec5478b05

> 
> The design work must start with a review of what we're trying to
> accomplish, and how to fit it into the rest of the system.  Here's my
> attempt.  Since my knowledge on snapshots is rather superficial, I'm
> cc'ing Kevin for additional snapshot expertise.  Kevin, please correct
> me when I talk nonsense.  I'm further cc'ing Eric and Peter for the
> management layer perspective.
> 
> A point-in-time snapshot of a system consists of a snapshot of its
> machine state and snapshots of its storage.  All the snapshots need to
> be made at the same point in time for the result to be consistent.
> 
> Snapshots of read-only storage carry no information and are commonly
> omitted.
> 
> Isolated storage snapshots can make sense, but snapshotting the machine
> state without also snapshotting the machine's storage doesn't sound
> useful to me.

Well, technically if all your storage is read-only then a full snapshot
is reduced to a machine state snapshot. Libvirt actually allows this for
external snapshots. For internal ones, it obviously doesn't work.

> 
> Both storage and machine state snapshots come in two flavours: internal
> and external.
> 
> External ones can be made with any block backend, but internal storage
> snapshots work only with certain formats, notably qcow2.  QMP supports
> both kinds of storage snapshots.
> 
> Both kinds of storage snapshots need exclusive access while they work.
> They're relatively quick, but the delay could be noticable for large
> internal snapshots, and perhaps for external snapshots on really slow
> storage.
> 
> Internal machine state snapshots are currently only available via HMP's
> savevm, which integrates internal machine state and storage snapshots.
> This is non-live, i.e. the guest is stopped while the snapshot gets
> saved.  I figure we could make it live if we really wanted to.  Another
> instance of the emerging background job concept.
> 
> On the implementation level, QCOW2 can't currently store a machine state
> snapshot without also storing a storage snapshot.  I guess we could
> change this if we really wanted to.
> 
> External machine state snapshots are basically migrate to file.
> Supported by QMP.
> 
> Live migration to file is possible, but currently wastes space, because
> memory dirtied during migration gets saved multiple times.  Could be
> fixed either by making migration update previously saved memory instead
> of appending (beware, random I/O), or by compacting the file afterwards.
> 
> Non-live migration to file doesn't waste space that way.
> 
> To take multiple *consistent* snapshots, you have to bundle them up in a
> transaction.  Transactions currently support only *storage* snapshots,
> though.
> 
> Work-around for external machine state snapshot: migrate to file
> (possibly live), leaving the guest sopped on completion, take storage
> snapshots, resume guest.

This is exactly what I've implemented in libvirt.

> 
> You can combine internal and external storage snapshots with an external
> machine state snapshot to get a mixed system snapshot.

Libvirt allows this for two separate snapshots of a single vm, but it's
really buggy. You can take such snapshot, but reverting or deleting it
is where the fun begins. In a mixed snapshot you need to track the
backing files along with the internal snapshots in qcow properly and
libvirt currently doesn't really allow that.

> 
> You currently can't do that with an internal machine state snapshot: the
> only way to take one is HMP savevm, which insists on internally
> snapshotting all writable storage, and doesn't transact together with
> external storage snapshots.
> 
> Except for the case "purely internal snapshot with just one writable
> storage device", a system snapshot consists of multiple parts stored in
> separate files.  Tying the parts together is a management problem.  QEMU
> provides rudimentary management for purely internal snapshots, but it's
> flawed: missing storage isn't detected, and additional storage can creep
> in if snapshot tags or IDs happen to match.  I guess managing the parts
> is better left to the management layer.
> 
> I figure a fully general QMP interface would let you perform a system
> snapshot by combining storage snapshots of either kind with either kind
> of machine state snapshot.

That'd be the ideal state. It's questionable though whether combining
internal and external snapshot targets for a single snapshot is useful
or desirable, but having a general interface is probably the best
solution.

> 
> We already have most of the building blocks: we can take external and
> internal storage snapshots, and combine them in transactions.
> 
> What's missing is transactionable machine state snapshots.

That will be totally great. Especially in combination with the ability
to do live snapshot and discard updated storage pages.

> 
> We know how to work around it for external machine state snapshots (see
> above), but a transaction-based solution might be nicer.
> 
> Any solution for internal machine state snapshots in QMP should at least
> try to fit into this.  Some restrictions are okay.  For instance, we
> probably need to restrict internal machine state snapshots to piggyback
> on an internal storage snapshot for now, so we don't have to dig up
> QCOW2 just to get QMP support.

That's fine. Libvirt already mandates at least one QCOW2 storage file
for a VM for an internal snapshot. Allowing us to specify which one will
be used would be great though.

> 
> We can talk about more convenient interfaces for common special cases,
> but I feel we need to design for the general case.  We don't have to
> implement the completely general case right away, though.  As long as we
> know where we want to go, incremental steps towards that goal are fine.
> 
> Can we create a transactionable QMP command to take an internal machine
> state snapshot?
> 
> This would be like HMP savevm with the following differences:
> 
> * Separate parameters for tag and ID.  I'll have none of this
>   overloading nonsense in QMP.
> 
> * Specify the destination block device.  I'll have none of this "pick a
>   device in some magic, undocumented way" in QMP.
> 
> * Leave alone the other block devices.  Adding transaction actions to
>   snapshot all the writable block devices to get a full system snapshot
>   is the user's responsibility.
> 
> Limitations:
> 
> * No live internal machine snapshot, yet.
> 
> * The storage device taking the internal snapshot must also be
>   internally snapshot for now.  In fact, the command does both
>   (tolerable wart).

Having a boolean to switch to live although it will not be implemented
yet would be great. That

> 
> Open questions:
> 
> * Do we want the QMP command to delete existing snapshots with
>   conflicting tag / ID, like HMP savevm does?  Or do we want it to fail
>   the transaction?

For libvirt this doesn't matter that much. When you create all snapshots
using the APIs we make sure that the IDs don't collide. If you tamper
with it externally though, you are on your own.

> 
> * Do we need transactions for switching to a system snapshot, too?
> 
> Opinions?
> 
> Denis, I understand doing this right will be more work than simply
> rebadging HMP's savevm etc for QMP, and probably a good deal more than
> you anticipated.  Sorry about that.  I'm trying to accomodate you as far
> as possible without screwing up QMP.  Not screwing up QMP too much is my
> job as maintainer.

+1

Peter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 7077 bytes --]

On Fri, Jan 22, 2016 at 16:31:04 +0100, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
> > On 01/19/2016 09:11 PM, Markus Armbruster wrote:
> >> "Denis V. Lunev" <den@openvz.org> writes:

[...]

> >> This would be like HMP savevm with the following differences:
> >>
> >> * Separate parameters for tag and ID.  I'll have none of this
> >>    overloading nonsense in QMP.
> > why do we need both 'name' and 'id' values in the future at all?
> > Why single descriptive parameter is not enough? From my point
> > of view all this overloading is non-sense and in future one
> > name parameter should be kept.
> 
> Ever since we support multiple snapshots (commit faea38e, Aug 2006), a
> snapshot has both an ID and a tag.  We can debate whether that's a good
> idea, but we can't go back in time and change the image formats.
> 
> Monitor commands and qemu-img let you identify a snapshot by ID or by
> tag.  They generally use a single, overloaded parameter, and match it
> against both.  Exception: QMP blockdev-snapshot-delete-internal-sync has
> two unoverloaded parameters, to avoid ambiguity.  Quoting commit
> 44e3e05:
> 
>     This interface use id and name as optional parameters, to handle the
>     case that one image contain multiple snapshots with same name which
>     may be '', but with different id.
>     
>     Adding parameter id is for historical compatiability reason, and
>     that case is not possible in qemu's new interface for internal
>     snapshot at block device level, but still possible in qemu-img.
> 
> Are you proposing to ditch the ability to identify snapshots by ID?  I
> doubt we can.

Speaking for libvirt: We don't care, we use the snapshot name as the
identifier, so ID can be killed from our PoV. Backwards compatibility
can be hard though ...

> 
> Since HMP commands should be build on top of QMP commands, HMP savevm's
> underlying QMP commands cannot be less general than HMP savevm.  HMP
> savevm can do both ID and tag.  Therefore, the underlying QMP commands
> must be able to do that, too.
> 
> Once we accept that, the only question is whether to continue to do that
> with a single overloaded parameter, or two unambigous ones.  For QMP, my
> answer is unambigous.
> 
> >> * Specify the destination block device.  I'll have none of this "pick a
> >>    device in some magic, undocumented way" in QMP.
> > agreed
> >
> >> * Leave alone the other block devices.  Adding transaction actions to
> >>    snapshot all the writable block devices to get a full system snapshot
> >>    is the user's responsibility.
> > this requires clarification:
> > - do you mean that the list of the devices should come from the
> > management layer?
> 
> See below.
> 
> > If so this is error prone at the restoration stage. From my point of
> > view restoration
> > must check that all RW devices are restored properly and there are no missed
> > ones.
> 
> Agreed.
> 
> >       Do you propose to put this into libvirt? From my point of view
> > this flexibility
> > is not that necessary.
> 
> VM snapshot / restore has much in common with migration: both save and
> restore machine state.  Another similarity is that you have to ensure
> identical block device contents.  For migration, this is the management
> layer's job.  My point is: the management layer is already prepared to
> track block device contents.
> 
> For the special case all-internal snapshot / restore, QEMU provides
> convenience features:
> 
> * You can take a consistent, complete, all-internal snapshot with
>   savevm.
> 
> * You can restore all parts of an all-internal snapshot with loadvm.
> 
> * You can delete all parts of an all-internal snapshot with delvm.
> 
> Except "all parts" is a lie: if you manage to lose a block device since
> savevm, QEMU won't notice.  Ensuring you still have them all is left to
> the management layer even there.

If you work only on internal snapshots with libvirt we always make sure
that all parts of the 

> 
> Once external snapshots come into play, QEMU doesn't even try.  It has
> no idea where to find external snapshots.  It's completely left to the
> management layer.

This is still mostly buggy/unimplemented in libvirt although the plan is
to do it properly.

> 
> I'm not fundamentally opposed to convenience features in HMP or even
> QMP.  But I don't want to start with them.  I want us to get the core
> functionality right before we add convenience.

This depends on whether anybody would want to use that interface
manually. As we are speaking about QMP it's not really likely that
anyone would need convenience wrappers, since you need a lot of data and
decisions to do such operation properly anyways.

> 
> I wrote:
> 
>     A point-in-time snapshot of a system consists of a snapshot of its
>     machine state and snapshots of its storage.  All the snapshots need
>     to be made at the same point in time for the result to be
>     consistent.
> 
> and
> 
>     I figure a fully general QMP interface would let you perform a
>     system snapshot by combining storage snapshots of either kind with
>     either kind of machine state snapshot.
> 
> where "either kind" is external or internal.
> 
> If we decide that we'll never need that much flexibility, we can discuss
> restrictions, and how to design a restricted interface.
> 
> For instance, if we decide that we'll never need to mix external and
> internal in the same snapshot, we can discuss how an the all-internal
> and all-external interfaces could look like.
> 
> I'm not the person to decide how much flexibility we'll need.  I'm happy
> to listen to advice.  I'm hoping the management layer guys I cc'ed can
> provide some.

This is really hard to decide. Libvirt currently only allows
full-internal or full-external snapshots. Doing a mixed one is not
really the focus. We need to cover mixed trees of snapshots and their
children first. If we do it properly doing combined snapshot should be
trivial then.

I can't imagine how that would be useful though.

> 
> > There is one important thing missed in your description.
> > Machine snapshot MUST handle configuration change. This means
> > that at the moment of snapshot VM can have different amount of
> > disks and other peripherals and this should be properly handled.
> 
> I'm not sure I understand.
> 
> Configuration changes between which two points of time?  savevm and
> loadvm?
> 
> How should configuraton change be handled?

This is handled in libvirt. If you 'savevm' then do some stuff (e.g.
hot-unplug a disk) and then attempt to loadvm, libvirt detects that the
guest ABI would change and kills qemu and restarts it with the config at
the time of 'savevm' along with loading of the snapshot. When we added
external snapshots the approach may be buggy (we don't detect that the
backing file of a disk has changed yet) but the idea of the
infrastructure already exists.

Peter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 6/8] migration, block: better select BDS for VM state loading

[Denis V. Lunev]

On 01/14/2016 02:28 PM, Denis V. Lunev wrote:
> This patch does 2 things:
> - it merges all snapshot validity checks for load_vmstate into one function
> - it now selects BDS to load VM state by availability of the state in the
>    snapshot
>
> This commit is preparatory to allow to select BDS to save snapshot for QMP.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>   block/snapshot.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/snapshot.h |  1 +
>   migration/savevm.c       | 33 ++------------------------
>   3 files changed, 64 insertions(+), 31 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 2d86b88..77affbc 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -489,3 +489,64 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
>       }
>       return bs;
>   }
> +
> +
> +static bool validate_bs(BlockDriverState *bs, BlockDriverState **vmstate_bs,
> +                        const char *name, Error **errp)
> +{
> +    QEMUSnapshotInfo sn;
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +            return true;
> +        }
> +        error_setg(errp,
> +                   "Device '%s' is writable but does not support snapshots",
> +                   bdrv_get_device_name(bs));
> +        return false;
> +    }
> +
> +    if (bdrv_snapshot_find(bs, &sn, name) < 0) {
> +        error_setg(errp,
> +                   "Device '%s' does not have the requested snapshot '%s'",
> +                   bdrv_get_device_name(bs), name);
> +        return false;
> +    }
> +
> +    if (sn.vm_state_size == 0) {
> +        return true;
> +    }
> +    if (*vmstate_bs != NULL) {
> +        error_setg(errp, "Devices '%s' and '%s' both has vmstate with the "
> +                   "requested snapshot '%s'", bdrv_get_device_name(bs),
> +                   bdrv_get_device_name(*vmstate_bs), name);
> +        return false;
> +    }
> +    *vmstate_bs = bs;
> +
> +    return true;
> +}
> +
> +BlockDriverState *bdrv_all_validate_snapshot(const char *name, Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +    BlockDriverState *vmstate_bs = NULL;
> +
> +    while ((bs = bdrv_next(bs))) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +        bool ok;
> +
> +        aio_context_acquire(ctx);
> +        ok = validate_bs(bs, &vmstate_bs, name, errp);
> +        aio_context_release(ctx);
> +
> +        if (!ok) {
> +            return NULL;
> +        }
> +    }
> +
> +    if (vmstate_bs == NULL) {
> +        error_setg(errp, "VM state for the snapshot '%s' not found", name);
> +    }
> +    return vmstate_bs;
> +}
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index c6910da6..a8506e9 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -92,5 +92,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>                                BlockDriverState **first_bad_bs);
>   
>   BlockDriverState *bdrv_all_find_vmstate_bs(void);
> +BlockDriverState *bdrv_all_validate_snapshot(const char *name, Error **errp);
>   
>   #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 6b34017..fed8664 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2022,45 +2022,16 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
>   int load_vmstate(const char *name, Error **errp)
>   {
>       BlockDriverState *bs, *bs_vm_state;
> -    QEMUSnapshotInfo sn;
>       QEMUFile *f;
>       int ret;
>       AioContext *aio_context;
>   
> -    if (!bdrv_all_can_snapshot(&bs)) {
> -        error_setg(errp,
> -                   "Device '%s' is writable but does not support snapshots",
> -                   bdrv_get_device_name(bs));
> -        return -ENOTSUP;
> -    }
> -    ret = bdrv_all_find_snapshot(name, &bs);
> -    if (ret < 0) {
> -        error_setg(errp,
> -                   "Device '%s' does not have the requested snapshot '%s'",
> -                   bdrv_get_device_name(bs), name);
> -        return ret;
> -    }
> -
> -    bs_vm_state = bdrv_all_find_vmstate_bs();
> -    if (!bs_vm_state) {
> -        error_setg(errp, "No block device supports snapshots");
> +    bs_vm_state = bdrv_all_validate_snapshot(name, errp);
> +    if (bs == NULL) {
we should have bs_vm_state here checked...