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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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