On 3/5/24 20:34, Daniel P. Berrangé wrote:
> [Вы нечасто получаете письма от berra...@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.drobys...@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 <lcapitul...@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 <danielhb...@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 :|
>
