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é <berra...@redhat.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com>
> ---
> qga/commands-posix.c | 41 +++++------------------------------------
> 1 file changed, 5 insertions(+), 36 deletions(-)
Reviewed-by: Daniel P. Berrangé <berra...@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 :|
