1. Patchew
  2. QEMU
  3. [PATCH v3 0/7] qga/commands-posix: replace code duplicating commands with a helper
  4. View patch

[PATCH v3 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs

Andrey Drobyshev posted 7 patches 1 year, 11 months ago
Maintainers: Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>
There is a newer version of this series
[PATCH v3 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs
Posted by Andrey Drobyshev 1 year, 11 months ago 
Since commit 246d76eba ("qga: guest_suspend: decoupling pm-utils and sys
logic") pm-utils logic is running in a separate child from the sysfs
logic.  Now when suspending via sysfs we don't really need to do that in
a separate process as we only need to perform one write to /sys/power/state.

Let's just use g_file_set_contents() to simplify things here.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 41 +++++------------------------------------
 1 file changed, 5 insertions(+), 36 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 610d225d30..e0ea377f65 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1928,52 +1928,21 @@ static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
 
 static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
 {
-    Error *local_err = NULL;
+    GError *local_gerr = NULL;
     const char *sysfile_strs[3] = {"disk", "mem", NULL};
     const char *sysfile_str = sysfile_strs[mode];
-    pid_t pid;
-    int status;
 
     if (!sysfile_str) {
         error_setg(errp, "unknown guest suspend mode");
         return;
     }
 
-    pid = fork();
-    if (!pid) {
-        /* child */
-        int fd;
-
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
-        if (fd < 0) {
-            _exit(EXIT_FAILURE);
-        }
-
-        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
-            _exit(EXIT_FAILURE);
-        }
-
-        _exit(EXIT_SUCCESS);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
-
-    ga_wait_child(pid, &status, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!g_file_set_contents(LINUX_SYS_STATE_FILE, sysfile_str,
+                             -1, &local_gerr)) {
+        error_setg(errp, "suspend: cannot write to '%s': %s",
+                   LINUX_SYS_STATE_FILE, local_gerr->message);
         return;
     }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to suspend");
-    }
-
 }
 
 static void guest_suspend(SuspendMode mode, Error **errp)
-- 
2.39.3
Re: [PATCH v3 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs
Posted by Daniel P. Berrangé 1 year, 10 months ago 
On Fri, Mar 15, 2024 at 02:29:45PM +0200, Andrey Drobyshev wrote:
> Since commit 246d76eba ("qga: guest_suspend: decoupling pm-utils and sys
> logic") pm-utils logic is running in a separate child from the sysfs
> logic.  Now when suspending via sysfs we don't really need to do that in
> a separate process as we only need to perform one write to /sys/power/state.
> 
> Let's just use g_file_set_contents() to simplify things here.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c | 41 +++++------------------------------------
>  1 file changed, 5 insertions(+), 36 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 610d225d30..e0ea377f65 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1928,52 +1928,21 @@ static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>  
>  static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>  {
> -    Error *local_err = NULL;
> +    GError *local_gerr = NULL;
>      const char *sysfile_strs[3] = {"disk", "mem", NULL};
>      const char *sysfile_str = sysfile_strs[mode];
> -    pid_t pid;
> -    int status;
>  
>      if (!sysfile_str) {
>          error_setg(errp, "unknown guest suspend mode");
>          return;
>      }
>  
> -    pid = fork();
> -    if (!pid) {
> -        /* child */
> -        int fd;
> -
> -        setsid();
> -        reopen_fd_to_null(0);
> -        reopen_fd_to_null(1);
> -        reopen_fd_to_null(2);
> -
> -        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> -        if (fd < 0) {
> -            _exit(EXIT_FAILURE);
> -        }
> -
> -        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
> -            _exit(EXIT_FAILURE);
> -        }
> -
> -        _exit(EXIT_SUCCESS);
> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        return;
> -    }
> -
> -    ga_wait_child(pid, &status, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!g_file_set_contents(LINUX_SYS_STATE_FILE, sysfile_str,
> +                             -1, &local_gerr)) {
> +        error_setg(errp, "suspend: cannot write to '%s': %s",
> +                   LINUX_SYS_STATE_FILE, local_gerr->message);

You need to declare with "g_autoptr(GError) local_gerr = NULL" to
avoid a leak here.

>          return;
>      }
> -
> -    if (WEXITSTATUS(status)) {
> -        error_setg(errp, "child process has failed to suspend");
> -    }
> -
>  }
>  
>  static void guest_suspend(SuspendMode mode, Error **errp)
> -- 
> 2.39.3
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v3 6/7] qga/commands-posix: don't do fork()/exec() when suspending via sysfs
Posted by Andrey Drobyshev 1 year, 10 months ago 
On 3/19/24 20:02, Daniel P. Berrangé wrote:
> On Fri, Mar 15, 2024 at 02:29:45PM +0200, Andrey Drobyshev wrote:
>> Since commit 246d76eba ("qga: guest_suspend: decoupling pm-utils and sys
>> logic") pm-utils logic is running in a separate child from the sysfs
>> logic.  Now when suspending via sysfs we don't really need to do that in
>> a separate process as we only need to perform one write to /sys/power/state.
>>
>> Let's just use g_file_set_contents() to simplify things here.
>>
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 41 +++++------------------------------------
>>  1 file changed, 5 insertions(+), 36 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 610d225d30..e0ea377f65 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1928,52 +1928,21 @@ static bool linux_sys_state_supports_mode(SuspendMode mode, Error **errp)
>>  
>>  static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>>  {
>> -    Error *local_err = NULL;
>> +    GError *local_gerr = NULL;
>>      const char *sysfile_strs[3] = {"disk", "mem", NULL};
>>      const char *sysfile_str = sysfile_strs[mode];
>> -    pid_t pid;
>> -    int status;
>>  
>>      if (!sysfile_str) {
>>          error_setg(errp, "unknown guest suspend mode");
>>          return;
>>      }
>>  
>> -    pid = fork();
>> -    if (!pid) {
>> -        /* child */
>> -        int fd;
>> -
>> -        setsid();
>> -        reopen_fd_to_null(0);
>> -        reopen_fd_to_null(1);
>> -        reopen_fd_to_null(2);
>> -
>> -        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>> -        if (fd < 0) {
>> -            _exit(EXIT_FAILURE);
>> -        }
>> -
>> -        if (write(fd, sysfile_str, strlen(sysfile_str)) < 0) {
>> -            _exit(EXIT_FAILURE);
>> -        }
>> -
>> -        _exit(EXIT_SUCCESS);
>> -    } else if (pid < 0) {
>> -        error_setg_errno(errp, errno, "failed to create child process");
>> -        return;
>> -    }
>> -
>> -    ga_wait_child(pid, &status, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> +    if (!g_file_set_contents(LINUX_SYS_STATE_FILE, sysfile_str,
>> +                             -1, &local_gerr)) {
>> +        error_setg(errp, "suspend: cannot write to '%s': %s",
>> +                   LINUX_SYS_STATE_FILE, local_gerr->message);
> 
> You need to declare with "g_autoptr(GError) local_gerr = NULL" to
> avoid a leak here.
> 

Of course, thanks for noticing.

>>          return;
>>      }
>> -
>> -    if (WEXITSTATUS(status)) {
>> -        error_setg(errp, "child process has failed to suspend");
>> -    }
>> -
>>  }
>>  
>>  static void guest_suspend(SuspendMode mode, Error **errp)
>> -- 
>> 2.39.3
>>
> 
> With regards,
> Daniel

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.