[Qemu-devel] [PATCH v2 0/6] QGA: systemd hibernate/suspend/hybrid-sleep

[Qemu-devel] [PATCH v2 0/6] QGA: systemd hibernate/suspend/hybrid-sleep

[Daniel Henrique Barboza]

changes in v2 from Marc-Andre Lureau review:
- use error_free() accordingly
- use g_spawn_sync() instead of fork() in run_process_child()
- previous version link:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05499.html


This series adds systemd suspend support for QGA. Some newer
guests don't have pmutils anymore, leaving us with just the
Linux state file mechanism to suspend the guest OS, which does
not support hybrid-sleep. With this implementation, QGA is
now able to hybrid suspend newer guests again.

Most of the patches are cleanups in the existing suspend code,
aiming at both simplifying it and making it easier to extend
it with systemd.


Daniel Henrique Barboza (6):
  qga: refactoring qmp_guest_suspend_* functions
  qga: bios_supports_mode: decoupling pm-utils and sys logic
  qga: guest_suspend: decoupling pm-utils and sys logic
  qga: removing switch statements, adding run_process_child
  qga: systemd hibernate/suspend/hybrid-sleep support
  qga: removing bios_supports_mode

 qga/commands-posix.c | 316 ++++++++++++++++++++++++++++---------------
 1 file changed, 208 insertions(+), 108 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/6] qga: refactoring qmp_guest_suspend_* functions

[Daniel Henrique Barboza]

To be able to add new suspend mechanisms we need to detach
the existing QMP functions from the current implementation
specifics.

At this moment we have functions such as qmp_guest_suspend_ram
calling bios_suspend_mode and guest_suspend passing the
pmutils command and arguments as parameters. This patch
removes this logic from the QMP functions, moving them to
the respective functions that will have to deal with which
binary to use.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 87 ++++++++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 32 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index eae817191b..63c49791a4 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1438,15 +1438,38 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 #define LINUX_SYS_STATE_FILE "/sys/power/state"
 #define SUSPEND_SUPPORTED 0
 #define SUSPEND_NOT_SUPPORTED 1
+#define SUSPEND_MODE_DISK 1
+#define SUSPEND_MODE_RAM 2
+#define SUSPEND_MODE_HYBRID 3
 
-static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
-                               const char *sysfile_str, Error **errp)
+static void bios_supports_mode(int suspend_mode, Error **errp)
 {
     Error *local_err = NULL;
+    const char *pmutils_arg, *sysfile_str;
+    const char *pmutils_bin = "pm-is-supported";
     char *pmutils_path;
     pid_t pid;
     int status;
 
+    switch (suspend_mode) {
+
+    case SUSPEND_MODE_DISK:
+        pmutils_arg = "--hibernate";
+        sysfile_str = "disk";
+        break;
+    case SUSPEND_MODE_RAM:
+        pmutils_arg = "--suspend";
+        sysfile_str = "mem";
+        break;
+    case SUSPEND_MODE_HYBRID:
+        pmutils_arg = "--suspend-hybrid";
+        sysfile_str = NULL;
+        break;
+    default:
+        error_setg(errp, "guest suspend mode not supported");
+        return;
+    }
+
     pmutils_path = g_find_program_in_path(pmutils_bin);
 
     pid = fork();
@@ -1523,14 +1546,39 @@ out:
     g_free(pmutils_path);
 }
 
-static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
-                          Error **errp)
+static void guest_suspend(int suspend_mode, Error **errp)
 {
     Error *local_err = NULL;
+    const char *pmutils_bin, *sysfile_str;
     char *pmutils_path;
     pid_t pid;
     int status;
 
+    bios_supports_mode(suspend_mode, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    switch (suspend_mode) {
+
+    case SUSPEND_MODE_DISK:
+        pmutils_bin = "pm-hibernate";
+        sysfile_str = "disk";
+        break;
+    case SUSPEND_MODE_RAM:
+        pmutils_bin = "pm-suspend";
+        sysfile_str = "mem";
+        break;
+    case SUSPEND_MODE_HYBRID:
+        pmutils_bin = "pm-suspend-hybrid";
+        sysfile_str = NULL;
+        break;
+    default:
+        error_setg(errp, "unknown guest suspend mode");
+        return;
+    }
+
     pmutils_path = g_find_program_in_path(pmutils_bin);
 
     pid = fork();
@@ -1593,42 +1641,17 @@ out:
 
 void qmp_guest_suspend_disk(Error **errp)
 {
-    Error *local_err = NULL;
-
-    bios_supports_mode("pm-is-supported", "--hibernate", "disk", &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    guest_suspend("pm-hibernate", "disk", errp);
+    guest_suspend(SUSPEND_MODE_DISK, errp);
 }
 
 void qmp_guest_suspend_ram(Error **errp)
 {
-    Error *local_err = NULL;
-
-    bios_supports_mode("pm-is-supported", "--suspend", "mem", &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    guest_suspend("pm-suspend", "mem", errp);
+    guest_suspend(SUSPEND_MODE_RAM, errp);
 }
 
 void qmp_guest_suspend_hybrid(Error **errp)
 {
-    Error *local_err = NULL;
-
-    bios_supports_mode("pm-is-supported", "--suspend-hybrid", NULL,
-                       &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    guest_suspend("pm-suspend-hybrid", NULL, errp);
+    guest_suspend(SUSPEND_MODE_HYBRID, errp);
 }
 
 static GuestNetworkInterfaceList *
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/6] qga: bios_supports_mode: decoupling pm-utils and sys logic

[Daniel Henrique Barboza]

In bios_supports_mode there is a verification to assert if
the chosen suspend mode is supported by the pmutils tools and,
if not, we see if the Linux sys state files supports it.

This verification is done in the same function, one after
the other, and it works for now. But, when adding a new
suspend mechanism that will not necessarily follow the same
return 0 or 1 logic of pmutils, this code will be hard
to deal with.

This patch decouple the two existing logics into their own
functions, pmutils_supports_mode and linux_sys_state_supports_mode,
which in turn are used inside bios_support_mode. The existing
logic is kept but now it's easier to extend it.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 116 +++++++++++++++++++++++++------------------
 1 file changed, 68 insertions(+), 48 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 63c49791a4..89ffd8dc88 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1442,75 +1442,43 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 #define SUSPEND_MODE_RAM 2
 #define SUSPEND_MODE_HYBRID 3
 
-static void bios_supports_mode(int suspend_mode, Error **errp)
+static bool pmutils_supports_mode(int suspend_mode, Error **errp)
 {
     Error *local_err = NULL;
-    const char *pmutils_arg, *sysfile_str;
+    const char *pmutils_arg;
     const char *pmutils_bin = "pm-is-supported";
     char *pmutils_path;
     pid_t pid;
     int status;
+    bool ret = false;
 
     switch (suspend_mode) {
 
     case SUSPEND_MODE_DISK:
         pmutils_arg = "--hibernate";
-        sysfile_str = "disk";
         break;
     case SUSPEND_MODE_RAM:
         pmutils_arg = "--suspend";
-        sysfile_str = "mem";
         break;
     case SUSPEND_MODE_HYBRID:
         pmutils_arg = "--suspend-hybrid";
-        sysfile_str = NULL;
         break;
     default:
-        error_setg(errp, "guest suspend mode not supported");
-        return;
+        return ret;
     }
 
     pmutils_path = g_find_program_in_path(pmutils_bin);
+    if (!pmutils_path) {
+        return ret;
+    }
 
     pid = fork();
     if (!pid) {
-        char buf[32]; /* hopefully big enough */
-        ssize_t ret;
-        int fd;
-
         setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        if (pmutils_path) {
-            execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
-        }
-
+        execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
         /*
-         * If we get here either pm-utils is not installed or execle() has
-         * failed. Let's try the manual method if the caller wants it.
+         * If we get here execle() has failed.
          */
-
-        if (!sysfile_str) {
-            _exit(SUSPEND_NOT_SUPPORTED);
-        }
-
-        fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
-        if (fd < 0) {
-            _exit(SUSPEND_NOT_SUPPORTED);
-        }
-
-        ret = read(fd, buf, sizeof(buf)-1);
-        if (ret <= 0) {
-            _exit(SUSPEND_NOT_SUPPORTED);
-        }
-        buf[ret] = '\0';
-
-        if (strstr(buf, sysfile_str)) {
-            _exit(SUSPEND_SUPPORTED);
-        }
-
         _exit(SUSPEND_NOT_SUPPORTED);
     } else if (pid < 0) {
         error_setg_errno(errp, errno, "failed to create child process");
@@ -1523,17 +1491,11 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
         goto out;
     }
 
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        goto out;
-    }
-
     switch (WEXITSTATUS(status)) {
     case SUSPEND_SUPPORTED:
+        ret = true;
         goto out;
     case SUSPEND_NOT_SUPPORTED:
-        error_setg(errp,
-                   "the requested suspend mode is not supported by the guest");
         goto out;
     default:
         error_setg(errp,
@@ -1544,6 +1506,64 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
 
 out:
     g_free(pmutils_path);
+    return ret;
+}
+
+static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
+{
+    const char *sysfile_str;
+    char buf[32]; /* hopefully big enough */
+    int fd;
+    ssize_t ret;
+
+    switch (suspend_mode) {
+
+    case SUSPEND_MODE_DISK:
+        sysfile_str = "disk";
+        break;
+    case SUSPEND_MODE_RAM:
+        sysfile_str = "mem";
+        break;
+    default:
+        return false;
+    }
+
+    fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
+    if (fd < 0) {
+        return false;
+    }
+
+    ret = read(fd, buf, sizeof(buf) - 1);
+    if (ret <= 0) {
+        return false;
+    }
+    buf[ret] = '\0';
+
+    if (strstr(buf, sysfile_str)) {
+        return true;
+    }
+    return false;
+}
+
+static void bios_supports_mode(int suspend_mode, Error **errp)
+{
+    Error *local_err = NULL;
+    bool ret;
+
+    ret = pmutils_supports_mode(suspend_mode, &local_err);
+    if (ret) {
+        return;
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    ret = linux_sys_state_supports_mode(suspend_mode, errp);
+    if (!ret) {
+        error_setg(errp,
+                   "the requested suspend mode is not supported by the guest");
+        return;
+    }
 }
 
 static void guest_suspend(int suspend_mode, Error **errp)
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/6] qga: guest_suspend: decoupling pm-utils and sys logic

[Daniel Henrique Barboza]

Following the same logic of the previous patch, let's also
decouple the suspend logic from guest_suspend into specialized
functions, one for each strategy we support at this moment.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 170 +++++++++++++++++++++++++++----------------
 1 file changed, 108 insertions(+), 62 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 89ffd8dc88..4b6fbd9b80 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1509,6 +1509,65 @@ out:
     return ret;
 }
 
+static void pmutils_suspend(int suspend_mode, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *pmutils_bin;
+    char *pmutils_path;
+    pid_t pid;
+    int status;
+
+    switch (suspend_mode) {
+
+    case SUSPEND_MODE_DISK:
+        pmutils_bin = "pm-hibernate";
+        break;
+    case SUSPEND_MODE_RAM:
+        pmutils_bin = "pm-suspend";
+        break;
+    case SUSPEND_MODE_HYBRID:
+        pmutils_bin = "pm-suspend-hybrid";
+        break;
+    default:
+        error_setg(errp, "unknown guest suspend mode");
+        return;
+    }
+
+    pmutils_path = g_find_program_in_path(pmutils_bin);
+    if (!pmutils_path) {
+        error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
+        return;
+    }
+
+    pid = fork();
+    if (!pid) {
+        setsid();
+        execle(pmutils_path, pmutils_bin, NULL, environ);
+        /*
+         * If we get here execle() has failed.
+         */
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        goto out;
+    }
+
+    ga_wait_child(pid, &status, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
+    if (WEXITSTATUS(status)) {
+        error_setg(errp,
+                   "the helper program '%s' returned an unexpected exit status"
+                   " code (%d)", pmutils_path, WEXITSTATUS(status));
+    }
+
+out:
+    g_free(pmutils_path);
+}
+
 static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
 {
     const char *sysfile_str;
@@ -1545,64 +1604,28 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
     return false;
 }
 
-static void bios_supports_mode(int suspend_mode, Error **errp)
-{
-    Error *local_err = NULL;
-    bool ret;
-
-    ret = pmutils_supports_mode(suspend_mode, &local_err);
-    if (ret) {
-        return;
-    }
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    ret = linux_sys_state_supports_mode(suspend_mode, errp);
-    if (!ret) {
-        error_setg(errp,
-                   "the requested suspend mode is not supported by the guest");
-        return;
-    }
-}
-
-static void guest_suspend(int suspend_mode, Error **errp)
+static void linux_sys_state_suspend(int suspend_mode, Error **errp)
 {
     Error *local_err = NULL;
-    const char *pmutils_bin, *sysfile_str;
-    char *pmutils_path;
+    const char *sysfile_str;
     pid_t pid;
     int status;
 
-    bios_supports_mode(suspend_mode, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     switch (suspend_mode) {
 
     case SUSPEND_MODE_DISK:
-        pmutils_bin = "pm-hibernate";
         sysfile_str = "disk";
         break;
     case SUSPEND_MODE_RAM:
-        pmutils_bin = "pm-suspend";
         sysfile_str = "mem";
         break;
-    case SUSPEND_MODE_HYBRID:
-        pmutils_bin = "pm-suspend-hybrid";
-        sysfile_str = NULL;
-        break;
     default:
         error_setg(errp, "unknown guest suspend mode");
         return;
     }
 
-    pmutils_path = g_find_program_in_path(pmutils_bin);
-
     pid = fork();
-    if (pid == 0) {
+    if (!pid) {
         /* child */
         int fd;
 
@@ -1611,19 +1634,6 @@ static void guest_suspend(int suspend_mode, Error **errp)
         reopen_fd_to_null(1);
         reopen_fd_to_null(2);
 
-        if (pmutils_path) {
-            execle(pmutils_path, pmutils_bin, NULL, environ);
-        }
-
-        /*
-         * If we get here either pm-utils is not installed or execle() has
-         * failed. Let's try the manual method if the caller wants it.
-         */
-
-        if (!sysfile_str) {
-            _exit(EXIT_FAILURE);
-        }
-
         fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
         if (fd < 0) {
             _exit(EXIT_FAILURE);
@@ -1636,27 +1646,63 @@ static void guest_suspend(int suspend_mode, Error **errp)
         _exit(EXIT_SUCCESS);
     } else if (pid < 0) {
         error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
+        return;
     }
 
     ga_wait_child(pid, &status, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
-    }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        goto out;
+        return;
     }
 
     if (WEXITSTATUS(status)) {
         error_setg(errp, "child process has failed to suspend");
-        goto out;
     }
 
-out:
-    g_free(pmutils_path);
+}
+
+static void bios_supports_mode(int suspend_mode, Error **errp)
+{
+    Error *local_err = NULL;
+    bool ret;
+
+    ret = pmutils_supports_mode(suspend_mode, &local_err);
+    if (ret) {
+        return;
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    ret = linux_sys_state_supports_mode(suspend_mode, errp);
+    if (!ret) {
+        error_setg(errp,
+                   "the requested suspend mode is not supported by the guest");
+        return;
+    }
+}
+
+static void guest_suspend(int suspend_mode, Error **errp)
+{
+    Error *local_err = NULL;
+
+    bios_supports_mode(suspend_mode, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    pmutils_suspend(suspend_mode, &local_err);
+    if (!local_err) {
+        return;
+    }
+
+    error_free(local_err);
+
+    linux_sys_state_suspend(suspend_mode, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
 }
 
 void qmp_guest_suspend_disk(Error **errp)
-- 
2.17.1

[Qemu-devel] [PATCH v2 4/6] qga: removing switch statements, adding run_process_child

[Daniel Henrique Barboza]

This is a cleanup of the resulting code after detaching
pmutils and Linux sys state file logic:

- remove the SUSPEND_MODE_* macros and use an enumeration
instead. At the same time, drop the switch statements
at the start of each function and use the enumeration
index to get the right binary/argument;

- create a new function called run_process_child(). This
function uses g_spawn_sync() to execute a shell command,
returning the exit code. This is a common operation in the
pmutils functions and will be used in the systemd implementation
as well, so this function will avoid code repetition.

There are more places inside commands-posix.c where this new
run_process_child function can also be used, but one step
at a time.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 211 +++++++++++++++++--------------------------
 1 file changed, 84 insertions(+), 127 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 4b6fbd9b80..925544ae6d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1438,152 +1438,117 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 #define LINUX_SYS_STATE_FILE "/sys/power/state"
 #define SUSPEND_SUPPORTED 0
 #define SUSPEND_NOT_SUPPORTED 1
-#define SUSPEND_MODE_DISK 1
-#define SUSPEND_MODE_RAM 2
-#define SUSPEND_MODE_HYBRID 3
 
-static bool pmutils_supports_mode(int suspend_mode, Error **errp)
+typedef enum {
+    SUSPEND_MODE_DISK = 0,
+    SUSPEND_MODE_RAM = 1,
+    SUSPEND_MODE_HYBRID = 2,
+} SuspendMode;
+
+/*
+ * Executes a command in a child process using g_spawn_sync,
+ * returning an int >= 0 representing the exit status of the
+ * process.
+ *
+ * If the program wasn't found in path, returns -1.
+ *
+ * If a problem happened when creating the child process,
+ * returns -1 and errp is set.
+ */
+static int run_process_child(const char *command[], Error **errp)
 {
-    Error *local_err = NULL;
-    const char *pmutils_arg;
-    const char *pmutils_bin = "pm-is-supported";
-    char *pmutils_path;
-    pid_t pid;
-    int status;
-    bool ret = false;
+    int exit_status, spawn_flag;
+    GError *g_err = NULL;
+    bool success;
 
-    switch (suspend_mode) {
+    spawn_flag = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL |
+                 G_SPAWN_STDERR_TO_DEV_NULL;
 
-    case SUSPEND_MODE_DISK:
-        pmutils_arg = "--hibernate";
-        break;
-    case SUSPEND_MODE_RAM:
-        pmutils_arg = "--suspend";
-        break;
-    case SUSPEND_MODE_HYBRID:
-        pmutils_arg = "--suspend-hybrid";
-        break;
-    default:
-        return ret;
+    success =  g_spawn_sync(NULL, (char **)command, environ, spawn_flag,
+                            NULL, NULL, NULL, NULL,
+                            &exit_status, &g_err);
+
+    if (success) {
+        return WEXITSTATUS(exit_status);
     }
 
-    pmutils_path = g_find_program_in_path(pmutils_bin);
-    if (!pmutils_path) {
-        return ret;
+    if (g_err && (g_err->code != G_SPAWN_ERROR_NOENT)) {
+        error_setg(errp, "failed to create child process, error '%s'",
+                   g_err->message);
     }
 
-    pid = fork();
-    if (!pid) {
-        setsid();
-        execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
-        /*
-         * If we get here execle() has failed.
-         */
-        _exit(SUSPEND_NOT_SUPPORTED);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
+    g_error_free(g_err);
+    return -1;
+}
+
+static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *pmutils_args[3] = {"--hibernate", "--suspend",
+                                   "--suspend-hybrid"};
+    const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
+    int status;
+
+    status = run_process_child(cmd, &local_err);
+
+    if (status == SUSPEND_SUPPORTED) {
+        return true;
     }
 
-    ga_wait_child(pid, &status, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto out;
+    if ((status == -1) && !local_err) {
+        return false;
     }
 
-    switch (WEXITSTATUS(status)) {
-    case SUSPEND_SUPPORTED:
-        ret = true;
-        goto out;
-    case SUSPEND_NOT_SUPPORTED:
-        goto out;
-    default:
+    if (local_err) {
+        error_propagate(errp, local_err);
+    } else {
         error_setg(errp,
-                   "the helper program '%s' returned an unexpected exit status"
-                   " code (%d)", pmutils_path, WEXITSTATUS(status));
-        goto out;
+                   "the helper program '%s' returned an unexpected exit"
+                   " status code (%d)", "pm-is-supported", status);
     }
 
-out:
-    g_free(pmutils_path);
-    return ret;
+    return false;
 }
 
-static void pmutils_suspend(int suspend_mode, Error **errp)
+static void pmutils_suspend(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
-    const char *pmutils_bin;
-    char *pmutils_path;
-    pid_t pid;
+    const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
+                                       "pm-suspend-hybrid"};
+    const char *cmd[2] = {pmutils_binaries[mode], NULL};
     int status;
 
-    switch (suspend_mode) {
-
-    case SUSPEND_MODE_DISK:
-        pmutils_bin = "pm-hibernate";
-        break;
-    case SUSPEND_MODE_RAM:
-        pmutils_bin = "pm-suspend";
-        break;
-    case SUSPEND_MODE_HYBRID:
-        pmutils_bin = "pm-suspend-hybrid";
-        break;
-    default:
-        error_setg(errp, "unknown guest suspend mode");
-        return;
-    }
+    status = run_process_child(cmd, &local_err);
 
-    pmutils_path = g_find_program_in_path(pmutils_bin);
-    if (!pmutils_path) {
-        error_setg(errp, "the helper program '%s' was not found", pmutils_bin);
+    if (status == 0) {
         return;
     }
 
-    pid = fork();
-    if (!pid) {
-        setsid();
-        execle(pmutils_path, pmutils_bin, NULL, environ);
-        /*
-         * If we get here execle() has failed.
-         */
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
+    if ((status == -1) && !local_err) {
+        error_setg(errp, "the helper program '%s' was not found",
+                   pmutils_binaries[mode]);
+        return;
     }
 
-    ga_wait_child(pid, &status, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
-    }
-
-    if (WEXITSTATUS(status)) {
+    } else {
         error_setg(errp,
-                   "the helper program '%s' returned an unexpected exit status"
-                   " code (%d)", pmutils_path, WEXITSTATUS(status));
+                   "the helper program '%s' returned an unexpected exit"
+                   " status code (%d)", pmutils_binaries[mode], status);
     }
-
-out:
-    g_free(pmutils_path);
 }
 
-static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
+static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
 {
-    const char *sysfile_str;
+    const char *sysfile_strs[3] = {"disk", "mem", NULL};
+    const char *sysfile_str = sysfile_strs[mode];
     char buf[32]; /* hopefully big enough */
     int fd;
     ssize_t ret;
 
-    switch (suspend_mode) {
-
-    case SUSPEND_MODE_DISK:
-        sysfile_str = "disk";
-        break;
-    case SUSPEND_MODE_RAM:
-        sysfile_str = "mem";
-        break;
-    default:
+    if (!sysfile_str) {
+        error_setg(errp, "unknown guest suspend mode");
         return false;
     }
 
@@ -1604,22 +1569,15 @@ static bool linux_sys_state_supports_mode(int suspend_mode, Error **errp)
     return false;
 }
 
-static void linux_sys_state_suspend(int suspend_mode, Error **errp)
+static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
-    const char *sysfile_str;
+    const char *sysfile_strs[3] = {"disk", "mem", NULL};
+    const char *sysfile_str = sysfile_strs[mode];
     pid_t pid;
     int status;
 
-    switch (suspend_mode) {
-
-    case SUSPEND_MODE_DISK:
-        sysfile_str = "disk";
-        break;
-    case SUSPEND_MODE_RAM:
-        sysfile_str = "mem";
-        break;
-    default:
+    if (!sysfile_str) {
         error_setg(errp, "unknown guest suspend mode");
         return;
     }
@@ -1661,12 +1619,12 @@ static void linux_sys_state_suspend(int suspend_mode, Error **errp)
 
 }
 
-static void bios_supports_mode(int suspend_mode, Error **errp)
+static void bios_supports_mode(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
     bool ret;
 
-    ret = pmutils_supports_mode(suspend_mode, &local_err);
+    ret = pmutils_supports_mode(mode, &local_err);
     if (ret) {
         return;
     }
@@ -1674,32 +1632,31 @@ static void bios_supports_mode(int suspend_mode, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-    ret = linux_sys_state_supports_mode(suspend_mode, errp);
+    ret = linux_sys_state_supports_mode(mode, &local_err);
     if (!ret) {
         error_setg(errp,
                    "the requested suspend mode is not supported by the guest");
-        return;
     }
 }
 
-static void guest_suspend(int suspend_mode, Error **errp)
+static void guest_suspend(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
 
-    bios_supports_mode(suspend_mode, &local_err);
+    bios_supports_mode(mode, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    pmutils_suspend(suspend_mode, &local_err);
+    pmutils_suspend(mode, &local_err);
     if (!local_err) {
         return;
     }
 
     error_free(local_err);
 
-    linux_sys_state_suspend(suspend_mode, &local_err);
+    linux_sys_state_suspend(mode, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
     }
-- 
2.17.1

[Qemu-devel] [PATCH v2 5/6] qga: systemd hibernate/suspend/hybrid-sleep support

[Daniel Henrique Barboza]

pmutils isn't being supported by newer OSes like Fedora 27
or Mint. This means that the only suspend option QGA offers
for these guests are writing directly into the Linux sys state
file. This also means that QGA also loses the ability to do
hybrid suspend in those guests - this suspend mode is only
available when using pmutils.

Newer guests can use systemd facilities to do all the suspend
types QGA supports. The mapping in comparison with pmutils is:

- pm-hibernate -> systemctl hibernate
- pm-suspend -> systemctl suspend
- pm-suspend-hybrid -> systemctl hybrid-sleep

To discover whether systemd supports these functions, we inspect
the status of the services that implements them.

With this patch, we can offer hybrid suspend again for newer
guests that do not have pmutils support anymore.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 925544ae6d..3546b74fed 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1481,6 +1481,63 @@ static int run_process_child(const char *command[], Error **errp)
     return -1;
 }
 
+static bool systemd_supports_mode(SuspendMode mode, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *systemctl_args[3] = {"systemd-hibernate", "systemd-suspend",
+                                     "systemd-hybrid-sleep"};
+    const char *cmd[4] = {"systemctl", "status", systemctl_args[mode], NULL};
+    int status;
+
+    status = run_process_child(cmd, &local_err);
+
+    /*
+     * systemctl status uses LSB return codes so we can expect
+     * status > 0 and be ok. To assert if the guest has support
+     * for the selected suspend mode, status should be < 4. 4 is
+     * the code for unknown service status, the return value when
+     * the service does not exist. A common value is status = 3
+     * (program is not running).
+     */
+    if (status > 0 && status < 4) {
+        return true;
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+
+    return false;
+}
+
+static void systemd_suspend(SuspendMode mode, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *systemctl_args[3] = {"hibernate", "suspend", "hybrid-sleep"};
+    const char *cmd[3] = {"systemctl", systemctl_args[mode], NULL};
+    int status;
+
+    status = run_process_child(cmd, &local_err);
+
+    if (status == 0) {
+        return;
+    }
+
+    if ((status == -1) && !local_err) {
+        error_setg(errp, "the helper program 'systemctl %s' was not found",
+                   systemctl_args[mode]);
+        return;
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+    } else {
+        error_setg(errp, "the helper program 'systemctl %s' returned an "
+                   "unexpected exit status code (%d)",
+                   systemctl_args[mode], status);
+    }
+}
+
 static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
@@ -1624,6 +1681,14 @@ static void bios_supports_mode(SuspendMode mode, Error **errp)
     Error *local_err = NULL;
     bool ret;
 
+    ret = systemd_supports_mode(mode, &local_err);
+    if (ret) {
+        return;
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     ret = pmutils_supports_mode(mode, &local_err);
     if (ret) {
         return;
@@ -1649,6 +1714,13 @@ static void guest_suspend(SuspendMode mode, Error **errp)
         return;
     }
 
+    systemd_suspend(mode, &local_err);
+    if (!local_err) {
+        return;
+    }
+
+    error_free(local_err);
+
     pmutils_suspend(mode, &local_err);
     if (!local_err) {
         return;
-- 
2.17.1

[Qemu-devel] [PATCH v2 6/6] qga: removing bios_supports_mode

[Daniel Henrique Barboza]

bios_support_mode verifies if the guest has support for a certain
suspend mode but it doesn't inform back which suspend tool
provides it. The caller, guest_suspend, executes all suspend
strategies in order again.

After adding systemd suspend support, bios_support_mode now will
verify for support for systemd, then pmutils, then Linux sys state
file. In a worst case scenario where both systemd and pmutils isn't
supported but Linux sys state is:

- bios_supports_mode will check for systemd, then pmutils, then
Linux sys state. It will tell guest_suspend that there is support,
but it will not tell who provides it;

- guest_suspend will try to execute (and fail) systemd suspend,
then pmutils suspend, to only then use the Linux sys suspend.
The time spent executing systemd and pmutils suspend was wasted
and could be avoided, but only bios_support_mode knew it but
didn't inform it back.

A quicker approach is to nuke bios_supports_mode and control
whether we found support at all with a bool flag inside
guest_suspend. guest_suspend will search for suspend support
and execute it as soon as possible. If the a given suspend
mechanism fails, continue to the next. If no suspend
support is found, the "not supported" message is still being
sent back to the user.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qga/commands-posix.c | 54 +++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3546b74fed..d0d38cb7d9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1676,60 +1676,42 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
 
 }
 
-static void bios_supports_mode(SuspendMode mode, Error **errp)
-{
-    Error *local_err = NULL;
-    bool ret;
-
-    ret = systemd_supports_mode(mode, &local_err);
-    if (ret) {
-        return;
-    }
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    ret = pmutils_supports_mode(mode, &local_err);
-    if (ret) {
-        return;
-    }
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    ret = linux_sys_state_supports_mode(mode, &local_err);
-    if (!ret) {
-        error_setg(errp,
-                   "the requested suspend mode is not supported by the guest");
-    }
-}
-
 static void guest_suspend(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
+    bool mode_supported = false;
 
-    bios_supports_mode(mode, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
+    if (systemd_supports_mode(mode, &local_err)) {
+        mode_supported = true;
+        systemd_suspend(mode, &local_err);
     }
 
-    systemd_suspend(mode, &local_err);
     if (!local_err) {
         return;
     }
 
     error_free(local_err);
 
-    pmutils_suspend(mode, &local_err);
+    if (pmutils_supports_mode(mode, &local_err)) {
+        mode_supported = true;
+        pmutils_suspend(mode, &local_err);
+    }
+
     if (!local_err) {
         return;
     }
 
     error_free(local_err);
 
-    linux_sys_state_suspend(mode, &local_err);
-    if (local_err) {
+    if (linux_sys_state_supports_mode(mode, &local_err)) {
+        mode_supported = true;
+        linux_sys_state_suspend(mode, &local_err);
+    }
+
+    if (!mode_supported) {
+        error_setg(errp,
+                   "the requested suspend mode is not supported by the guest");
+    } else if (local_err) {
         error_propagate(errp, local_err);
     }
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 0/6] QGA: systemd hibernate/suspend/hybrid-sleep

[Murilo Opsfelder Araujo]

On Thu, Jun 21, 2018 at 07:21:47AM -0300, Daniel Henrique Barboza wrote:
> changes in v2 from Marc-Andre Lureau review:
> - use error_free() accordingly
> - use g_spawn_sync() instead of fork() in run_process_child()
> - previous version link:
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05499.html
> 
> 
> This series adds systemd suspend support for QGA. Some newer
> guests don't have pmutils anymore, leaving us with just the
> Linux state file mechanism to suspend the guest OS, which does
> not support hybrid-sleep. With this implementation, QGA is
> now able to hybrid suspend newer guests again.
> 
> Most of the patches are cleanups in the existing suspend code,
> aiming at both simplifying it and making it easier to extend
> it with systemd.
> 
> 
> Daniel Henrique Barboza (6):
>   qga: refactoring qmp_guest_suspend_* functions
>   qga: bios_supports_mode: decoupling pm-utils and sys logic
>   qga: guest_suspend: decoupling pm-utils and sys logic
>   qga: removing switch statements, adding run_process_child
>   qga: systemd hibernate/suspend/hybrid-sleep support
>   qga: removing bios_supports_mode

Hi, Daniel.

Your patches look good, just some minor comments about how they're
organized and coding style, if you allow me.

I'd suggest to introduce the new API, functions, and typedef at first
(preferably one thing per patch to easier the review) without wiring
them up.

After the new stuff is ready to use, I'd wire them up (one per patch)
and update/remove old API they're replacing.

Introducing the new API and wiring it up later makes it easier to
review.  For example, bios_supports_mode() is changed by patch 1/6, then
it's moved around by patch 3/6, and finally removed by patch 6/6.

Do we need an empty line after opening a switch statement?  I'd drop it,
as seen in other parts of the code.

Does run_process_child() fit better under util/, or another place where
it can be shared throughout the code, if that's the case?

Cheers
Murilo

Re: [Qemu-devel] [PATCH v2 0/6] QGA: systemd hibernate/suspend/hybrid-sleep

[Daniel Henrique Barboza]

Hi,

On 06/21/2018 05:19 PM, Murilo Opsfelder Araujo wrote:
> On Thu, Jun 21, 2018 at 07:21:47AM -0300, Daniel Henrique Barboza wrote:
>> changes in v2 from Marc-Andre Lureau review:
>> - use error_free() accordingly
>> - use g_spawn_sync() instead of fork() in run_process_child()
>> - previous version link:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05499.html
>>
>>
>> This series adds systemd suspend support for QGA. Some newer
>> guests don't have pmutils anymore, leaving us with just the
>> Linux state file mechanism to suspend the guest OS, which does
>> not support hybrid-sleep. With this implementation, QGA is
>> now able to hybrid suspend newer guests again.
>>
>> Most of the patches are cleanups in the existing suspend code,
>> aiming at both simplifying it and making it easier to extend
>> it with systemd.
>>
>>
>> Daniel Henrique Barboza (6):
>>    qga: refactoring qmp_guest_suspend_* functions
>>    qga: bios_supports_mode: decoupling pm-utils and sys logic
>>    qga: guest_suspend: decoupling pm-utils and sys logic
>>    qga: removing switch statements, adding run_process_child
>>    qga: systemd hibernate/suspend/hybrid-sleep support
>>    qga: removing bios_supports_mode
> Hi, Daniel.
>
> Your patches look good, just some minor comments about how they're
> organized and coding style, if you allow me.
>
> I'd suggest to introduce the new API, functions, and typedef at first
> (preferably one thing per patch to easier the review) without wiring
> them up.
>
> After the new stuff is ready to use, I'd wire them up (one per patch)
> and update/remove old API they're replacing.
>
> Introducing the new API and wiring it up later makes it easier to
> review.  For example, bios_supports_mode() is changed by patch 1/6, then
> it's moved around by patch 3/6, and finally removed by patch 6/6.

The idea was to present the patches as incremental cleanups from
the existing code up to the point where the new API can be gracefully
added.

I think there's a point to be made about squashing all cleanups patches
in a single (or fewer) patches, but I believe it would be too annoying to
review that many changes at once - things started very coupled with
QMP functions hard-wiring pmutils binaries as parameters to the callers
(bios_support_mode and guest_suspend). If you compare that to the
result up to patch 4/6 you'll see that a lot was changed, and systemd
support was yet to be added.

Following the same incremental idea, patch 6/6 was created after I've
noticed that bios_support_mode was too sub-optimal after adding systemd
support. Yes, one can say that it was already sub-optimal before and it
should be removed right at the start, but I didn't bothered with this
change at that point due to other cleanups.

Anyway, if more people feels like the series structure should be changed
I can re-send the series again.

>
> Do we need an empty line after opening a switch statement?  I'd drop it,
> as seen in other parts of the code.

Didn't noticed it. If I end up respinning this series I'll remove it.

>
> Does run_process_child() fit better under util/, or another place where
> it can be shared throughout the code, if that's the case?

As I've mentioned in the commit message of patch 5/6, one step at a
time. If the function can be used in other places inside commands-posix.c
(IMO a fair assumption, but I didn't look it up carefully), then we could
first use this new function inside commands-posix.c and, if the function
turns out to be useful elsewhere, moved it to /util.

I didn't want to sound "pretentious" by adding a new function to /util
just because it is being used by the code I'm sending and because I
have a hunch that it will be useful to everyone else.



Thanks,


Daniel

>
> Cheers
> Murilo
>