[Qemu-devel] [PATCH v1 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 creates a child process and executes a (shell)
command, returning the command return 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 <address@hidden>
---
 qga/commands-posix.c | 190 +++++++++++++++++--------------------------
 1 file changed, 76 insertions(+), 114 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a2870f9ab9..d5e3805ce9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1438,152 +1438,122 @@ 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;
+
+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;
+    char *cmd_path = g_find_program_in_path(command[0]);
     pid_t pid;
-    int status;
-    bool ret = false;
-
-    switch (suspend_mode) {
-
-    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;
-    }
+    int status, ret = -1;
 
-    pmutils_path = g_find_program_in_path(pmutils_bin);
-    if (!pmutils_path) {
+    if (!cmd_path) {
         return ret;
     }
 
     pid = fork();
     if (!pid) {
         setsid();
-        execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
         /*
-         * If we get here execle() has failed.
+         * execve receives a char* const argv[] as second arg but we're
+         * receiving a const char*[]. Since execve does not change the
+         * array contents it's tolerable to cast here.
          */
-        _exit(SUSPEND_NOT_SUPPORTED);
+        execve(cmd_path, (char* const*)command, environ);
+        _exit(errno);
     } else if (pid < 0) {
         error_setg_errno(errp, errno, "failed to create child process");
+        ret = EXIT_FAILURE;
         goto out;
     }
 
     ga_wait_child(pid, &status, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
+        ret = EXIT_FAILURE;
         goto out;
     }
 
-    switch (WEXITSTATUS(status)) {
-    case SUSPEND_SUPPORTED:
-        ret = true;
-        goto out;
-    case SUSPEND_NOT_SUPPORTED:
-        goto out;
-    default:
-        error_setg(errp,
-                   "the helper program '%s' returned an unexpected exit status"
-                   " code (%d)", pmutils_path, WEXITSTATUS(status));
-        goto out;
-    }
+    ret = WEXITSTATUS(status);
 
 out:
-    g_free(pmutils_path);
+    g_free(cmd_path);
     return ret;
 }
 
-static void pmutils_suspend(int suspend_mode, Error **errp)
+static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
 {
     Error *local_err = NULL;
-    const char *pmutils_bin;
-    char *pmutils_path;
-    pid_t pid;
+    const char *pmutils_args[3] = {"--hibernate", "--suspend",
+                                   "--suspend-hybrid"};
+    const char *cmd[3] = {"pm-is-supported", pmutils_args[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);
-        return;
+    if (status == SUSPEND_SUPPORTED) {
+        return true;
     }
 
-    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) {
+        return false;
     }
 
-    ga_wait_child(pid, &status, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
+    } else {
+        error_setg(errp,
+                   "the helper program '%s' returned an unexpected exit"
+                   " status code (%d)", "pm-is-supported", status);
     }
 
-    if (WEXITSTATUS(status)) {
-        error_setg(errp,
-                   "the helper program '%s' returned an unexpected exit status"
-                   " code (%d)", pmutils_path, WEXITSTATUS(status));
+    return false;
+}
+
+static void pmutils_suspend(SuspendMode mode, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
+                                       "pm-suspend-hybrid"};
+    const char *cmd[2] = {pmutils_binaries[mode], NULL};
+    int status;
+
+    status = run_process_child(cmd, &local_err);
+
+    if (status == 0) {
+        return;
     }
 
-out:
-    g_free(pmutils_path);
+    if (status == -1) {
+        error_setg(errp, "the helper program '%s' was not found",
+                    pmutils_binaries[mode]);
+        return;
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+    } else {
+        error_setg(errp,
+                   "the helper program '%s' returned an unexpected exit"
+                   " status code (%d)", pmutils_binaries[mode], status);
+    }
 }
 
-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 +1574,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 +1624,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 +1637,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;
     }
 
     local_err = NULL;
 
-    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