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

[PATCH v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs

Andrey Drobyshev posted 7 patches 8 months, 4 weeks ago
Maintainers: Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>
There is a newer version of this series
[PATCH v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs
Posted by Andrey Drobyshev 8 months, 4 weeks ago 
We replace the direct call to open() with a "sh -c 'echo ...'" call, so
that it becomes an executable command.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qga/commands-posix.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index dd2a7ad2e6..f3f4a05e2d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1921,49 +1921,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
     Error *local_err = 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;
-    }
+    g_autofree char *echo_cmd = g_strdup_printf(
+        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
+    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
 
-    ga_wait_child(pid, &status, &local_err);
+    ga_run_command(argv, NULL, "suspend", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         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 v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs
Posted by Daniel P. Berrangé 8 months, 3 weeks ago 
On Fri, Mar 01, 2024 at 07:28:57PM +0200, Andrey Drobyshev wrote:
> We replace the direct call to open() with a "sh -c 'echo ...'" call, so
> that it becomes an executable command.

Introduced an indirection via the shell is a significant step
backwards IMHO.

> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  qga/commands-posix.c | 36 ++++--------------------------------
>  1 file changed, 4 insertions(+), 32 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index dd2a7ad2e6..f3f4a05e2d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1921,49 +1921,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>      Error *local_err = 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);


This pre-existing code is strange to me.  Why do we need to fork a
new process in order to write to /sys/power/state ?

Looking at the original commit

  commit 11d0f1255bd5651f628280dc96c4ce9d63ae9236
  Author: Luiz Capitulino <lcapitulino@redhat.com>
  Date:   Tue Feb 28 11:03:03 2012 -0300

    qemu-ga: add guest-suspend-disk
    

The code made a little more sense, as after fork() it first
tried to execve  'pm-utils', and then had the sysfs codepath
as a fallback. IOW having the sysfs code after fork() was a
much easier code structure.

This was all changed in

  commit 246d76eba1944d7e59affb288ec27d7fcfb5d256
  Author: Daniel Henrique Barboza <danielhb413@gmail.com>
  Date:   Thu Jun 21 07:21:50 2018 -0300

    qga: guest_suspend: decoupling pm-utils and sys logic


so the pm-utils logic runs in a separate forked child
from the sysfs logic.

AFAICT, that should have made it completely redundant to
fork a process to access /sys/power/state.

Does anyone know of a reason to keep the fork() here ? Of
not we should just be calling 'g_file_set_contents' without
fork

> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        return;
> -    }
> +    g_autofree char *echo_cmd = g_strdup_printf(
> +        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
> +    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
>  
> -    ga_wait_child(pid, &status, &local_err);
> +    ga_run_command(argv, NULL, "suspend", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          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 v2 6/7] qga/commands-posix: use ga_run_command helper when suspending via sysfs
Posted by Andrey Drobyshev 8 months, 2 weeks ago 
On 3/5/24 20:34, Daniel P. Berrangé wrote:
> [Вы нечасто получаете письма от berrange@redhat.com. Узнайте, почему это важно, по адресу https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Fri, Mar 01, 2024 at 07:28:57PM +0200, Andrey Drobyshev wrote:
>> We replace the direct call to open() with a "sh -c 'echo ...'" call, so
>> that it becomes an executable command.
> 
> Introduced an indirection via the shell is a significant step
> backwards IMHO.
> 
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  qga/commands-posix.c | 36 ++++--------------------------------
>>  1 file changed, 4 insertions(+), 32 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index dd2a7ad2e6..f3f4a05e2d 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1921,49 +1921,21 @@ static void linux_sys_state_suspend(SuspendMode mode, Error **errp)
>>      Error *local_err = 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);
> 
> 
> This pre-existing code is strange to me.  Why do we need to fork a
> new process in order to write to /sys/power/state ?
> 
> Looking at the original commit
> 
>   commit 11d0f1255bd5651f628280dc96c4ce9d63ae9236
>   Author: Luiz Capitulino <lcapitulino@redhat.com>
>   Date:   Tue Feb 28 11:03:03 2012 -0300
> 
>     qemu-ga: add guest-suspend-disk
> 
> 
> The code made a little more sense, as after fork() it first
> tried to execve  'pm-utils', and then had the sysfs codepath
> as a fallback. IOW having the sysfs code after fork() was a
> much easier code structure.
> 
> This was all changed in
> 
>   commit 246d76eba1944d7e59affb288ec27d7fcfb5d256
>   Author: Daniel Henrique Barboza <danielhb413@gmail.com>
>   Date:   Thu Jun 21 07:21:50 2018 -0300
> 
>     qga: guest_suspend: decoupling pm-utils and sys logic
> 
> 
> so the pm-utils logic runs in a separate forked child
> from the sysfs logic.
> 
> AFAICT, that should have made it completely redundant to
> fork a process to access /sys/power/state.
> 
> Does anyone know of a reason to keep the fork() here ? Of
> not we should just be calling 'g_file_set_contents' without
> fork
> 

In the original commit message Luiz Capitulino explained that multiple
forks are needed to properly reap child processes needed for execl().
AFAIU since writing to /sys/power/state doesn't require execl(), fork()
isn't needed either.  I think the 2nd commit you mention simply kept
things as they were since it wasn't the original goal of the patch.  So
we're just looking at legacy code.

I agree that in this case my original patch is redundant and we may
replace it with smth like g_file_set_contents().  I'll add it in v3.

>> -    } else if (pid < 0) {
>> -        error_setg_errno(errp, errno, "failed to create child process");
>> -        return;
>> -    }
>> +    g_autofree char *echo_cmd = g_strdup_printf(
>> +        "echo %s > " LINUX_SYS_STATE_FILE, sysfile_str);
>> +    const char *argv[] = {"sh", "-c", echo_cmd, NULL};
>>
>> -    ga_wait_child(pid, &status, &local_err);
>> +    ga_run_command(argv, NULL, "suspend", &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          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 :|
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.