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é <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.
>
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
