[Qemu-devel] [RFC 00/22]: QMP: Convert savevm/loadvm/delvm

[Qemu-devel] [RFC 00/22]: QMP: Convert savevm/loadvm/delvm

[Luiz Capitulino]

 Hi there,

 Libvirt has a new snapshot API which uses savevm, loadvm and delvm, so we
need to convert them to QMP.

 I thought this wouldn't be difficult, but while doing and testing this work
I hit a number of problems and had to made not so easy decisions.

 Most of the time, the problem is that the handler implementation is not
as consistent as it should be. Usually, this doesn't affect users very much
but the headache begins when you have to make this available under QMP.

 Here goes a list of macro issues/decisions. Details (and other small problems)
in the patches.

   1. Multiple failures: do_delvm() and do_savevm() report errors in a
      QTAILQ_FOREACH() loop and they don't return when an error happens.

      Although QMP will end up reporting only the first error, this is
      considered a bug because handlers are not expected to continue
      executing when an error happens.

      There are two solutions:

            1) Return right away in the first error
            2) Identify fatal errors and only report those

      I don't know the implications of doing 1) and don't know how to
      to do 2) (although do_loadvm() works this way).

      Can someone from the block layer help here?

   2. ID vs. TAG: all the three commands accept only one parameter, called
      'name'. This is automagically interpreted as either a TAG name or an
      ID number.

      QMP is machine-like, so it's boring and cold, meaning that this kind
      of automatic stuff is not good. If we want something to be an ID, we
      have to be able to say 'this must be an ID', if it can't, it should
      just fail.

      So, I considered adding new optional parameters 'tag' and 'id', but
      soon I realized that this is not as easy as it seems, as it's also
      needed to change the qcow2 driver plus refactorings..

      Markus, I'm willing to live with 'name', are you ok with that?

   3. Exposing errno macro names: several error functions print errno codes,
      while it's debatable if this is good or not, we just can't create a
      QError for every single possible error.

      So, my solution for this is to expose names instead of codes.

      For example, this error:

        "Error -5 while writing VM"

      Becomes:

        "Failed to save VM state (EIO)"

      Of course this is also available under QMP.

[Qemu-devel] [PATCH 01/22] QMP: Introduce RESUME event

[Luiz Capitulino]

It's emitted when the Virtual Machine resumes execution.

This is needed because next patches will convert savevm/loadvm
to QMP and they call vm_stop() (which causes QMP to emit the
STOP event) and vm_start() (which doesn't emit any event).

Clients will get confused with a STOP event without a matching
RESUME event.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |   12 ++++++++++++
 monitor.c          |    3 +++
 monitor.h          |    1 +
 vl.c               |    1 +
 4 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index c084a47..01ec85f 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -38,6 +38,18 @@ Example:
 { "event": "RESET",
     "timestamp": { "seconds": 1267041653, "microseconds": 9518 } }
 
+RESUME
+------
+
+Emitted when the Virtual Machine resumes execution.
+
+Data: None.
+
+Example:
+
+{ "event": "RESUME",
+    "timestamp": { "seconds": 1271770767, "microseconds": 582542 } }
+
 RTC_CHANGE
 ----------
 
diff --git a/monitor.c b/monitor.c
index afea639..975e77c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -423,6 +423,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_STOP:
             event_name = "STOP";
             break;
+        case QEVENT_RESUME:
+            event_name = "RESUME";
+            break;
         case QEVENT_VNC_CONNECTED:
             event_name = "VNC_CONNECTED";
             break;
diff --git a/monitor.h b/monitor.h
index 5bdeed1..ea15469 100644
--- a/monitor.h
+++ b/monitor.h
@@ -21,6 +21,7 @@ typedef enum MonitorEvent {
     QEVENT_RESET,
     QEVENT_POWERDOWN,
     QEVENT_STOP,
+    QEVENT_RESUME,
     QEVENT_VNC_CONNECTED,
     QEVENT_VNC_INITIALIZED,
     QEVENT_VNC_DISCONNECTED,
diff --git a/vl.c b/vl.c
index b997cb9..3ba1dc6 100644
--- a/vl.c
+++ b/vl.c
@@ -1692,6 +1692,7 @@ void vm_start(void)
         vm_running = 1;
         vm_state_notify(1, 0);
         resume_all_vcpus();
+        monitor_protocol_event(QEVENT_RESUME, NULL);
     }
 }
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 02/22] savevm: Don't check the return of qemu_fopen_bdrv()

[Luiz Capitulino]

It never fails.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/savevm.c b/savevm.c
index 086c92a..254f9fc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1705,10 +1705,6 @@ void do_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");
-        goto the_end;
-    }
     ret = qemu_savevm_state(mon, f);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
@@ -1790,10 +1786,6 @@ int load_vmstate(const char *name)
 
     /* restore the VM state */
     f = qemu_fopen_bdrv(bs, 0);
-    if (!f) {
-        error_report("Could not open VM state file");
-        return -EINVAL;
-    }
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 03/22] savevm: Introduce delete_snapshot() and use it

[Luiz Capitulino]

del_existing_snapshots() and do_delvm() can share some code as
the two call bdrv_snapshot_delete() and print a message to the
user if an error has happened.

This commit introduces a new function to share this code.

Please, note that while do_delvm() should stay the same,
del_existing_snapshots() will now print a specific message for
-ENOTSUP errors (instead of a generic one).

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |   39 ++++++++++++++++++++++++---------------
 1 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/savevm.c b/savevm.c
index 254f9fc..cc6cbb2 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1619,6 +1619,28 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     return ret;
 }
 
+static int delete_snapshot(Monitor *mon, BlockDriverState *bs,
+                           const char *name)
+{
+    int ret;
+
+    ret = bdrv_snapshot_delete(bs, name);
+    if (ret < 0) {
+        switch (ret) {
+        case -ENOTSUP:
+            monitor_printf(mon, "Snapshots not supported on device '%s'\n",
+                           bdrv_get_device_name(bs));
+            break;
+        default:
+            monitor_printf(mon, "Error %d while deleting snapshot on '%s'\n",
+                           ret, bdrv_get_device_name(bs));
+            break;
+        }
+    }
+
+    return ret;
+}
+
 /*
  * Deletes snapshots of a given name in all opened images.
  */
@@ -1634,11 +1656,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
-            ret = bdrv_snapshot_delete(bs, name);
+            ret = delete_snapshot(mon, bs, name);
             if (ret < 0) {
-                monitor_printf(mon,
-                               "Error while deleting snapshot on '%s'\n",
-                               bdrv_get_device_name(bs));
                 return -1;
             }
         }
@@ -1799,7 +1818,6 @@ void do_delvm(Monitor *mon, const QDict *qdict)
 {
     DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
-    int ret;
     const char *name = qdict_get_str(qdict, "name");
 
     bs = get_bs_snapshots();
@@ -1811,16 +1829,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
         if (bdrv_has_snapshot(bs1)) {
-            ret = bdrv_snapshot_delete(bs1, name);
-            if (ret < 0) {
-                if (ret == -ENOTSUP)
-                    monitor_printf(mon,
-                                   "Snapshots not supported on device '%s'\n",
-                                   bdrv_get_device_name(bs1));
-                else
-                    monitor_printf(mon, "Error %d while deleting snapshot on "
-                                   "'%s'\n", ret, bdrv_get_device_name(bs1));
-            }
+            delete_snapshot(mon, bs1, name);
         }
     }
 }
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Luiz Capitulino]

do_loadvm(), which implements the 'loadvm' Monitor command, pauses
the emulation to load the saved VM, however it will only resume
it if the loading succeeds.

In other words, if the user issues 'loadvm' and it fails, the
end result will be an error message and a paused VM.

This seems an undesirable side effect to me because, most of the
time, if a Monitor command fails the best thing we can do is to
leave the VM as it were before the command was executed.

FIXME: This will try to run a potentially corrupted image, the
       solution is to split load_vmstate() in two and only keep
       the VM paused if qemu_loadvm_state() fails.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 975e77c..542c858 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2472,8 +2472,11 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
 
     vm_stop(0);
 
-    if (load_vmstate(name) >= 0 && saved_vm_running)
+    load_vmstate(name);
+
+    if (saved_vm_running) {
         vm_start();
+    }
 }
 
 int monitor_get_fd(Monitor *mon, const char *fdname)
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 05/22] savevm: load_vmstate(): Return 'ret' on error

[Luiz Capitulino]

When looping to restore the snapshot on all drives, load_vmstate()
will return 0 if bdrv_snapshot_goto() returns an error.

This seems a trick to avoid the call to vm_start() in do_loadvm(),
however it brings two problems:

1. The call to load_vmstate() from main() will succeed

2. In QMP, it's just not allowed to fail and return 0

This commit fixes that.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index cc6cbb2..5024829 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1793,7 +1793,7 @@ int load_vmstate(const char *name)
                 }
                 /* fatal on snapshot block device */
                 if (bs == bs1)
-                    return 0;
+                    return ret;
             }
         }
     }
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 06/22] savevm: load_vmstate(): Improve error check

[Luiz Capitulino]

We should fail if something goes wrong in the call to
bdrv_snapshot_find().

Currently, we only fail if 'sn.vm_state_size' is not what
we expect it to be.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 5024829..3ccb246 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1800,8 +1800,11 @@ int load_vmstate(const char *name)
 
     /* Don't even try to load empty VM states */
     ret = bdrv_snapshot_find(bs, &sn, name);
-    if ((ret >= 0) && (sn.vm_state_size == 0))
+    if (ret < 0) {
+        return ret;
+    } else if (sn.vm_state_size == 0) {
         return -EINVAL;
+    }
 
     /* restore the VM state */
     f = qemu_fopen_bdrv(bs, 0);
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Luiz Capitulino]

There are error handling functions in QEMU which print errno codes
to the user. While it's debatable if this is good from a user
perspective, sometimes it's the best you can do because it's what
system calls return and this is also useful for debugging.

So, we need a way to expose those codes in QMP. We can't use the
codes themselfs because they may vary between systems.

The best solution I can think of is returning the string
representation of the name. For example, EIO becomes "EIO".

This is what get_errno_string() does.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-error.c |   25 +++++++++++++++++++++++++
 qemu-error.h |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/qemu-error.c b/qemu-error.c
index 5a35e7c..55ce133 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
     va_end(ap);
     error_printf("\n");
 }
+
+/*
+ * This is probably only useful for QMP
+ */
+const char *get_errno_string(int err)
+{
+    assert(err < 0);
+
+    switch (err) {
+    case -EINVAL:
+        return "EINVAL";
+    case -EIO:
+        return "EIO";
+    case -ENOENT:
+        return "ENOENT";
+    case -ENOMEDIUM:
+        return "ENOMEDIUM";
+    case -ENOTSUP:
+        return "ENOTSUP";
+    default:
+        return "unknown";
+    }
+
+    abort();
+}
diff --git a/qemu-error.h b/qemu-error.h
index a45609f..bf2b890 100644
--- a/qemu-error.h
+++ b/qemu-error.h
@@ -38,4 +38,5 @@ void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
 
+const char *get_errno_string(int err);
 #endif
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 08/22] QError: New QERR_SNAPSHOT_NO_DEVICE

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 034c7de..648d438 100644
--- a/qerror.c
+++ b/qerror.c
@@ -181,6 +181,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_SNAPSHOT_NO_DEVICE,
+        .desc      = "No block device can accept snapshots",
+    },
+    {
         .error_fmt = QERR_TOO_MANY_FILES,
         .desc      = "Too many open files",
     },
diff --git a/qerror.h b/qerror.h
index c98c61a..2f6633c 100644
--- a/qerror.h
+++ b/qerror.h
@@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_SNAPSHOT_NO_DEVICE \
+    "{ 'class': 'SnapshotNoDevice', 'data': {} }"
+
 #define QERR_TOO_MANY_FILES \
     "{ 'class': 'TooManyFiles', 'data': {} }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 09/22] QError: New QERR_SNAPSHOT_DELETE_FAILED

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 648d438..347ff36 100644
--- a/qerror.c
+++ b/qerror.c
@@ -181,6 +181,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_SNAPSHOT_DELETE_FAILED,
+        .desc      = "Failed to delete snapshot in device '%(device)' reason: %(reason)",
+    },
+    {
         .error_fmt = QERR_SNAPSHOT_NO_DEVICE,
         .desc      = "No block device can accept snapshots",
     },
diff --git a/qerror.h b/qerror.h
index 2f6633c..0c0023e 100644
--- a/qerror.h
+++ b/qerror.h
@@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_SNAPSHOT_DELETE_FAILED \
+    "{ 'class': 'SnapshotDeleteFailed', 'data': { 'device': %s, 'reason': %s } }"
+
 #define QERR_SNAPSHOT_NO_DEVICE \
     "{ 'class': 'SnapshotNoDevice', 'data': {} }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 10/22] QError: New QERR_SNAPSHOT_CREATE_FAILED

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 347ff36..e0df25f 100644
--- a/qerror.c
+++ b/qerror.c
@@ -181,6 +181,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_SNAPSHOT_CREATE_FAILED,
+        .desc      = "Failed to create snapshot in device '%(device)' reason: %(reason)",
+    },
+    {
         .error_fmt = QERR_SNAPSHOT_DELETE_FAILED,
         .desc      = "Failed to delete snapshot in device '%(device)' reason: %(reason)",
     },
diff --git a/qerror.h b/qerror.h
index 0c0023e..07ba419 100644
--- a/qerror.h
+++ b/qerror.h
@@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_SNAPSHOT_CREATE_FAILED \
+    "{ 'class': 'SnapshotCreateFailed', 'data': { 'device': %s, 'reason': %s } }"
+
 #define QERR_SNAPSHOT_DELETE_FAILED \
     "{ 'class': 'SnapshotDeleteFailed', 'data': { 'device': %s, 'reason': %s } }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 11/22] QError: New QERR_SNAPSHOT_ACTIVATE_FAILED

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index e0df25f..aa6892d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -181,6 +181,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_SNAPSHOT_ACTIVATE_FAILED,
+        .desc      = "Failed to activate snapshot in device '%(device)' reason: %(reason)",
+    },
+    {
         .error_fmt = QERR_SNAPSHOT_CREATE_FAILED,
         .desc      = "Failed to create snapshot in device '%(device)' reason: %(reason)",
     },
diff --git a/qerror.h b/qerror.h
index 07ba419..4a056b3 100644
--- a/qerror.h
+++ b/qerror.h
@@ -151,6 +151,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_SNAPSHOT_ACTIVATE_FAILED \
+    "{ 'class': 'SnapshotActivateFailed', 'data': { 'device': %s, 'reason': %s } }"
+
 #define QERR_SNAPSHOT_CREATE_FAILED \
     "{ 'class': 'SnapshotCreateFailed', 'data': { 'device': %s, 'reason': %s } }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 12/22] QError: New QERR_STATEVM_SAVE_FAILED

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index aa6892d..95c1a95 100644
--- a/qerror.c
+++ b/qerror.c
@@ -197,6 +197,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "No block device can accept snapshots",
     },
     {
+        .error_fmt = QERR_STATEVM_SAVE_FAILED,
+        .desc      = "Failed to save VM state (%(reason))",
+    },
+    {
         .error_fmt = QERR_TOO_MANY_FILES,
         .desc      = "Too many open files",
     },
diff --git a/qerror.h b/qerror.h
index 4a056b3..b915e26 100644
--- a/qerror.h
+++ b/qerror.h
@@ -163,6 +163,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SNAPSHOT_NO_DEVICE \
     "{ 'class': 'SnapshotNoDevice', 'data': {} }"
 
+#define QERR_STATEVM_SAVE_FAILED \
+    "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': %s } }"
+
 #define QERR_TOO_MANY_FILES \
     "{ 'class': 'TooManyFiles', 'data': {} }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 13/22] QError: New QERR_STATEVM_LOAD_FAILED

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 95c1a95..b0cd7b1 100644
--- a/qerror.c
+++ b/qerror.c
@@ -201,6 +201,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Failed to save VM state (%(reason))",
     },
     {
+        .error_fmt = QERR_STATEVM_LOAD_FAILED,
+        .desc      = "Failed to load VM state (%(reason))",
+    },
+    {
         .error_fmt = QERR_TOO_MANY_FILES,
         .desc      = "Too many open files",
     },
diff --git a/qerror.h b/qerror.h
index b915e26..900a62b 100644
--- a/qerror.h
+++ b/qerror.h
@@ -166,6 +166,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_STATEVM_SAVE_FAILED \
     "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': %s } }"
 
+#define QERR_STATEVM_LOAD_FAILED \
+    "{ 'class': 'StateVmLoadFailed', 'data': { 'reason': %s } }"
+
 #define QERR_TOO_MANY_FILES \
     "{ 'class': 'TooManyFiles', 'data': {} }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 14/22] QError: New QERR_DEVICE_NO_SNAPSHOT

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index b0cd7b1..078fc43 100644
--- a/qerror.c
+++ b/qerror.c
@@ -101,6 +101,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' has no child bus",
     },
     {
+        .error_fmt = QERR_DEVICE_NO_SNAPSHOT,
+        .desc      = "Device '%(device)' doesn't support snapshots",
+    },
+    {
         .error_fmt = QERR_DUPLICATE_ID,
         .desc      = "Duplicate ID '%(id)' for %(object)",
     },
diff --git a/qerror.h b/qerror.h
index 900a62b..da12160 100644
--- a/qerror.h
+++ b/qerror.h
@@ -91,6 +91,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_NO_BUS \
     "{ 'class': 'DeviceNoBus', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_NO_SNAPSHOT \
+    "{ 'class': 'DeviceNoSnapshot', 'data': { 'device': %s } }"
+
 #define QERR_DUPLICATE_ID \
     "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 15/22] QError: New QERR_SNAPSHOT_NOT_FOUND

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 078fc43..c8e9393 100644
--- a/qerror.c
+++ b/qerror.c
@@ -105,6 +105,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' doesn't support snapshots",
     },
     {
+        .error_fmt = QERR_SNAPSHOT_NOT_FOUND,
+        .desc      = "Could not find snapshot '%(name)' in device '%(device)'",
+    },
+    {
         .error_fmt = QERR_DUPLICATE_ID,
         .desc      = "Duplicate ID '%(id)' for %(object)",
     },
diff --git a/qerror.h b/qerror.h
index da12160..bf8af24 100644
--- a/qerror.h
+++ b/qerror.h
@@ -166,6 +166,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SNAPSHOT_NO_DEVICE \
     "{ 'class': 'SnapshotNoDevice', 'data': {} }"
 
+#define QERR_SNAPSHOT_NOT_FOUND \
+    "{ 'class': 'SnapshotNotFound', 'data': { 'device': %s, 'name': %s } }"
+
 #define QERR_STATEVM_SAVE_FAILED \
     "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': %s } }"
 
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 16/22] savevm: Convert delete_snapshot() to QError

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/savevm.c b/savevm.c
index 3ccb246..55cf848 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1628,12 +1628,11 @@ static int delete_snapshot(Monitor *mon, BlockDriverState *bs,
     if (ret < 0) {
         switch (ret) {
         case -ENOTSUP:
-            monitor_printf(mon, "Snapshots not supported on device '%s'\n",
-                           bdrv_get_device_name(bs));
+            qerror_report(QERR_DEVICE_NO_SNAPSHOT, bdrv_get_device_name(bs));
             break;
         default:
-            monitor_printf(mon, "Error %d while deleting snapshot on '%s'\n",
-                           ret, bdrv_get_device_name(bs));
+            qerror_report(QERR_SNAPSHOT_DELETE_FAILED, bdrv_get_device_name(bs),
+                          get_errno_string(ret));
             break;
         }
     }
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 17/22] savevm: delete_snapshot(): Remove unused parameter

[Luiz Capitulino]

The parameter 'mon' is unused after the conversion to
QError, done by the previous commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/savevm.c b/savevm.c
index 55cf848..643273e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1619,8 +1619,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     return ret;
 }
 
-static int delete_snapshot(Monitor *mon, BlockDriverState *bs,
-                           const char *name)
+static int delete_snapshot(BlockDriverState *bs, const char *name)
 {
     int ret;
 
@@ -1643,7 +1642,7 @@ static int delete_snapshot(Monitor *mon, BlockDriverState *bs,
 /*
  * Deletes snapshots of a given name in all opened images.
  */
-static int del_existing_snapshots(Monitor *mon, const char *name)
+static int del_existing_snapshots(const char *name)
 {
     BlockDriverState *bs;
     DriveInfo *dinfo;
@@ -1655,7 +1654,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
-            ret = delete_snapshot(mon, bs, name);
+            ret = delete_snapshot(bs, name);
             if (ret < 0) {
                 return -1;
             }
@@ -1717,7 +1716,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     sn->vm_clock_nsec = qemu_get_clock(vm_clock);
 
     /* Delete old snapshots of the same name */
-    if (name && del_existing_snapshots(mon, name) < 0) {
+    if (name && del_existing_snapshots(name) < 0) {
         goto the_end;
     }
 
@@ -1831,7 +1830,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
         if (bdrv_has_snapshot(bs1)) {
-            delete_snapshot(mon, bs1, name);
+            delete_snapshot(bs1, name);
         }
     }
 }
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 18/22] savevm: Convert do_delvm() to QObject, QError

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-monitor.hx |    3 ++-
 savevm.c        |   14 ++++++++++----
 sysemu.h        |    2 +-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 5ea5748..71cb1a2 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -274,7 +274,8 @@ ETEXI
         .args_type  = "name:s",
         .params     = "tag|id",
         .help       = "delete a VM snapshot from its tag or id",
-        .mhandler.cmd = do_delvm,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_delvm,
     },
 
 STEXI
diff --git a/savevm.c b/savevm.c
index 643273e..031eeff 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1815,24 +1815,30 @@ int load_vmstate(const char *name)
     return 0;
 }
 
-void do_delvm(Monitor *mon, const QDict *qdict)
+int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    int ret;
     DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
     const char *name = qdict_get_str(qdict, "name");
 
     bs = get_bs_snapshots();
     if (!bs) {
-        monitor_printf(mon, "No block device supports snapshots\n");
-        return;
+        qerror_report(QERR_SNAPSHOT_NO_DEVICE);
+        return -1;
     }
 
+    ret = -1;
+
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
         if (bdrv_has_snapshot(bs1)) {
-            delete_snapshot(bs1, name);
+            /* FIXME: will report multiple failures in QMP */
+            ret = delete_snapshot(bs1, name);
         }
     }
+
+    return (ret < 0 ? -1 : 0);
 }
 
 void do_info_snapshots(Monitor *mon)
diff --git a/sysemu.h b/sysemu.h
index fa921df..ee14b8c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -55,7 +55,7 @@ void qemu_system_reset(void);
 
 void do_savevm(Monitor *mon, const QDict *qdict);
 int load_vmstate(const char *name);
-void do_delvm(Monitor *mon, const QDict *qdict);
+int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data);
 void do_info_snapshots(Monitor *mon);
 
 void cpu_synchronize_all_states(void);
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 19/22] savevm: Convert do_savevm() to QError

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/savevm.c b/savevm.c
index 031eeff..8a9e9d1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1682,7 +1682,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 
     bs = get_bs_snapshots();
     if (!bs) {
-        monitor_printf(mon, "No block device can accept snapshots\n");
+        qerror_report(QERR_SNAPSHOT_NO_DEVICE);
         return;
     }
 
@@ -1726,7 +1726,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
-        monitor_printf(mon, "Error %d while writing VM\n", ret);
+        qerror_report(QERR_STATEVM_SAVE_FAILED, get_errno_string(ret));
         goto the_end;
     }
 
@@ -1739,8 +1739,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
             ret = bdrv_snapshot_create(bs1, sn);
             if (ret < 0) {
-                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
+                qerror_report(QERR_SNAPSHOT_CREATE_FAILED,
+                              bdrv_get_device_name(bs1), get_errno_string(ret));
             }
         }
     }
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 20/22] savevm: Convert do_savevm() to QObject

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-monitor.hx |    3 ++-
 savevm.c        |    7 +++++--
 sysemu.h        |    2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 71cb1a2..9a699d4 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -242,7 +242,8 @@ ETEXI
         .args_type  = "name:s?",
         .params     = "[tag|id]",
         .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
-        .mhandler.cmd = do_savevm,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_savevm,
     },
 
 STEXI
diff --git a/savevm.c b/savevm.c
index 8a9e9d1..39a935d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1664,7 +1664,7 @@ static int del_existing_snapshots(const char *name)
     return 0;
 }
 
-void do_savevm(Monitor *mon, const QDict *qdict)
+int do_savevm(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
@@ -1683,7 +1683,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     bs = get_bs_snapshots();
     if (!bs) {
         qerror_report(QERR_SNAPSHOT_NO_DEVICE);
-        return;
+        return -1;
     }
 
     /* ??? Should this occur after vm_stop?  */
@@ -1717,6 +1717,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 
     /* Delete old snapshots of the same name */
     if (name && del_existing_snapshots(name) < 0) {
+        ret = -1;
         goto the_end;
     }
 
@@ -1739,6 +1740,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
             ret = bdrv_snapshot_create(bs1, sn);
             if (ret < 0) {
+                /* FIXME: will report multiple failures in QMP */
                 qerror_report(QERR_SNAPSHOT_CREATE_FAILED,
                               bdrv_get_device_name(bs1), get_errno_string(ret));
             }
@@ -1748,6 +1750,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
  the_end:
     if (saved_vm_running)
         vm_start();
+    return (ret < 0 ? -1 : 0);
 }
 
 int load_vmstate(const char *name)
diff --git a/sysemu.h b/sysemu.h
index ee14b8c..8d8bb64 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -53,7 +53,7 @@ int qemu_exit_requested(void);
 extern qemu_irq qemu_system_powerdown;
 void qemu_system_reset(void);
 
-void do_savevm(Monitor *mon, const QDict *qdict);
+int do_savevm(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int load_vmstate(const char *name);
 int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data);
 void do_info_snapshots(Monitor *mon);
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 21/22] savevm: Convert do_loadvm() to QError

[Luiz Capitulino]

Note that the current code (switch statement) reports errors and
warnings. In QMP though, only errors can be reported.

To fix this we introduce two functions: load_vmstate_warn() and
load_vmstate_error(). So that we maintain the user monitor behavior,
and do the right thing for QMP.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 savevm.c |   67 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/savevm.c b/savevm.c
index 39a935d..8473833 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1753,6 +1753,42 @@ int do_savevm(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return (ret < 0 ? -1 : 0);
 }
 
+static void load_vmstate_warn(int err_code, BlockDriverState *bs1,
+                              const char *name)
+{
+    switch (err_code) {
+    case -ENOTSUP:
+        error_report("Warning: Snapshots not supported on device '%s'",
+                     bdrv_get_device_name(bs1));
+        break;
+    case -ENOENT:
+        error_report("Warning: Could not find snapshot '%s' on device '%s'",
+                     name, bdrv_get_device_name(bs1));
+        break;
+    default:
+        error_report("Warning: Error %d while activating snapshot on '%s'",
+                     err_code, bdrv_get_device_name(bs1));
+        break;
+    }
+}
+
+static void load_vmstate_error(int err_code, BlockDriverState *bs1,
+                               const char *name)
+{
+    switch (err_code) {
+    case -ENOTSUP:
+        qerror_report(QERR_DEVICE_NO_SNAPSHOT, bdrv_get_device_name(bs1));
+        break;
+    case -ENOENT:
+        qerror_report(QERR_SNAPSHOT_NOT_FOUND, bdrv_get_device_name(bs1), name);
+        break;
+    default:
+        qerror_report(QERR_SNAPSHOT_ACTIVATE_FAILED,
+                      bdrv_get_device_name(bs1), get_errno_string(err_code));
+        break;
+    }
+}
+
 int load_vmstate(const char *name)
 {
     DriveInfo *dinfo;
@@ -1763,7 +1799,7 @@ int load_vmstate(const char *name)
 
     bs = get_bs_snapshots();
     if (!bs) {
-        error_report("No block device supports snapshots");
+        qerror_report(QERR_SNAPSHOT_NO_DEVICE);
         return -EINVAL;
     }
 
@@ -1775,26 +1811,13 @@ int load_vmstate(const char *name)
         if (bdrv_has_snapshot(bs1)) {
             ret = bdrv_snapshot_goto(bs1, name);
             if (ret < 0) {
-                switch(ret) {
-                case -ENOTSUP:
-                    error_report("%sSnapshots not supported on device '%s'",
-                                 bs != bs1 ? "Warning: " : "",
-                                 bdrv_get_device_name(bs1));
-                    break;
-                case -ENOENT:
-                    error_report("%sCould not find snapshot '%s' on device '%s'",
-                                 bs != bs1 ? "Warning: " : "",
-                                 name, bdrv_get_device_name(bs1));
-                    break;
-                default:
-                    error_report("%sError %d while activating snapshot on '%s'",
-                                 bs != bs1 ? "Warning: " : "",
-                                 ret, bdrv_get_device_name(bs1));
-                    break;
-                }
-                /* fatal on snapshot block device */
-                if (bs == bs1)
+                if (bs != bs1) {
+                   load_vmstate_warn(ret, bs1, name); 
+                } else {
+                    /* fatal on snapshot block device */
+                    load_vmstate_error(ret, bs1, name);
                     return ret;
+                }
             }
         }
     }
@@ -1802,8 +1825,10 @@ int load_vmstate(const char *name)
     /* Don't even try to load empty VM states */
     ret = bdrv_snapshot_find(bs, &sn, name);
     if (ret < 0) {
+        qerror_report(QERR_SNAPSHOT_NOT_FOUND, bdrv_get_device_name(bs), name);
         return ret;
     } else if (sn.vm_state_size == 0) {
+        qerror_report(QERR_STATEVM_LOAD_FAILED, get_errno_string(-EINVAL));
         return -EINVAL;
     }
 
@@ -1812,7 +1837,7 @@ int load_vmstate(const char *name)
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
-        error_report("Error %d while loading VM state", ret);
+        qerror_report(QERR_STATEVM_LOAD_FAILED, get_errno_string(ret));
         return ret;
     }
     return 0;
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 22/22] savevm: Convert do_loadvm() to QObject

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    7 +++++--
 qemu-monitor.hx |    3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 542c858..32f0396 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2465,18 +2465,21 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return -1;
 }
 
-static void do_loadvm(Monitor *mon, const QDict *qdict)
+static int do_loadvm(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    int ret;
     int saved_vm_running  = vm_running;
     const char *name = qdict_get_str(qdict, "name");
 
     vm_stop(0);
 
-    load_vmstate(name);
+    ret = load_vmstate(name);
 
     if (saved_vm_running) {
         vm_start();
     }
+
+    return (ret < 0 ? -1 : 0);
 }
 
 int monitor_get_fd(Monitor *mon, const char *fdname)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9a699d4..d1d02b0 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -260,7 +260,8 @@ ETEXI
         .args_type  = "name:s",
         .params     = "tag|id",
         .help       = "restore a VM snapshot from its tag or id",
-        .mhandler.cmd = do_loadvm,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_loadvm,
     },
 
 STEXI
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> do_loadvm(), which implements the 'loadvm' Monitor command, pauses
> the emulation to load the saved VM, however it will only resume
> it if the loading succeeds.
>
> In other words, if the user issues 'loadvm' and it fails, the
> end result will be an error message and a paused VM.
>
> This seems an undesirable side effect to me because, most of the
> time, if a Monitor command fails the best thing we can do is to
> leave the VM as it were before the command was executed.
>
> FIXME: This will try to run a potentially corrupted image, the
>        solution is to split load_vmstate() in two and only keep
>        the VM paused if qemu_loadvm_state() fails.

Any of the other errors in loadvm also requires you to not load the
state.

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Nack.

This can cause disk corruption.  You tried to load a vm image, failed
the load somehow (notice the somehow) and you try to run it anyways.
That is a recipe for disaster.  If load_vmstate() fails -> you don't run.

Current code is a mess, but don't do things worse.

Later, Juan.

[Qemu-devel] Re: [PATCH 12/22] QError: New QERR_STATEVM_SAVE_FAILED

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

VMState already used for two things in qemu, but I think that changing
to StateVM _only_ on the error messages is wrong.  Leave it as vmstate,
and if rest of things get ever changed, change just everything?

> ---
>  qerror.c |    4 ++++
>  qerror.h |    3 +++
>  2 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/qerror.c b/qerror.c
> index aa6892d..95c1a95 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -197,6 +197,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "No block device can accept snapshots",
>      },
>      {
> +        .error_fmt = QERR_STATEVM_SAVE_FAILED,
> +        .desc      = "Failed to save VM state (%(reason))",
> +    },
> +    {
>          .error_fmt = QERR_TOO_MANY_FILES,
>          .desc      = "Too many open files",
>      },
> diff --git a/qerror.h b/qerror.h
> index 4a056b3..b915e26 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -163,6 +163,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_SNAPSHOT_NO_DEVICE \
>      "{ 'class': 'SnapshotNoDevice', 'data': {} }"
>  
> +#define QERR_STATEVM_SAVE_FAILED \
> +    "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': %s } }"
> +
>  #define QERR_TOO_MANY_FILES \
>      "{ 'class': 'TooManyFiles', 'data': {} }"

[Qemu-devel] Re: [RFC 00/22]: QMP: Convert savevm/loadvm/delvm

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
>  Hi there,
>
>  Libvirt has a new snapshot API which uses savevm, loadvm and delvm, so we
> need to convert them to QMP.
>
>  I thought this wouldn't be difficult, but while doing and testing this work
> I hit a number of problems and had to made not so easy decisions.
>
>  Most of the time, the problem is that the handler implementation is not
> as consistent as it should be. Usually, this doesn't affect users very much
> but the headache begins when you have to make this available under QMP.
>
>  Here goes a list of macro issues/decisions. Details (and other small problems)
> in the patches.
>
>    1. Multiple failures: do_delvm() and do_savevm() report errors in a
>       QTAILQ_FOREACH() loop and they don't return when an error happens.
>
>       Although QMP will end up reporting only the first error, this is
>       considered a bug because handlers are not expected to continue
>       executing when an error happens.

I haven't looked at all the error cases, but I think that part of the
problem are devices like cdrom, that are by definition _non_
snapshotable  (what an ugly word).

>       There are two solutions:
>
>             1) Return right away in the first error
>             2) Identify fatal errors and only report those
>
>       I don't know the implications of doing 1) and don't know how to
>       to do 2) (although do_loadvm() works this way).
>
>       Can someone from the block layer help here?

Use case:  We want to save snapshot foo.  It can be:
- snapshot foo already exist in some device. What to do:
   - just fail and ask the user for other name
   - overwrite the name on the devices that already have it
   - any other good idea?

if we just fail the save, then we tell the user: devices bar1 and bar2
already have that snapshot name.  What to do?
- force the user to use a different name (not such a bad idea)
- ask the user to stop the vm and remove the snaphost from the ofended
  devices with qemu-img.  High avalavility people don't like to be
  forced to stop machines.
- delvm just ignore the devices that don't have the snapshot name
  (basically what we have now)
- we add a delvm_force or del_device_vm <device> <name> operations.\
- any other great idea?

Except where commented, I like the rest of the patches.  The loadvm is a
change that I don't agree with.  The StateVM name is something that I
can live with.

Thanks, Juan.

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Luiz Capitulino]

On Tue, 20 Apr 2010 23:28:23 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > do_loadvm(), which implements the 'loadvm' Monitor command, pauses
> > the emulation to load the saved VM, however it will only resume
> > it if the loading succeeds.
> >
> > In other words, if the user issues 'loadvm' and it fails, the
> > end result will be an error message and a paused VM.
> >
> > This seems an undesirable side effect to me because, most of the
> > time, if a Monitor command fails the best thing we can do is to
> > leave the VM as it were before the command was executed.
> >
> > FIXME: This will try to run a potentially corrupted image, the
> >        solution is to split load_vmstate() in two and only keep
> >        the VM paused if qemu_loadvm_state() fails.
> 
> Any of the other errors in loadvm also requires you to not load the
> state.

 Really? Everything that happens before qemu_fopen_bdrv() seems to be
only looking for the snapshot..

>
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> Nack.
> 
> This can cause disk corruption.  You tried to load a vm image, failed
> the load somehow (notice the somehow) and you try to run it anyways.
> That is a recipe for disaster.  If load_vmstate() fails -> you don't run.

 My understanding is that the loading only happens in qemu_loadvm_state(),
is this wrong?

 If it isn't, my plan is to split load_vmstate() in two functions:

- load_vmstate_prepare(): everything before qemu_fopen_bdrv()
- load_vmstate_finish(): qemu_loadvm_state() block

 then, do_loadvm() would do:

err = load_vmstate_prepare();
if (err && vm_running) {
   vm_start();
   return -1;
}

err = load_vmstate_finish();
if (err) {
  return -1;
}

vm_start();
return 0;

 And load_vmstate() would just call prepare() and finish(), maintaining
its current behavior.

 It's important to understand why this is needed: currently you have a
'return 0' in line savevm.c:1872. It's an error path, but I'm almost sure
that this trick is needed because if you return -1 there and loadvm fails
for stupid reasons (like a not found snapshot) you get a paused VM.

[Qemu-devel] Re: [PATCH 12/22] QError: New QERR_STATEVM_SAVE_FAILED

[Luiz Capitulino]

On Tue, 20 Apr 2010 23:31:22 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> VMState already used for two things in qemu, but I think that changing
> to StateVM _only_ on the error messages is wrong.  Leave it as vmstate,
> and if rest of things get ever changed, change just everything?

 Let's use VMState then, I will fix.

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Daniel P. Berrange]

On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
> There are error handling functions in QEMU which print errno codes
> to the user. While it's debatable if this is good from a user
> perspective, sometimes it's the best you can do because it's what
> system calls return and this is also useful for debugging.
> 
> So, we need a way to expose those codes in QMP. We can't use the
> codes themselfs because they may vary between systems.
> 
> The best solution I can think of is returning the string
> representation of the name. For example, EIO becomes "EIO".
> 
> This is what get_errno_string() does.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-error.c |   25 +++++++++++++++++++++++++
>  qemu-error.h |    1 +
>  2 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-error.c b/qemu-error.c
> index 5a35e7c..55ce133 100644
> --- a/qemu-error.c
> +++ b/qemu-error.c
> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
>      va_end(ap);
>      error_printf("\n");
>  }
> +
> +/*
> + * This is probably only useful for QMP
> + */
> +const char *get_errno_string(int err)
> +{
> +    assert(err < 0);
> +
> +    switch (err) {
> +    case -EINVAL:
> +        return "EINVAL";
> +    case -EIO:
> +        return "EIO";
> +    case -ENOENT:
> +        return "ENOENT";
> +    case -ENOMEDIUM:
> +        return "ENOMEDIUM";
> +    case -ENOTSUP:
> +        return "ENOTSUP";
> +    default:
> +        return "unknown";
> +    }
> +
> +    abort();
> +}

Wouldn't it be nicer to return strerror_r()  output instead of errno
names ?


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Tue, 20 Apr 2010 23:28:23 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> > do_loadvm(), which implements the 'loadvm' Monitor command, pauses
>> > the emulation to load the saved VM, however it will only resume
>> > it if the loading succeeds.
>> >
>> > In other words, if the user issues 'loadvm' and it fails, the
>> > end result will be an error message and a paused VM.
>> >
>> > This seems an undesirable side effect to me because, most of the
>> > time, if a Monitor command fails the best thing we can do is to
>> > leave the VM as it were before the command was executed.
>> >
>> > FIXME: This will try to run a potentially corrupted image, the
>> >        solution is to split load_vmstate() in two and only keep
>> >        the VM paused if qemu_loadvm_state() fails.
>> 
>> Any of the other errors in loadvm also requires you to not load the
>> state.
>
>  Really? Everything that happens before qemu_fopen_bdrv() seems to be
> only looking for the snapshot..

Let's see:

    bs = get_bs_snapshots();
    if (!bs) {
        error_report("No block device supports snapshots");
        return -EINVAL;
    }

//// If we are asked to load a vm and there are no snapshots on any disk
//// ... trying to run the image look overkill

    /* Flush all IO requests so they don't interfere with the new state.  */
    qemu_aio_flush();

    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs1 = dinfo->bdrv;
        if (bdrv_has_snapshot(bs1)) {

/// We found a device that has snapshots
            ret = bdrv_snapshot_goto(bs1, name);
            if (ret < 0) {
/// And don't have a snapshot with the name that we wanted
                switch(ret) {
                case -ENOTSUP:
                    error_report("%sSnapshots not supported on device '%s'",
                                 bs != bs1 ? "Warning: " : "",
                                 bdrv_get_device_name(bs1));
                    break;
                case -ENOENT:
                    error_report("%sCould not find snapshot '%s' on device '%s'",
                                 bs != bs1 ? "Warning: " : "",
                                 name, bdrv_get_device_name(bs1));
                    break;
                default:
                    error_report("%sError %d while activating snapshot on '%s'",
                                 bs != bs1 ? "Warning: " : "",
                                 ret, bdrv_get_device_name(bs1));
                    break;
                }
                /* fatal on snapshot block device */
// I think that one inconditional exit with predjuice could be in order here

// Notice that bdrv_snapshot_goto() modifies the disk, name is as bad as
// you can get.  It just open the disk, opens the snapshot, increases
// its counter of users, and makes it available for use after here
// (i.e. loading state, posibly conflicting with previous running
// VM a.k.a. disk corruption.

                if (bs == bs1)
                    return 0;

// This error is as bad as it can gets :(  We have to load a vmstate,
// and the disk that should have the memory image don't have it.
// This is an error, I just put the wrong nunmber the previous time.
// Notice that this error should be very rare.
            }
        }
    }

As stated, I don't think that trying to run the machine at any point
would make any sense.  Only case where it is safe to run it is if the
failure is at get_bs_snapshots(), but at that point running the machine
means:

<something happens>
$ loadvm other_image
  Error "other_image" snapshot don't exist.
$

running the previous VM looks like something that should be done
explicitely.  If the error happened after that get_bs_snapshots(),
We would need a new flag to just refuse to continue.  Only valid
operations at that point are other loadvm operations, i.e. our state is
wrong one way or another.

>  My understanding is that the loading only happens in qemu_loadvm_state(),
> is this wrong?

As stated on description, don't make sense that split.  It all case,
what we need is the new flag to not allow other run operations other
than loadvm.

Later, Juan.

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Kevin Wolf]

Am 20.04.2010 23:09, schrieb Luiz Capitulino:
> do_loadvm(), which implements the 'loadvm' Monitor command, pauses
> the emulation to load the saved VM, however it will only resume
> it if the loading succeeds.
> 
> In other words, if the user issues 'loadvm' and it fails, the
> end result will be an error message and a paused VM.
> 
> This seems an undesirable side effect to me because, most of the
> time, if a Monitor command fails the best thing we can do is to
> leave the VM as it were before the command was executed.

I completely agree with Juan here, this is broken.

If you could leave the VM as it was before, just like you describe
above, everything would be okay. But in fact you can't tell where the
error occurred. You may still have the old state; or you may have loaded
the snapshot on one disk, but not on the other one; or you may have
loaded snapshots for all disks, but only half of the devices.

We must not run a machine in such an unknown state. I'd even go further
and say that after a failed loadvm we must even prevent that a user uses
the cont command to resume manually.

> FIXME: This will try to run a potentially corrupted image, the
>        solution is to split load_vmstate() in two and only keep
>        the VM paused if qemu_loadvm_state() fails.

Your suggestion of having a prepare function that doesn't change any
state looks fine to me. It could just check if the snapshot is there and
contains VM state. This should cover all of the trivial cases where
recovery is really as easy as resuming the old state.

Another problem that I see is that it's really hard to define what an
error is. Current code print "Warning: ..." for all non-primary images
and continues with loading the snapshot. I'm really unsure what the
right behaviour would be if a snapshot doesn't exist on a secondary
image (note that this is not the CD-ROM case, these drives are already
excluded by bdrv_has_snapshot as they are read-only).

Kevin

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Kevin Wolf]

Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
> On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
>> There are error handling functions in QEMU which print errno codes
>> to the user. While it's debatable if this is good from a user
>> perspective, sometimes it's the best you can do because it's what
>> system calls return and this is also useful for debugging.
>>
>> So, we need a way to expose those codes in QMP. We can't use the
>> codes themselfs because they may vary between systems.
>>
>> The best solution I can think of is returning the string
>> representation of the name. For example, EIO becomes "EIO".
>>
>> This is what get_errno_string() does.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  qemu-error.c |   25 +++++++++++++++++++++++++
>>  qemu-error.h |    1 +
>>  2 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-error.c b/qemu-error.c
>> index 5a35e7c..55ce133 100644
>> --- a/qemu-error.c
>> +++ b/qemu-error.c
>> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
>>      va_end(ap);
>>      error_printf("\n");
>>  }
>> +
>> +/*
>> + * This is probably only useful for QMP
>> + */
>> +const char *get_errno_string(int err)
>> +{
>> +    assert(err < 0);
>> +
>> +    switch (err) {
>> +    case -EINVAL:
>> +        return "EINVAL";
>> +    case -EIO:
>> +        return "EIO";
>> +    case -ENOENT:
>> +        return "ENOENT";
>> +    case -ENOMEDIUM:
>> +        return "ENOMEDIUM";
>> +    case -ENOTSUP:
>> +        return "ENOTSUP";
>> +    default:
>> +        return "unknown";
>> +    }
>> +
>> +    abort();
>> +}
> 
> Wouldn't it be nicer to return strerror_r()  output instead of errno
> names ?

I agree. And it would be more complete, too.

Kevin

[Qemu-devel] Re: [PATCH 18/22] savevm: Convert do_delvm() to QObject, QError

[Kevin Wolf]

Am 20.04.2010 23:09, schrieb Luiz Capitulino:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-monitor.hx |    3 ++-
>  savevm.c        |   14 ++++++++++----
>  sysemu.h        |    2 +-
>  3 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 5ea5748..71cb1a2 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -274,7 +274,8 @@ ETEXI
>          .args_type  = "name:s",
>          .params     = "tag|id",
>          .help       = "delete a VM snapshot from its tag or id",
> -        .mhandler.cmd = do_delvm,
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_delvm,
>      },
>  
>  STEXI
> diff --git a/savevm.c b/savevm.c
> index 643273e..031eeff 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name)
>      return 0;
>  }
>  
> -void do_delvm(Monitor *mon, const QDict *qdict)
> +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> +    int ret;
>      DriveInfo *dinfo;
>      BlockDriverState *bs, *bs1;
>      const char *name = qdict_get_str(qdict, "name");
>  
>      bs = get_bs_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device supports snapshots\n");
> -        return;
> +        qerror_report(QERR_SNAPSHOT_NO_DEVICE);
> +        return -1;
>      }
>  
> +    ret = -1;
> +
>      QTAILQ_FOREACH(dinfo, &drives, next) {
>          bs1 = dinfo->bdrv;
>          if (bdrv_has_snapshot(bs1)) {
> -            delete_snapshot(bs1, name);
> +            /* FIXME: will report multiple failures in QMP */
> +            ret = delete_snapshot(bs1, name);
>          }
>      }
> +
> +    return (ret < 0 ? -1 : 0);

Doesn't this return success when the first drive fails and the second
one succeeds?

Kevin

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[malc]

On Wed, 21 Apr 2010, Kevin Wolf wrote:

> Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
> > On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
> >> There are error handling functions in QEMU which print errno codes
> >> to the user. While it's debatable if this is good from a user
> >> perspective, sometimes it's the best you can do because it's what
> >> system calls return and this is also useful for debugging.
> >>
> >> So, we need a way to expose those codes in QMP. We can't use the
> >> codes themselfs because they may vary between systems.
> >>
> >> The best solution I can think of is returning the string
> >> representation of the name. For example, EIO becomes "EIO".
> >>
> >> This is what get_errno_string() does.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >>  qemu-error.c |   25 +++++++++++++++++++++++++
> >>  qemu-error.h |    1 +
> >>  2 files changed, 26 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qemu-error.c b/qemu-error.c
> >> index 5a35e7c..55ce133 100644
> >> --- a/qemu-error.c
> >> +++ b/qemu-error.c
> >> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
> >>      va_end(ap);
> >>      error_printf("\n");
> >>  }
> >> +
> >> +/*
> >> + * This is probably only useful for QMP
> >> + */
> >> +const char *get_errno_string(int err)
> >> +{
> >> +    assert(err < 0);
> >> +
> >> +    switch (err) {
> >> +    case -EINVAL:
> >> +        return "EINVAL";
> >> +    case -EIO:
> >> +        return "EIO";
> >> +    case -ENOENT:
> >> +        return "ENOENT";
> >> +    case -ENOMEDIUM:
> >> +        return "ENOMEDIUM";
> >> +    case -ENOTSUP:
> >> +        return "ENOTSUP";
> >> +    default:
> >> +        return "unknown";
> >> +    }
> >> +
> >> +    abort();
> >> +}
> > 
> > Wouldn't it be nicer to return strerror_r()  output instead of errno
> > names ?
> 
> I agree. And it would be more complete, too.

OTOH it has a problem of returning translated messages (subject to
LC_MESSAGES value).

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Luiz Capitulino]

On Wed, 21 Apr 2010 10:36:29 +0200
Juan Quintela <quintela@redhat.com> wrote:

>     QTAILQ_FOREACH(dinfo, &drives, next) {
>         bs1 = dinfo->bdrv;
>         if (bdrv_has_snapshot(bs1)) {
> 
> /// We found a device that has snapshots
>             ret = bdrv_snapshot_goto(bs1, name);
>             if (ret < 0) {
> /// And don't have a snapshot with the name that we wanted
>                 switch(ret) {
>                 case -ENOTSUP:
>                     error_report("%sSnapshots not supported on device '%s'",
>                                  bs != bs1 ? "Warning: " : "",
>                                  bdrv_get_device_name(bs1));
>                     break;
>                 case -ENOENT:
>                     error_report("%sCould not find snapshot '%s' on device '%s'",
>                                  bs != bs1 ? "Warning: " : "",
>                                  name, bdrv_get_device_name(bs1));
>                     break;
>                 default:
>                     error_report("%sError %d while activating snapshot on '%s'",
>                                  bs != bs1 ? "Warning: " : "",
>                                  ret, bdrv_get_device_name(bs1));
>                     break;
>                 }
>                 /* fatal on snapshot block device */
> // I think that one inconditional exit with predjuice could be in order here
> 
> // Notice that bdrv_snapshot_goto() modifies the disk, name is as bad as
> // you can get.  It just open the disk, opens the snapshot, increases
> // its counter of users, and makes it available for use after here
> // (i.e. loading state, posibly conflicting with previous running
> // VM a.k.a. disk corruption.
> 
>                 if (bs == bs1)
>                     return 0;
> 
> // This error is as bad as it can gets :(  We have to load a vmstate,
> // and the disk that should have the memory image don't have it.
> // This is an error, I just put the wrong nunmber the previous time.
> // Notice that this error should be very rare.

 So, the current code is buggy and if you fix it (by returning -1)
you'll get another bug: loadvm will stop the VM for trivial errors
like a not found image.

 How do you plan to fix this?

> As stated, I don't think that trying to run the machine at any point
> would make any sense.  Only case where it is safe to run it is if the
> failure is at get_bs_snapshots(), but at that point running the machine
> means:

 Actually, it must not pause the VM when recovery is (clearly) possible,
otherwise it's a usability bug for the user Monitor and a possibly serious
bug when you don't have human intervention (eg. QMP).

> 
> <something happens>
> $ loadvm other_image
>   Error "other_image" snapshot don't exist.
> $
> 
> running the previous VM looks like something that should be done
> explicitely.  If the error happened after that get_bs_snapshots(),
> We would need a new flag to just refuse to continue.  Only valid
> operations at that point are other loadvm operations, i.e. our state is
> wrong one way or another.

 It's not clear to me how this flag can help, but anyway, what we need
here is:

1. Fail when failure is reported (vs. report a failure and return OK)

2. Don't keep the VM paused when recovery is possible

 If you can fix that, it's ok to me: I'll drop this and the next patch.

 Otherwise I'll have to insist on the split.

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Luiz Capitulino]

On Wed, 21 Apr 2010 15:28:16 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 20.04.2010 23:09, schrieb Luiz Capitulino:
> > do_loadvm(), which implements the 'loadvm' Monitor command, pauses
> > the emulation to load the saved VM, however it will only resume
> > it if the loading succeeds.
> > 
> > In other words, if the user issues 'loadvm' and it fails, the
> > end result will be an error message and a paused VM.
> > 
> > This seems an undesirable side effect to me because, most of the
> > time, if a Monitor command fails the best thing we can do is to
> > leave the VM as it were before the command was executed.
> 
> I completely agree with Juan here, this is broken.

 Yeah, it's an RFC. I decided to send it as is because I needed feedback as
this series is a snowball.

> If you could leave the VM as it was before, just like you describe
> above, everything would be okay. But in fact you can't tell where the
> error occurred. You may still have the old state; or you may have loaded
> the snapshot on one disk, but not on the other one; or you may have
> loaded snapshots for all disks, but only half of the devices.
> 
> We must not run a machine in such an unknown state. I'd even go further
> and say that after a failed loadvm we must even prevent that a user uses
> the cont command to resume manually.

 Maybe 'info status' should have a specific status for this too.

 (Assuming we don't want to radically call exit(1)).

> > FIXME: This will try to run a potentially corrupted image, the
> >        solution is to split load_vmstate() in two and only keep
> >        the VM paused if qemu_loadvm_state() fails.
> 
> Your suggestion of having a prepare function that doesn't change any
> state looks fine to me. It could just check if the snapshot is there and
> contains VM state. This should cover all of the trivial cases where
> recovery is really as easy as resuming the old state.

 That's exactly what I want to do.

> Another problem that I see is that it's really hard to define what an
> error is. Current code print "Warning: ..." for all non-primary images
> and continues with loading the snapshot. I'm really unsure what the
> right behaviour would be if a snapshot doesn't exist on a secondary
> image (note that this is not the CD-ROM case, these drives are already
> excluded by bdrv_has_snapshot as they are read-only).

 Defining the right behavior and deciding what to expose have been
proven very difficult tasks in the QMP world.

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Luiz Capitulino]

On Wed, 21 Apr 2010 18:42:38 +0400 (MSD)
malc <av1474@comtv.ru> wrote:

> On Wed, 21 Apr 2010, Kevin Wolf wrote:
> 
> > Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
> > > On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
> > >> There are error handling functions in QEMU which print errno codes
> > >> to the user. While it's debatable if this is good from a user
> > >> perspective, sometimes it's the best you can do because it's what
> > >> system calls return and this is also useful for debugging.
> > >>
> > >> So, we need a way to expose those codes in QMP. We can't use the
> > >> codes themselfs because they may vary between systems.
> > >>
> > >> The best solution I can think of is returning the string
> > >> representation of the name. For example, EIO becomes "EIO".
> > >>
> > >> This is what get_errno_string() does.
> > >>
> > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > >> ---
> > >>  qemu-error.c |   25 +++++++++++++++++++++++++
> > >>  qemu-error.h |    1 +
> > >>  2 files changed, 26 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/qemu-error.c b/qemu-error.c
> > >> index 5a35e7c..55ce133 100644
> > >> --- a/qemu-error.c
> > >> +++ b/qemu-error.c
> > >> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
> > >>      va_end(ap);
> > >>      error_printf("\n");
> > >>  }
> > >> +
> > >> +/*
> > >> + * This is probably only useful for QMP
> > >> + */
> > >> +const char *get_errno_string(int err)
> > >> +{
> > >> +    assert(err < 0);
> > >> +
> > >> +    switch (err) {
> > >> +    case -EINVAL:
> > >> +        return "EINVAL";
> > >> +    case -EIO:
> > >> +        return "EIO";
> > >> +    case -ENOENT:
> > >> +        return "ENOENT";
> > >> +    case -ENOMEDIUM:
> > >> +        return "ENOMEDIUM";
> > >> +    case -ENOTSUP:
> > >> +        return "ENOTSUP";
> > >> +    default:
> > >> +        return "unknown";
> > >> +    }
> > >> +
> > >> +    abort();
> > >> +}
> > > 
> > > Wouldn't it be nicer to return strerror_r()  output instead of errno
> > > names ?
> > 
> > I agree. And it would be more complete, too.
> 
> OTOH it has a problem of returning translated messages (subject to
> LC_MESSAGES value).

 Exactly, and I'm not sure if there's anything that ensure they're
exactly the same among different systems.

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Daniel P. Berrange]

On Wed, Apr 21, 2010 at 12:12:14PM -0300, Luiz Capitulino wrote:
> On Wed, 21 Apr 2010 18:42:38 +0400 (MSD)
> malc <av1474@comtv.ru> wrote:
> 
> > On Wed, 21 Apr 2010, Kevin Wolf wrote:
> > 
> > > Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
> > > > On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
> > > >> There are error handling functions in QEMU which print errno codes
> > > >> to the user. While it's debatable if this is good from a user
> > > >> perspective, sometimes it's the best you can do because it's what
> > > >> system calls return and this is also useful for debugging.
> > > >>
> > > >> So, we need a way to expose those codes in QMP. We can't use the
> > > >> codes themselfs because they may vary between systems.
> > > >>
> > > >> The best solution I can think of is returning the string
> > > >> representation of the name. For example, EIO becomes "EIO".
> > > >>
> > > >> This is what get_errno_string() does.
> > > >>
> > > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > >> ---
> > > >>  qemu-error.c |   25 +++++++++++++++++++++++++
> > > >>  qemu-error.h |    1 +
> > > >>  2 files changed, 26 insertions(+), 0 deletions(-)
> > > >>
> > > >> diff --git a/qemu-error.c b/qemu-error.c
> > > >> index 5a35e7c..55ce133 100644
> > > >> --- a/qemu-error.c
> > > >> +++ b/qemu-error.c
> > > >> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
> > > >>      va_end(ap);
> > > >>      error_printf("\n");
> > > >>  }
> > > >> +
> > > >> +/*
> > > >> + * This is probably only useful for QMP
> > > >> + */
> > > >> +const char *get_errno_string(int err)
> > > >> +{
> > > >> +    assert(err < 0);
> > > >> +
> > > >> +    switch (err) {
> > > >> +    case -EINVAL:
> > > >> +        return "EINVAL";
> > > >> +    case -EIO:
> > > >> +        return "EIO";
> > > >> +    case -ENOENT:
> > > >> +        return "ENOENT";
> > > >> +    case -ENOMEDIUM:
> > > >> +        return "ENOMEDIUM";
> > > >> +    case -ENOTSUP:
> > > >> +        return "ENOTSUP";
> > > >> +    default:
> > > >> +        return "unknown";
> > > >> +    }
> > > >> +
> > > >> +    abort();
> > > >> +}
> > > > 
> > > > Wouldn't it be nicer to return strerror_r()  output instead of errno
> > > > names ?
> > > 
> > > I agree. And it would be more complete, too.
> > 
> > OTOH it has a problem of returning translated messages (subject to
> > LC_MESSAGES value).
> 
>  Exactly, and I'm not sure if there's anything that ensure they're
> exactly the same among different systems.

I thought QMP already declared that the printable error strings are subject
to arbitrary change at any time, which includes translation? Apps needing 
something reliable should be hooking onto the error code.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Kevin Wolf]

Am 21.04.2010 17:08, schrieb Luiz Capitulino:
> On Wed, 21 Apr 2010 15:28:16 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 20.04.2010 23:09, schrieb Luiz Capitulino:
>>> do_loadvm(), which implements the 'loadvm' Monitor command, pauses
>>> the emulation to load the saved VM, however it will only resume
>>> it if the loading succeeds.
>>>
>>> In other words, if the user issues 'loadvm' and it fails, the
>>> end result will be an error message and a paused VM.
>>>
>>> This seems an undesirable side effect to me because, most of the
>>> time, if a Monitor command fails the best thing we can do is to
>>> leave the VM as it were before the command was executed.
>>
>> I completely agree with Juan here, this is broken.
> 
>  Yeah, it's an RFC. I decided to send it as is because I needed feedback as
> this series is a snowball.

Which is the right thing to do, so we can find such problems. :-)

>> If you could leave the VM as it was before, just like you describe
>> above, everything would be okay. But in fact you can't tell where the
>> error occurred. You may still have the old state; or you may have loaded
>> the snapshot on one disk, but not on the other one; or you may have
>> loaded snapshots for all disks, but only half of the devices.
>>
>> We must not run a machine in such an unknown state. I'd even go further
>> and say that after a failed loadvm we must even prevent that a user uses
>> the cont command to resume manually.
> 
>  Maybe 'info status' should have a specific status for this too.
> 
>  (Assuming we don't want to radically call exit(1)).

A different status that disallows cont sounds good. But exit() would
work for me as well and is probably easier. However, I'm not sure if it
would work well for management.

>>> FIXME: This will try to run a potentially corrupted image, the
>>>        solution is to split load_vmstate() in two and only keep
>>>        the VM paused if qemu_loadvm_state() fails.
>>
>> Your suggestion of having a prepare function that doesn't change any
>> state looks fine to me. It could just check if the snapshot is there and
>> contains VM state. This should cover all of the trivial cases where
>> recovery is really as easy as resuming the old state.
> 
>  That's exactly what I want to do.
> 
>> Another problem that I see is that it's really hard to define what an
>> error is. Current code print "Warning: ..." for all non-primary images
>> and continues with loading the snapshot. I'm really unsure what the
>> right behaviour would be if a snapshot doesn't exist on a secondary
>> image (note that this is not the CD-ROM case, these drives are already
>> excluded by bdrv_has_snapshot as they are read-only).
> 
>  Defining the right behavior and deciding what to expose have been
> proven very difficult tasks in the QMP world.

There's nothing QMP specific about it. The problem has always been
there, QMP just means that you involuntarily get to review all of it.

Kevin

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Luiz Capitulino]

On Wed, 21 Apr 2010 16:15:58 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Apr 21, 2010 at 12:12:14PM -0300, Luiz Capitulino wrote:
> > On Wed, 21 Apr 2010 18:42:38 +0400 (MSD)
> > malc <av1474@comtv.ru> wrote:
> > 
> > > On Wed, 21 Apr 2010, Kevin Wolf wrote:
> > > 
> > > > Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
> > > > > On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
> > > > >> There are error handling functions in QEMU which print errno codes
> > > > >> to the user. While it's debatable if this is good from a user
> > > > >> perspective, sometimes it's the best you can do because it's what
> > > > >> system calls return and this is also useful for debugging.
> > > > >>
> > > > >> So, we need a way to expose those codes in QMP. We can't use the
> > > > >> codes themselfs because they may vary between systems.
> > > > >>
> > > > >> The best solution I can think of is returning the string
> > > > >> representation of the name. For example, EIO becomes "EIO".
> > > > >>
> > > > >> This is what get_errno_string() does.
> > > > >>
> > > > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > >> ---
> > > > >>  qemu-error.c |   25 +++++++++++++++++++++++++
> > > > >>  qemu-error.h |    1 +
> > > > >>  2 files changed, 26 insertions(+), 0 deletions(-)
> > > > >>
> > > > >> diff --git a/qemu-error.c b/qemu-error.c
> > > > >> index 5a35e7c..55ce133 100644
> > > > >> --- a/qemu-error.c
> > > > >> +++ b/qemu-error.c
> > > > >> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
> > > > >>      va_end(ap);
> > > > >>      error_printf("\n");
> > > > >>  }
> > > > >> +
> > > > >> +/*
> > > > >> + * This is probably only useful for QMP
> > > > >> + */
> > > > >> +const char *get_errno_string(int err)
> > > > >> +{
> > > > >> +    assert(err < 0);
> > > > >> +
> > > > >> +    switch (err) {
> > > > >> +    case -EINVAL:
> > > > >> +        return "EINVAL";
> > > > >> +    case -EIO:
> > > > >> +        return "EIO";
> > > > >> +    case -ENOENT:
> > > > >> +        return "ENOENT";
> > > > >> +    case -ENOMEDIUM:
> > > > >> +        return "ENOMEDIUM";
> > > > >> +    case -ENOTSUP:
> > > > >> +        return "ENOTSUP";
> > > > >> +    default:
> > > > >> +        return "unknown";
> > > > >> +    }
> > > > >> +
> > > > >> +    abort();
> > > > >> +}
> > > > > 
> > > > > Wouldn't it be nicer to return strerror_r()  output instead of errno
> > > > > names ?
> > > > 
> > > > I agree. And it would be more complete, too.
> > > 
> > > OTOH it has a problem of returning translated messages (subject to
> > > LC_MESSAGES value).
> > 
> >  Exactly, and I'm not sure if there's anything that ensure they're
> > exactly the same among different systems.
> 
> I thought QMP already declared that the printable error strings are subject
> to arbitrary change at any time, which includes translation? Apps needing 
> something reliable should be hooking onto the error code.

 Fair enough, although mixed languages would be a pain (ie. error desc
in english and this one in something else).

> 
> Regards,
> Daniel

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Wed, 21 Apr 2010 10:36:29 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>>     QTAILQ_FOREACH(dinfo, &drives, next) {
>>         bs1 = dinfo->bdrv;
>>         if (bdrv_has_snapshot(bs1)) {
>> 
>> /// We found a device that has snapshots
>>             ret = bdrv_snapshot_goto(bs1, name);
>>             if (ret < 0) {
>> /// And don't have a snapshot with the name that we wanted
>>                 switch(ret) {
>>                 case -ENOTSUP:
>>                     error_report("%sSnapshots not supported on device '%s'",
>>                                  bs != bs1 ? "Warning: " : "",
>>                                  bdrv_get_device_name(bs1));
>>                     break;
>>                 case -ENOENT:
>>                     error_report("%sCould not find snapshot '%s' on device '%s'",
>>                                  bs != bs1 ? "Warning: " : "",
>>                                  name, bdrv_get_device_name(bs1));
>>                     break;
>>                 default:
>>                     error_report("%sError %d while activating snapshot on '%s'",
>>                                  bs != bs1 ? "Warning: " : "",
>>                                  ret, bdrv_get_device_name(bs1));
>>                     break;
>>                 }
>>                 /* fatal on snapshot block device */
>> // I think that one inconditional exit with predjuice could be in order here
>> 
>> // Notice that bdrv_snapshot_goto() modifies the disk, name is as bad as
>> // you can get.  It just open the disk, opens the snapshot, increases
>> // its counter of users, and makes it available for use after here
>> // (i.e. loading state, posibly conflicting with previous running
>> // VM a.k.a. disk corruption.
>> 
>>                 if (bs == bs1)
>>                     return 0;
>> 
>> // This error is as bad as it can gets :(  We have to load a vmstate,
>> // and the disk that should have the memory image don't have it.
>> // This is an error, I just put the wrong nunmber the previous time.
>> // Notice that this error should be very rare.
>
>  So, the current code is buggy and if you fix it (by returning -1)
> you'll get another bug: loadvm will stop the VM for trivial errors
> like a not found image.

It is not a trivial error!!!!  And worse, it is not recoverable :(

>  How do you plan to fix this?

Returning error and stoping machine.

>> As stated, I don't think that trying to run the machine at any point
>> would make any sense.  Only case where it is safe to run it is if the
>> failure is at get_bs_snapshots(), but at that point running the machine
>> means:
>
>  Actually, it must not pause the VM when recovery is (clearly) possible,
> otherwise it's a usability bug for the user Monitor and a possibly serious
> bug when you don't have human intervention (eg. QMP).

It is not posible, we have change the device status from what was
before.  bets are off.  we don't have a way to go back to the "safe state".

>> <something happens>
>> $ loadvm other_image
>>   Error "other_image" snapshot don't exist.
>> $
>> 
>> running the previous VM looks like something that should be done
>> explicitely.  If the error happened after that get_bs_snapshots(),
>> We would need a new flag to just refuse to continue.  Only valid
>> operations at that point are other loadvm operations, i.e. our state is
>> wrong one way or another.
>
>  It's not clear to me how this flag can help, but anyway, what we need
> here is:
>
> 1. Fail when failure is reported (vs. report a failure and return OK)

This is a bug, plain an simple.

> 2. Don't keep the VM paused when recovery is possible
>
>  If you can fix that, it's ok to me: I'll drop this and the next patch.
>
>  Otherwise I'll have to insist on the split.

Re-read my email.   At this point, nothing is fixable :(  After doing
the 1st:

>>             ret = bdrv_snapshot_goto(bs1, name);

and not returning an error -> state has changed, period.  You can't
restart the machine.

If you prefer, you can chang loadvm in a way that after a failure -> you
can't "cont" it until you get a "working" loadvm.

Later, Juan.

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Kevin Wolf]

Am 21.04.2010 17:39, schrieb Juan Quintela:
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> 2. Don't keep the VM paused when recovery is possible
>>
>>  If you can fix that, it's ok to me: I'll drop this and the next patch.
>>
>>  Otherwise I'll have to insist on the split.
> 
> Re-read my email.   At this point, nothing is fixable :(  After doing
> the 1st:
> 
>>>             ret = bdrv_snapshot_goto(bs1, name);
> 
> and not returning an error -> state has changed, period.  You can't
> restart the machine.

Right, at this point. But the most likely error for bdrv_snapshot_goto
is that the snapshot doesn't even exist. This is something that you can
check before you change any state. I think this is what Luiz meant (at
least it is what I said and he agreed).

Kevin

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Wed, 21 Apr 2010 15:28:16 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
>
>> Am 20.04.2010 23:09, schrieb Luiz Capitulino:
>> > do_loadvm(), which implements the 'loadvm' Monitor command, pauses
>> > the emulation to load the saved VM, however it will only resume
>> > it if the loading succeeds.
>> > 
>> > In other words, if the user issues 'loadvm' and it fails, the
>> > end result will be an error message and a paused VM.
>> > 
>> > This seems an undesirable side effect to me because, most of the
>> > time, if a Monitor command fails the best thing we can do is to
>> > leave the VM as it were before the command was executed.
>> 
>> I completely agree with Juan here, this is broken.
>
>  Yeah, it's an RFC. I decided to send it as is because I needed feedback as
> this series is a snowball.
>
>> If you could leave the VM as it was before, just like you describe
>> above, everything would be okay. But in fact you can't tell where the
>> error occurred. You may still have the old state; or you may have loaded
>> the snapshot on one disk, but not on the other one; or you may have
>> loaded snapshots for all disks, but only half of the devices.
>> 
>> We must not run a machine in such an unknown state. I'd even go further
>> and say that after a failed loadvm we must even prevent that a user uses
>> the cont command to resume manually.
>
>  Maybe 'info status' should have a specific status for this too.
>
>  (Assuming we don't want to radically call exit(1)).

I tried a variation of this in the past, and was not a clear agreement.

Basically, after a working migration to other host, you don't want to
allow "cont" on the source node (it target has ever changed anything, it
would give disk corruption).

But my suggestion to disable "cont" after that got complains that people
wanted a "I_know_what_I_am_doing_cont". (not the real syntax).  Perhaps
it is time to revise this issue?

Later, Juan.

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> Am 21.04.2010 17:08, schrieb Luiz Capitulino:

> A different status that disallows cont sounds good. But exit() would
> work for me as well and is probably easier. However, I'm not sure if it
> would work well for management.

I think that management would prefer it.  It is developers who try to
loadvm several times on the same machine.  For management, if loadvm
fails, having to restart qemu is the less of its problems, they already
have infrastructure to kill process and rerun (probably with something
tweeaked).

>>  Defining the right behavior and deciding what to expose have been
>> proven very difficult tasks in the QMP world.
>
> There's nothing QMP specific about it. The problem has always been
> there, QMP just means that you involuntarily get to review all of it.

And we are thankful for the review :)

Later, Juan.

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Wed, Apr 21, 2010 at 12:12:14PM -0300, Luiz Capitulino wrote:
>> On Wed, 21 Apr 2010 18:42:38 +0400 (MSD)
>> malc <av1474@comtv.ru> wrote:
>> 
>> > On Wed, 21 Apr 2010, Kevin Wolf wrote:
>> > 
>> > > Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
>> > > > On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
>> > > >> There are error handling functions in QEMU which print errno codes
>> > > >> to the user. While it's debatable if this is good from a user
>> > > >> perspective, sometimes it's the best you can do because it's what
>> > > >> system calls return and this is also useful for debugging.
>> > > >>
>> > > >> So, we need a way to expose those codes in QMP. We can't use the
>> > > >> codes themselfs because they may vary between systems.
>> > > >>
>> > > >> The best solution I can think of is returning the string
>> > > >> representation of the name. For example, EIO becomes "EIO".
>> > > >>
>> > > >> This is what get_errno_string() does.
>> > > >>
>> > > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > > >> ---
>> > > >>  qemu-error.c |   25 +++++++++++++++++++++++++
>> > > >>  qemu-error.h |    1 +
>> > > >>  2 files changed, 26 insertions(+), 0 deletions(-)
>> > > >>
>> > > >> diff --git a/qemu-error.c b/qemu-error.c
>> > > >> index 5a35e7c..55ce133 100644
>> > > >> --- a/qemu-error.c
>> > > >> +++ b/qemu-error.c
>> > > >> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
>> > > >>      va_end(ap);
>> > > >>      error_printf("\n");
>> > > >>  }
>> > > >> +
>> > > >> +/*
>> > > >> + * This is probably only useful for QMP
>> > > >> + */
>> > > >> +const char *get_errno_string(int err)
>> > > >> +{
>> > > >> +    assert(err < 0);
>> > > >> +
>> > > >> +    switch (err) {
>> > > >> +    case -EINVAL:
>> > > >> +        return "EINVAL";
>> > > >> +    case -EIO:
>> > > >> +        return "EIO";
>> > > >> +    case -ENOENT:
>> > > >> +        return "ENOENT";
>> > > >> +    case -ENOMEDIUM:
>> > > >> +        return "ENOMEDIUM";
>> > > >> +    case -ENOTSUP:
>> > > >> +        return "ENOTSUP";
>> > > >> +    default:
>> > > >> +        return "unknown";
>> > > >> +    }
>> > > >> +
>> > > >> +    abort();
>> > > >> +}
>> > > > 
>> > > > Wouldn't it be nicer to return strerror_r()  output instead of errno
>> > > > names ?
>> > > 
>> > > I agree. And it would be more complete, too.
>> > 
>> > OTOH it has a problem of returning translated messages (subject to
>> > LC_MESSAGES value).
>> 
>>  Exactly, and I'm not sure if there's anything that ensure they're
>> exactly the same among different systems.
>
> I thought QMP already declared that the printable error strings are subject
> to arbitrary change at any time, which includes translation? Apps needing 
> something reliable should be hooking onto the error code.

Yes, but the value of get_errno_string() is put into the error's data
object, where the "client should not attempt to parse this" clause does
not apply.

We need to decide whether clients need to know the errno or not.

If they do, we need to encode errno in a way that doesn't depend on
QEMU's host system.  The encoding proposed by Luiz is as good as any, I
think.

If they don't, then substituting text obtained from strerror_r() into
desc is the way to go.  There's currently no way to do that without
putting something it the error's data object, so that would need fixing.
Simple: don't send members whose names start with '_' across the wire.

Re: [Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Jamie Lokier]

Juan Quintela wrote:
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Wed, 21 Apr 2010 15:28:16 +0200
> > Kevin Wolf <kwolf@redhat.com> wrote:
> I tried a variation of this in the past, and was not a clear agreement.
> 
> Basically, after a working migration to other host, you don't want to
> allow "cont" on the source node (it target has ever changed anything, it
> would give disk corruption).

This is not true if the target is using a copy of the disk.

Making copies is cheap on some hosts (Linux btrfs with it's COW features).

Forking a guest can be handy for testing things, starting from a known
run state.  The only thing to get confused is networking because of
duplicate addresses, and there are host-side ways around that (Linux
network namespaces).

If I understand correctly, we can already do this by migrating to a
file and copying the files.  There's no reason to block the live
equivalent, provided there is a way to copy the disk image when it's
quiesced.

So it's wrong to block "cont" on the source, but 
"cont --I_know_what_I_am_doing" might be good advice :-)

> But my suggestion to disable "cont" after that got complains that people
> wanted a "I_know_what_I_am_doing_cont". (not the real syntax).  Perhaps
> it is time to revise this issue?

-- Jamie

[Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM

[Luiz Capitulino]

On Wed, 21 Apr 2010 17:42:54 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 21.04.2010 17:39, schrieb Juan Quintela:
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >> 2. Don't keep the VM paused when recovery is possible
> >>
> >>  If you can fix that, it's ok to me: I'll drop this and the next patch.
> >>
> >>  Otherwise I'll have to insist on the split.
> > 
> > Re-read my email.   At this point, nothing is fixable :(  After doing
> > the 1st:
> > 
> >>>             ret = bdrv_snapshot_goto(bs1, name);
> > 
> > and not returning an error -> state has changed, period.  You can't
> > restart the machine.
> 
> Right, at this point. But the most likely error for bdrv_snapshot_goto
> is that the snapshot doesn't even exist. This is something that you can
> check before you change any state. I think this is what Luiz meant (at
> least it is what I said and he agreed).

 Yes, that's it.

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Luiz Capitulino]

On Wed, 21 Apr 2010 19:13:16 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Wed, Apr 21, 2010 at 12:12:14PM -0300, Luiz Capitulino wrote:
> >> On Wed, 21 Apr 2010 18:42:38 +0400 (MSD)
> >> malc <av1474@comtv.ru> wrote:
> >> 
> >> > On Wed, 21 Apr 2010, Kevin Wolf wrote:
> >> > 
> >> > > Am 21.04.2010 10:28, schrieb Daniel P. Berrange:
> >> > > > On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
> >> > > >> There are error handling functions in QEMU which print errno codes
> >> > > >> to the user. While it's debatable if this is good from a user
> >> > > >> perspective, sometimes it's the best you can do because it's what
> >> > > >> system calls return and this is also useful for debugging.
> >> > > >>
> >> > > >> So, we need a way to expose those codes in QMP. We can't use the
> >> > > >> codes themselfs because they may vary between systems.
> >> > > >>
> >> > > >> The best solution I can think of is returning the string
> >> > > >> representation of the name. For example, EIO becomes "EIO".
> >> > > >>
> >> > > >> This is what get_errno_string() does.
> >> > > >>
> >> > > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > > >> ---
> >> > > >>  qemu-error.c |   25 +++++++++++++++++++++++++
> >> > > >>  qemu-error.h |    1 +
> >> > > >>  2 files changed, 26 insertions(+), 0 deletions(-)
> >> > > >>
> >> > > >> diff --git a/qemu-error.c b/qemu-error.c
> >> > > >> index 5a35e7c..55ce133 100644
> >> > > >> --- a/qemu-error.c
> >> > > >> +++ b/qemu-error.c
> >> > > >> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
> >> > > >>      va_end(ap);
> >> > > >>      error_printf("\n");
> >> > > >>  }
> >> > > >> +
> >> > > >> +/*
> >> > > >> + * This is probably only useful for QMP
> >> > > >> + */
> >> > > >> +const char *get_errno_string(int err)
> >> > > >> +{
> >> > > >> +    assert(err < 0);
> >> > > >> +
> >> > > >> +    switch (err) {
> >> > > >> +    case -EINVAL:
> >> > > >> +        return "EINVAL";
> >> > > >> +    case -EIO:
> >> > > >> +        return "EIO";
> >> > > >> +    case -ENOENT:
> >> > > >> +        return "ENOENT";
> >> > > >> +    case -ENOMEDIUM:
> >> > > >> +        return "ENOMEDIUM";
> >> > > >> +    case -ENOTSUP:
> >> > > >> +        return "ENOTSUP";
> >> > > >> +    default:
> >> > > >> +        return "unknown";
> >> > > >> +    }
> >> > > >> +
> >> > > >> +    abort();
> >> > > >> +}
> >> > > > 
> >> > > > Wouldn't it be nicer to return strerror_r()  output instead of errno
> >> > > > names ?
> >> > > 
> >> > > I agree. And it would be more complete, too.
> >> > 
> >> > OTOH it has a problem of returning translated messages (subject to
> >> > LC_MESSAGES value).
> >> 
> >>  Exactly, and I'm not sure if there's anything that ensure they're
> >> exactly the same among different systems.
> >
> > I thought QMP already declared that the printable error strings are subject
> > to arbitrary change at any time, which includes translation? Apps needing 
> > something reliable should be hooking onto the error code.
> 
> Yes, but the value of get_errno_string() is put into the error's data
> object, where the "client should not attempt to parse this" clause does
> not apply.
> 
> We need to decide whether clients need to know the errno or not.
> 
> If they do, we need to encode errno in a way that doesn't depend on
> QEMU's host system.  The encoding proposed by Luiz is as good as any, I
> think.
> 
> If they don't, then substituting text obtained from strerror_r() into
> desc is the way to go.  There's currently no way to do that without
> putting something it the error's data object, so that would need fixing.
> Simple: don't send members whose names start with '_' across the wire.

 That's even better indeed, actually we could do both: put the errno
in the data object _and_ the strerror_r() text in desc.

[Qemu-devel] Re: [PATCH 18/22] savevm: Convert do_delvm() to QObject, QError

[Luiz Capitulino]

On Wed, 21 Apr 2010 16:18:18 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 20.04.2010 23:09, schrieb Luiz Capitulino:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qemu-monitor.hx |    3 ++-
> >  savevm.c        |   14 ++++++++++----
> >  sysemu.h        |    2 +-
> >  3 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> > index 5ea5748..71cb1a2 100644
> > --- a/qemu-monitor.hx
> > +++ b/qemu-monitor.hx
> > @@ -274,7 +274,8 @@ ETEXI
> >          .args_type  = "name:s",
> >          .params     = "tag|id",
> >          .help       = "delete a VM snapshot from its tag or id",
> > -        .mhandler.cmd = do_delvm,
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = do_delvm,
> >      },
> >  
> >  STEXI
> > diff --git a/savevm.c b/savevm.c
> > index 643273e..031eeff 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name)
> >      return 0;
> >  }
> >  
> > -void do_delvm(Monitor *mon, const QDict *qdict)
> > +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> > +    int ret;
> >      DriveInfo *dinfo;
> >      BlockDriverState *bs, *bs1;
> >      const char *name = qdict_get_str(qdict, "name");
> >  
> >      bs = get_bs_snapshots();
> >      if (!bs) {
> > -        monitor_printf(mon, "No block device supports snapshots\n");
> > -        return;
> > +        qerror_report(QERR_SNAPSHOT_NO_DEVICE);
> > +        return -1;
> >      }
> >  
> > +    ret = -1;
> > +
> >      QTAILQ_FOREACH(dinfo, &drives, next) {
> >          bs1 = dinfo->bdrv;
> >          if (bdrv_has_snapshot(bs1)) {
> > -            delete_snapshot(bs1, name);
> > +            /* FIXME: will report multiple failures in QMP */
> > +            ret = delete_snapshot(bs1, name);
> >          }
> >      }
> > +
> > +    return (ret < 0 ? -1 : 0);
> 
> Doesn't this return success when the first drive fails and the second
> one succeeds?

 Yes, but what's the real status when this happens? Did delvm
succeed or not?

 I think users will ask the same as we'll print error messages.

 Another question is: is it acceptable to return on the first
error?

[Qemu-devel] Re: [PATCH 18/22] savevm: Convert do_delvm() to QObject, QError

[Kevin Wolf]

Am 22.04.2010 15:48, schrieb Luiz Capitulino:
> On Wed, 21 Apr 2010 16:18:18 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 20.04.2010 23:09, schrieb Luiz Capitulino:
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>  qemu-monitor.hx |    3 ++-
>>>  savevm.c        |   14 ++++++++++----
>>>  sysemu.h        |    2 +-
>>>  3 files changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
>>> index 5ea5748..71cb1a2 100644
>>> --- a/qemu-monitor.hx
>>> +++ b/qemu-monitor.hx
>>> @@ -274,7 +274,8 @@ ETEXI
>>>          .args_type  = "name:s",
>>>          .params     = "tag|id",
>>>          .help       = "delete a VM snapshot from its tag or id",
>>> -        .mhandler.cmd = do_delvm,
>>> +        .user_print = monitor_user_noop,
>>> +        .mhandler.cmd_new = do_delvm,
>>>      },
>>>  
>>>  STEXI
>>> diff --git a/savevm.c b/savevm.c
>>> index 643273e..031eeff 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -1815,24 +1815,30 @@ int load_vmstate(const char *name)
>>>      return 0;
>>>  }
>>>  
>>> -void do_delvm(Monitor *mon, const QDict *qdict)
>>> +int do_delvm(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>  {
>>> +    int ret;
>>>      DriveInfo *dinfo;
>>>      BlockDriverState *bs, *bs1;
>>>      const char *name = qdict_get_str(qdict, "name");
>>>  
>>>      bs = get_bs_snapshots();
>>>      if (!bs) {
>>> -        monitor_printf(mon, "No block device supports snapshots\n");
>>> -        return;
>>> +        qerror_report(QERR_SNAPSHOT_NO_DEVICE);
>>> +        return -1;
>>>      }
>>>  
>>> +    ret = -1;
>>> +
>>>      QTAILQ_FOREACH(dinfo, &drives, next) {
>>>          bs1 = dinfo->bdrv;
>>>          if (bdrv_has_snapshot(bs1)) {
>>> -            delete_snapshot(bs1, name);
>>> +            /* FIXME: will report multiple failures in QMP */
>>> +            ret = delete_snapshot(bs1, name);
>>>          }
>>>      }
>>> +
>>> +    return (ret < 0 ? -1 : 0);
>>
>> Doesn't this return success when the first drive fails and the second
>> one succeeds?
> 
>  Yes, but what's the real status when this happens? Did delvm
> succeed or not?
> 
>  I think users will ask the same as we'll print error messages.

Don't know. We probably need ternary logic. :-)

I think I'd call it an error. But either way, that it depends on the
order of disks just doesn't feel right. The status of "disk1 fails and
disk2 succeeds" should not be different from "disk1 succeeds and disk2
fails".

>  Another question is: is it acceptable to return on the first
> error?

I guess it is.

Kevin

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Anthony Liguori]

On 04/21/2010 03:28 AM, Daniel P. Berrange wrote:
> On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
>    
>> There are error handling functions in QEMU which print errno codes
>> to the user. While it's debatable if this is good from a user
>> perspective, sometimes it's the best you can do because it's what
>> system calls return and this is also useful for debugging.
>>
>> So, we need a way to expose those codes in QMP. We can't use the
>> codes themselfs because they may vary between systems.
>>
>> The best solution I can think of is returning the string
>> representation of the name. For example, EIO becomes "EIO".
>>
>> This is what get_errno_string() does.
>>
>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>> ---
>>   qemu-error.c |   25 +++++++++++++++++++++++++
>>   qemu-error.h |    1 +
>>   2 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-error.c b/qemu-error.c
>> index 5a35e7c..55ce133 100644
>> --- a/qemu-error.c
>> +++ b/qemu-error.c
>> @@ -207,3 +207,28 @@ void error_report(const char *fmt, ...)
>>       va_end(ap);
>>       error_printf("\n");
>>   }
>> +
>> +/*
>> + * This is probably only useful for QMP
>> + */
>> +const char *get_errno_string(int err)
>> +{
>> +    assert(err<  0);
>> +
>> +    switch (err) {
>> +    case -EINVAL:
>> +        return "EINVAL";
>> +    case -EIO:
>> +        return "EIO";
>> +    case -ENOENT:
>> +        return "ENOENT";
>> +    case -ENOMEDIUM:
>> +        return "ENOMEDIUM";
>> +    case -ENOTSUP:
>> +        return "ENOTSUP";
>> +    default:
>> +        return "unknown";
>> +    }
>> +
>> +    abort();
>> +}
>>      
> Wouldn't it be nicer to return strerror_r()  output instead of errno
> names ?
>    

Both are equally wrong :-)

QMP should insult users from underlying platform quirks.  We should 
translate errnos to appropriate QMP error types.

Regards,

Anthony Liguori

> Daniel
>    

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 04/21/2010 03:28 AM, Daniel P. Berrange wrote:
>> On Tue, Apr 20, 2010 at 06:09:37PM -0300, Luiz Capitulino wrote:
[...]
>> Wouldn't it be nicer to return strerror_r()  output instead of errno
>> names ?
>>    
>
> Both are equally wrong :-)
>
> QMP should insult users from underlying platform quirks.  We should
> translate errnos to appropriate QMP error types.

While I find QMP's gold-plated errors pretty insulting, too, I wouldn't
go so far as to say QMP *should* insult its users.

SCNR

Re: [Qemu-devel] [PATCH 07/22] qemu-error: Introduce get_errno_string()

[Jamie Lokier]

Anthony Liguori wrote:
> QMP should insult users from underlying platform quirks.  We should 
> translate errnos to appropriate QMP error types.

Fair enough.  What should it do when the platform returns an errno
value that qemu doesn't know about, and wants to pass to the QMP caller?

-- Jamie