[Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Qemu-devel] [PATCH v2 for 2.6 0/5] 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.

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>

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 (5):
  migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
  qmp: create qmp_savevm command
  qmp: create qmp_delvm command
  migration: improve error reporting for hmp_loadvm
  qmp: create QMP implementation of loadvm command

 include/sysemu/sysemu.h |   2 +-
 migration/savevm.c      | 100 +++++++++++++++++++++++++++++++-----------------
 monitor.c               |   7 +++-
 qapi-schema.json        |  39 +++++++++++++++++++
 qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
 vl.c                    |   5 ++-
 6 files changed, 185 insertions(+), 39 deletions(-)

[Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_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.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: 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 | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..b7278ac 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)
+static void do_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     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));
+        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 +1944,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);
@@ -1977,22 +1976,20 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     /* 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:
@@ -2002,6 +1999,17 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
+
+    if (local_err != NULL) {
+        error_report_err(local_err);
+    }
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
2.5.0

[Qemu-devel] [PATCH 2/5] 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              | 12 ++++++++++++
 migration/savevm.c | 13 +------------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 25 +++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2140605..c9c7100 100644
--- a/hmp.c
+++ b/hmp.c
@@ -32,6 +32,7 @@
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
+#include "sysemu/sysemu.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -2378,3 +2379,14 @@ 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;
+
+    qmp_savevm(qdict_get_try_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 b7278ac..65e0081 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-static void do_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;
@@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
     }
 }
 
-void hmp_savevm(Monitor *mon, const QDict *qdict)
-{
-    Error *local_err = NULL;
-
-    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
-
-    if (local_err != NULL) {
-        error_report_err(local_err);
-    }
-}
-
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..0114132 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3971,3 +3971,16 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @savevm
+#
+# Save a VM snapshot. Without a name new snapshot is created",
+#
+# @name: identifier of a snapshot to be created
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'savevm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..f216c5e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4739,3 +4739,28 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+SQMP
+savevm
+------------------
+
+Save a VM snapshot. If no tag or id are provided, a new snapshot is created
+
+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/5] 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 c9c7100..a342c8c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2390,3 +2390,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 65e0081..f87a061 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2099,17 +2099,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 0114132..18c9a6c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3984,3 +3984,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 f216c5e..a630e8a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4764,3 +4764,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/5] 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               |  6 +++++-
 vl.c                    |  4 +++-
 4 files changed, 25 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 f87a061..7846437 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2026,7 +2026,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;
@@ -2035,20 +2035,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);
@@ -2058,10 +2060,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, "This is a disk-only snapshot. Revert to it offline "
+                   "using qemu-img.");
         return -EINVAL;
     }
 
@@ -2070,15 +2073,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;
     }
 
@@ -2092,7 +2096,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 9a35d72..3857724 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1739,10 +1739,14 @@ 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);
+    }
+    if (saved_vm_running) {
         vm_start();
     }
 }
diff --git a/vl.c b/vl.c
index 4211ff1..ee34edb 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/5] 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 | 12 ++++++++++++
 monitor.c          | 12 ++++++------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 23 +++++++++++++++++++++++
 4 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 7846437..07b0bf4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2103,6 +2103,18 @@ 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);
+
+    load_vmstate(name, errp);
+
+    if (saved_vm_running) {
+        vm_start();
+    }
+}
+
 void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs;
diff --git a/monitor.c b/monitor.c
index 3857724..aba2c62 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1737,18 +1737,18 @@ 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 (name == NULL) {
+        monitor_printf(mon, "Snapshot name is not set for hmp_loadvm");
+        return;
+    }
 
-    if (load_vmstate(name, &local_err) < 0) {
+    qmp_loadvm(name, &local_err);
+    if (local_err != NULL) {
         error_report_err(local_err);
     }
-    if (saved_vm_running) {
-        vm_start();
-    }
 }
 
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index 18c9a6c..9747df8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3997,3 +3997,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 a630e8a..4dedb62 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4787,3 +4787,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

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/04/2015 05:44 PM, Denis V. Lunev wrote:
> 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.
>
> 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>
>
> 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 (5):
>    migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
>    qmp: create qmp_savevm command
>    qmp: create qmp_delvm command
>    migration: improve error reporting for hmp_loadvm
>    qmp: create QMP implementation of loadvm command
>
>   include/sysemu/sysemu.h |   2 +-
>   migration/savevm.c      | 100 +++++++++++++++++++++++++++++++-----------------
>   monitor.c               |   7 +++-
>   qapi-schema.json        |  39 +++++++++++++++++++
>   qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
>   vl.c                    |   5 ++-
>   6 files changed, 185 insertions(+), 39 deletions(-)
>
ping

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/11/2015 12:33 PM, Denis V. Lunev wrote:
> On 12/04/2015 05:44 PM, Denis V. Lunev wrote:
>> 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.
>>
>> 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>
>>
>> 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 (5):
>>    migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
>>    qmp: create qmp_savevm command
>>    qmp: create qmp_delvm command
>>    migration: improve error reporting for hmp_loadvm
>>    qmp: create QMP implementation of loadvm command
>>
>>   include/sysemu/sysemu.h |   2 +-
>>   migration/savevm.c      | 100 
>> +++++++++++++++++++++++++++++++-----------------
>>   monitor.c               |   7 +++-
>>   qapi-schema.json        |  39 +++++++++++++++++++
>>   qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
>>   vl.c                    |   5 ++-
>>   6 files changed, 185 insertions(+), 39 deletions(-)
>>
> ping
ping v2

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/18/2015 09:10 AM, Denis V. Lunev wrote:
> On 12/11/2015 12:33 PM, Denis V. Lunev wrote:
>> On 12/04/2015 05:44 PM, Denis V. Lunev wrote:
>>> 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.
>>>
>>> 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>
>>>
>>> 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 (5):
>>>    migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
>>>    qmp: create qmp_savevm command
>>>    qmp: create qmp_delvm command
>>>    migration: improve error reporting for hmp_loadvm
>>>    qmp: create QMP implementation of loadvm command
>>>
>>>   include/sysemu/sysemu.h |   2 +-
>>>   migration/savevm.c      | 100 
>>> +++++++++++++++++++++++++++++++-----------------
>>>   monitor.c               |   7 +++-
>>>   qapi-schema.json        |  39 +++++++++++++++++++
>>>   qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
>>>   vl.c                    |   5 ++-
>>>   6 files changed, 185 insertions(+), 39 deletions(-)
>>>
>> ping
> ping v2
guys?

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

[Eric Blake]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> This would be useful in the next step when QMP version of this call will
> be introduced.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: 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 | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 

> @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      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.",

No trailing '.' in error_setg() calls.

> +                   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));
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));

Markus' series to add a prefixing notation would be better to use here
(although I didn't check if he caught this one in that series already):
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html

>  
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;
> +
> +    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> +
> +    if (local_err != NULL) {

I would have just written 'if (local_err) {'; but that's minor style.

Looks like a clean refactoring, other than the nit about the trailing
'.', so with that fixed:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> '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              | 12 ++++++++++++
>  migration/savevm.c | 13 +------------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 25 +++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 2140605..c9c7100 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -32,6 +32,7 @@
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> +#include "sysemu/sysemu.h"

What is this header needed for?

>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -2378,3 +2379,14 @@ 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;

I've been favoring 'err' rather than 'local_err', but that's not a
show-stopper since we still have both spellings mixed in the tree.

> +
> +    qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err);

qdict_get_try_str() can return NULL;, but qmp_savevm() requires a
non-NULL string.  You'll need to invent a name in HMP if the user didn't
give you one.

> +++ b/migration/savevm.c
> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -static void do_savevm(const char *name, Error **errp)
> +void qmp_savevm(const char *name, Error **errp)
>  {

This function still has some 'if (name)' conditionals; but according to
the qapi contract, name should never be NULL. You need to hoist the name
creation aspect to the caller (hmp_savevm), and in qmp_savevm you can
then assert(name).

>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
>      }
>  }
>  
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> -{
> -    Error *local_err = NULL;
> -
> -    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);

Hmm.  You are doing code motion of code you just added in 1/5; wouldn't
it be better if 1/5 stuck it in the right file to begin with?  And my
argument about needing the name logic in HMP (not QMP) means that maybe
the split in 1/5 isn't ideal, after all.

> +++ b/qapi-schema.json
> @@ -3971,3 +3971,16 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @savevm
> +#
> +# Save a VM snapshot. Without a name new snapshot is created",

Stale sentence, since a name is required.

> +#
> +# @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 9d8b42f..f216c5e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4739,3 +4739,28 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +SQMP
> +savevm
> +------------------

Line too long. Make it match the length of the line above.

> +
> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created

Stale sentence, since you are requiring a name.

Why does QMP not allow specifying an id?  Should it?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Denis V. Lunev]

On 12/24/2015 12:40 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
>> '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              | 12 ++++++++++++
>>   migration/savevm.c | 13 +------------
>>   qapi-schema.json   | 13 +++++++++++++
>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>   4 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 2140605..c9c7100 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -32,6 +32,7 @@
>>   #include "ui/console.h"
>>   #include "block/qapi.h"
>>   #include "qemu-io.h"
>> +#include "sysemu/sysemu.h"
> What is this header needed for?
>
>>   
>>   #ifdef CONFIG_SPICE
>>   #include <spice/enums.h>
>> @@ -2378,3 +2379,14 @@ 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;
> I've been favoring 'err' rather than 'local_err', but that's not a
> show-stopper since we still have both spellings mixed in the tree.
>
>> +
>> +    qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> qdict_get_try_str() can return NULL;, but qmp_savevm() requires a
> non-NULL string.  You'll need to invent a name in HMP if the user didn't
> give you one.
>
>> +++ b/migration/savevm.c
>> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>       return ret;
>>   }
>>   
>> -static void do_savevm(const char *name, Error **errp)
>> +void qmp_savevm(const char *name, Error **errp)
>>   {
> This function still has some 'if (name)' conditionals; but according to
> the qapi contract, name should never be NULL. You need to hoist the name
> creation aspect to the caller (hmp_savevm), and in qmp_savevm you can
> then assert(name).
>
>>       BlockDriverState *bs, *bs1;
>>       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
>> @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
>>       }
>>   }
>>   
>> -void hmp_savevm(Monitor *mon, const QDict *qdict)
>> -{
>> -    Error *local_err = NULL;
>> -
>> -    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> Hmm.  You are doing code motion of code you just added in 1/5; wouldn't
> it be better if 1/5 stuck it in the right file to begin with?  And my
> argument about needing the name logic in HMP (not QMP) means that maybe
> the split in 1/5 isn't ideal, after all.
I'll need a header change in 1/5 and subsequent removal of the item here
This was the motto of doing thing this way.


>> +++ b/qapi-schema.json
>> @@ -3971,3 +3971,16 @@
>>   ##
>>   { 'enum': 'ReplayMode',
>>     'data': [ 'none', 'record', 'play' ] }
>> +
>> +##
>> +# @savevm
>> +#
>> +# Save a VM snapshot. Without a name new snapshot is created",
> Stale sentence, since a name is required.
>
>> +#
>> +# @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 9d8b42f..f216c5e 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4739,3 +4739,28 @@ Example:
>>                    {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>                     "pop-vlan": 1, "id": 251658240}
>>      ]}
>> +
>> +EQMP
>> +
>> +SQMP
>> +savevm
>> +------------------
> Line too long. Make it match the length of the line above.
>
>> +
>> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created
> Stale sentence, since you are requiring a name.
>
> Why does QMP not allow specifying an id?  Should it?
>
ok. this is a something to think on.

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

[Eric Blake]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> 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 c9c7100..a342c8c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2390,3 +2390,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;

Same comment as earlier, about 'err' vs. 'local_err' naming,

> +
> +    qmp_delvm(qdict_get_str(qdict, "name"), &local_err);
> +
> +    if (local_err != NULL) {

and redundant comparison to NULL.

> +++ b/migration/savevm.c
> @@ -2099,17 +2099,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;

Instead of renaming 'err' to 'local_err',

> +
> +    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));

you'll want to use Markus' proposed error prefixing code.


> +SQMP
> +delvm
> +------------------

Divider too long.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> 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               |  6 +++++-
>  vl.c                    |  4 +++-
>  4 files changed, 25 insertions(+), 15 deletions(-)
> 

> -int load_vmstate(const char *name)
> +int load_vmstate(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs_vm_state;
>      QEMUSnapshotInfo sn;
> @@ -2035,20 +2035,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.",

No trailing '.'

> @@ -2058,10 +2060,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);

You need '-ret', since error_setg_errno() expects positive errno values.

[maybe, for convenience, we should teach error_setg_errno() to handle
both -EINVAL and EINVAL identically - but it would need good
justification and would touch a lot of the tree, so if we do it, it
would be a separate series]

>          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, "This is a disk-only snapshot. Revert to it offline "
> +                   "using qemu-img.");

According to Markus' recent cleanups, error_setg() should be a single
phrase, not two sentences.  You'll want the second sentence to be added
with error_append_hint():
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03508.html

>          return -EINVAL;
>      }
>  
> @@ -2070,15 +2073,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,

-ret again

> +                         "Error while activating snapshot '%s' on '%s'",
> +                         name, bdrv_get_device_name(bs));

but thanks for getting rid of the weird 'error -5 while...' in the
message :)

> @@ -2092,7 +2096,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");

-ret again

> +++ b/monitor.c
> @@ -1739,10 +1739,14 @@ 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);
> +    }
> +    if (saved_vm_running) {
>          vm_start();

Logic bug.  You blindly fall through, and could end up attempting
vm_start() even after an error, where previously it was not possible.
You probably meant '} else if (saved_vm_running) {'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> 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 | 12 ++++++++++++
>  monitor.c          | 12 ++++++------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 23 +++++++++++++++++++++++
>  4 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7846437..07b0bf4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2103,6 +2103,18 @@ 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);
> +
> +    load_vmstate(name, errp);
> +
> +    if (saved_vm_running) {
> +        vm_start();

Do you really mean to be calling vm_start() even if an error was
reported?  But to properly short-circuit, you need a local 'Error *err =
NULL; load_vmstate(name, &err);', rather than reusing errp (since the
caller may pass in NULL).

> +++ b/monitor.c
> @@ -1737,18 +1737,18 @@ 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 (name == NULL) {

Dead code.  qdict_get_str() crashes rather than returning NULL, if
"name" is not present.

> +++ b/qmp-commands.hx
> @@ -4787,3 +4787,26 @@ EQMP
>          .args_type  = "name:s",
>          .mhandler.cmd_new = qmp_marshal_delvm,
>      },
> +
> +SQMP
> +loadvm
> +------------------

Line too long.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Denis V. Lunev]

On 12/24/2015 12:27 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
>> This would be useful in the next step when QMP version of this call will
>> be introduced.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: 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 | 38 +++++++++++++++++++++++---------------
>>   1 file changed, 23 insertions(+), 15 deletions(-)
>>
>> @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>       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.",
> No trailing '.' in error_setg() calls.
>
>> +                   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));
>> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
>> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
> Markus' series to add a prefixing notation would be better to use here
> (although I didn't check if he caught this one in that series already):
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html

this series is not yet merged. I think that we could do this refactoring 
later on.
This thing could be considered independent. Anyway, this series has its 
own value
and it takes a lot of time to push it in. Could we do  error setting 
improvement later on?

Messages are not changed etc. I'll change them as you suggest.


>>   
>> +void hmp_savevm(Monitor *mon, const QDict *qdict)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
>> +
>> +    if (local_err != NULL) {
> I would have just written 'if (local_err) {'; but that's minor style.
from my point of view explicit != NULL exposes that local_err is a
pointer rather than a boolean value.

> Looks like a clean refactoring, other than the nit about the trailing
> '.', so with that fixed:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

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

[Denis V. Lunev]

On 12/24/2015 12:40 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
>> '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              | 12 ++++++++++++
>>   migration/savevm.c | 13 +------------
>>   qapi-schema.json   | 13 +++++++++++++
>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>   4 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 2140605..c9c7100 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -32,6 +32,7 @@
>>   #include "ui/console.h"
>>   #include "block/qapi.h"
>>   #include "qemu-io.h"
>> +#include "sysemu/sysemu.h"
> What is this header needed for?
>
it contains prototype of hmp_savevm. The following warning
appears without it:

irbis ~/src/qemu $ make -j8
   CC    hmp.o
hmp.c:2390:6: error: no previous prototype for ‘hmp_savevm’ 
[-Werror=missing-prototypes]
  void hmp_savevm(Monitor *mon, const QDict *qdict)
       ^
cc1: all warnings being treated as errors
/home/den/src/qemu/rules.mak:57: recipe for target 'hmp.o' failed
make: *** [hmp.o] Error 1
irbis ~/src/qemu $

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

[Eric Blake]

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

On 01/08/2016 04:27 AM, Denis V. Lunev wrote:

>>>         /* 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));
>>> +        error_setg(errp, "Error while deleting snapshot on device
>>> '%s': %s",
>>> +                   bdrv_get_device_name(bs1),
>>> error_get_pretty(local_err));
>> Markus' series to add a prefixing notation would be better to use here
>> (although I didn't check if he caught this one in that series already):
>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
> 
> this series is not yet merged. I think that we could do this refactoring
> later on.
> This thing could be considered independent. Anyway, this series has its
> own value
> and it takes a lot of time to push it in. Could we do  error setting
> improvement later on?

I don't care who rebases on top of the other, but maybe Markus will have
an opinion when he gets back online next week.


>>> +
>>> +    if (local_err != NULL) {
>> I would have just written 'if (local_err) {'; but that's minor style.
> from my point of view explicit != NULL exposes that local_err is a
> pointer rather than a boolean value.

But the code base already overwhelmingly relies on C's implicit
conversion of pointer to a boolean context, as it requires less typing;
being verbose doesn't make the code base any easier to read.  However,
since HACKING doesn't say one way or the other, I won't make you change.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Denis V. Lunev]

On 01/08/2016 07:14 PM, Eric Blake wrote:
> On 01/08/2016 04:27 AM, Denis V. Lunev wrote:
>
>>>>          /* 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));
>>>> +        error_setg(errp, "Error while deleting snapshot on device
>>>> '%s': %s",
>>>> +                   bdrv_get_device_name(bs1),
>>>> error_get_pretty(local_err));
>>> Markus' series to add a prefixing notation would be better to use here
>>> (although I didn't check if he caught this one in that series already):
>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
>> this series is not yet merged. I think that we could do this refactoring
>> later on.
>> This thing could be considered independent. Anyway, this series has its
>> own value
>> and it takes a lot of time to push it in. Could we do  error setting
>> improvement later on?
> I don't care who rebases on top of the other, but maybe Markus will have
> an opinion when he gets back online next week.
>
why we have to wait with this set due to this reason?
The code with error_prepend and current code are BOTH
correct. One is a bit shorter then other. Yes, it would
be nice to switch to it, but why this should be done in
this set?

This set solves real problem which has not been addressed
for a long time. Let's proceed, cool and shiny stuff
could be done later on, when it will be merged.

Moreover, merging this set will make my life easier
with merging these changes to our downstream.
Fixes will be merged while improvements will stay
in upstream only.


>>>> +
>>>> +    if (local_err != NULL) {
>>> I would have just written 'if (local_err) {'; but that's minor style.
>> from my point of view explicit != NULL exposes that local_err is a
>> pointer rather than a boolean value.
> But the code base already overwhelmingly relies on C's implicit
> conversion of pointer to a boolean context, as it requires less typing;
> being verbose doesn't make the code base any easier to read.  However,
> since HACKING doesn't say one way or the other, I won't make you change.
>
I do not understand your last words.

I am not agitating you with one approach or another. This
is a reason why I am writing code this way. The code written
this way looks better to me. This code is NEW and this does
not contradict any written rule in coding style policy.

If the code is working and correct, can we just move on with it?

Den

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

[Eric Blake]

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

On 01/08/2016 09:40 AM, Denis V. Lunev wrote:

>>>> Markus' series to add a prefixing notation would be better to use here
>>>> (although I didn't check if he caught this one in that series already):
>>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
>>> this series is not yet merged. I think that we could do this refactoring
>>> later on.
>>> This thing could be considered independent. Anyway, this series has its
>>> own value
>>> and it takes a lot of time to push it in. Could we do  error setting
>>> improvement later on?
>> I don't care who rebases on top of the other, but maybe Markus will have
>> an opinion when he gets back online next week.
>>
> why we have to wait with this set due to this reason?

One of you will have to rebase on the other - either you wait for
Markus' error_prepend to go in and you use it, or your patch goes in and
Markus updates his error_prepend patch to cover your additional instance
that will be benefitted by it.  I don't care which, and the timing is
really up to the maintainers and how fast they send pull requests.

> The code with error_prepend and current code are BOTH
> correct. One is a bit shorter then other. Yes, it would
> be nice to switch to it, but why this should be done in
> this set?

Exactly, we're saying the same things.

>>>>> +    if (local_err != NULL) {
>>>> I would have just written 'if (local_err) {'; but that's minor style.
>>> from my point of view explicit != NULL exposes that local_err is a
>>> pointer rather than a boolean value.
>> But the code base already overwhelmingly relies on C's implicit
>> conversion of pointer to a boolean context, as it requires less typing;
>> being verbose doesn't make the code base any easier to read.  However,
>> since HACKING doesn't say one way or the other, I won't make you change.
>>
> I do not understand your last words.
> 
> I am not agitating you with one approach or another. This
> is a reason why I am writing code this way. The code written
> this way looks better to me. This code is NEW and this does
> not contradict any written rule in coding style policy.
> 
> If the code is working and correct, can we just move on with it?

Once again, we are saying the same thing.  I pointed out a cosmetic
issue, but one where I do not have a strong enough leg to stand on to
force you to change your style, so what you did is fine as is.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Denis V. Lunev]

On 01/08/2016 08:54 PM, Eric Blake wrote:
> On 01/08/2016 09:40 AM, Denis V. Lunev wrote:
>
>>>>> Markus' series to add a prefixing notation would be better to use here
>>>>> (although I didn't check if he caught this one in that series already):
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
>>>> this series is not yet merged. I think that we could do this refactoring
>>>> later on.
>>>> This thing could be considered independent. Anyway, this series has its
>>>> own value
>>>> and it takes a lot of time to push it in. Could we do  error setting
>>>> improvement later on?
>>> I don't care who rebases on top of the other, but maybe Markus will have
>>> an opinion when he gets back online next week.
>>>
>> why we have to wait with this set due to this reason?
> One of you will have to rebase on the other - either you wait for
> Markus' error_prepend to go in and you use it, or your patch goes in and
> Markus updates his error_prepend patch to cover your additional instance
> that will be benefitted by it.  I don't care which, and the timing is
> really up to the maintainers and how fast they send pull requests.
>
>> The code with error_prepend and current code are BOTH
>> correct. One is a bit shorter then other. Yes, it would
>> be nice to switch to it, but why this should be done in
>> this set?
> Exactly, we're saying the same things.
>
>>>>>> +    if (local_err != NULL) {
>>>>> I would have just written 'if (local_err) {'; but that's minor style.
>>>> from my point of view explicit != NULL exposes that local_err is a
>>>> pointer rather than a boolean value.
>>> But the code base already overwhelmingly relies on C's implicit
>>> conversion of pointer to a boolean context, as it requires less typing;
>>> being verbose doesn't make the code base any easier to read.  However,
>>> since HACKING doesn't say one way or the other, I won't make you change.
>>>
>> I do not understand your last words.
>>
>> I am not agitating you with one approach or another. This
>> is a reason why I am writing code this way. The code written
>> this way looks better to me. This code is NEW and this does
>> not contradict any written rule in coding style policy.
>>
>> If the code is working and correct, can we just move on with it?
> Once again, we are saying the same thing.  I pointed out a cosmetic
> issue, but one where I do not have a strong enough leg to stand on to
> force you to change your style, so what you did is fine as is.
>
ok. perfect to be on the same page :)

I'll promise to switch to error_prepend code when it will be
merged. I hope that v4 of the set is good enough to
proceed.

Den