qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: decoupling pm-utils


From: Daniel Henrique Barboza
Subject: Re: [Qemu-devel] [PATCH v1 3/6] qga: guest_suspend: decoupling pm-utils and sys logic
Date: Wed, 20 Jun 2018 17:05:41 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

Hi,

On 06/19/2018 08:23 PM, Marc-André Lureau wrote:
Hi

On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
<address@hidden> wrote:
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 <address@hidden>
---
  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..a2870f9ab9 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;
+    }
+
+    local_err = NULL;
You should error_free(). Same in later patches.

Thanks, I've fixed it for v2.


Daniel


+
+    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







reply via email to

[Prev in Thread] Current Thread [Next in Thread]