Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

[Philippe Mathieu-Daudé]

On 4/22/20 3:07 PM, Markus Armbruster wrote:
The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
first to check_suspend_mode(), then to acquire_privilege(), then to
execute_async().  Continuing after errors here can only end in tears.
For instance, we risk tripping error_setv()'s assertion.

Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
Fixes: f54603b6aa765514b2519e74114a2f417759d727
Cc: Michael Roth <mdr...@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <arm...@redhat.com>
---
  qga/commands-win32.c | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9717a8d52d..5ba56327dd 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
  
      *mode = GUEST_SUSPEND_MODE_DISK;
      check_suspend_mode(*mode, &local_err);
+    if (local_err) {
+        goto out;
+    }
      acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+    if (local_err) {
+        goto out;
+    }
      execute_async(do_suspend, mode, &local_err);
  
+out:
      if (local_err) {

https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is 
slightly different by removing the if() check.

          error_propagate(errp, local_err);
          g_free(mode);
@@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
  
      *mode = GUEST_SUSPEND_MODE_RAM;
      check_suspend_mode(*mode, &local_err);
+    if (local_err) {
+        goto out;
+    }
      acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+    if (local_err) {
+        goto out;
+    }
      execute_async(do_suspend, mode, &local_err);
  
+out:
      if (local_err) {
          error_propagate(errp, local_err);
          g_free(mode);