| Message ID | 20240301172858.665135-7-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qga/commands-posix: replace code duplicating commands with a helper | expand |
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
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 :| >
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)
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(-)