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

Hi

On Tue, Jun 19, 2018 at 9:38 PM, Daniel Henrique Barboza
<danielhb413@gmail.com> 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 <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..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.

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



-- 
Marc-André Lureau

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
> <danielhb413@gmail.com> 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 <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..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
>>
>>
>
>