* Re: [PATCH] qga: Fix error handling in system suspend mode causing libvirt to hold lock indefinitely
2025-08-04 8:18 ` Kostiantyn Kostiuk
@ 2025-08-04 9:54 ` nici zheng
0 siblings, 0 replies; 3+ messages in thread
From: nici zheng @ 2025-08-04 9:54 UTC (permalink / raw)
To: Kostiantyn Kostiuk
Cc: qemu-devel, michael.roth, panzerzheng, Daniel Berrangé
[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]
Hi Kostiantyn,
Thank you again for your valuable feedback. I've conducted further
tests to address your questions, and here are the details:
1. Regarding exit code 1:
I performed an experiment with the kernel configuration:
When CONFIG_HIBERNATION is disabled in the kernel:
[root@VM-23-13-opencloudos OpenCloudOS-Kernel]# grep -i HIBERNATION
/boot/config-6.6.47-12.oc9.x86_64
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
# CONFIG_HIBERNATION is not set
The /sys/power/state shows no "disk" support:
[root@VM-0-94-opencloudos patch]# cat /sys/power/state
freeze mem
While "systemctl status systemd-hibernate" returns code 3:
[root@VM-0-94-opencloudos patch]# systemctl status systemd-hibernate
○ systemd-hibernate.service - Hibernate
Loaded: loaded (/usr/lib/systemd/system/systemd-hibernate.service; static)
Active: inactive (dead)
Docs: man:systemd-hibernate.service(8)
[root@VM-0-94-opencloudos patch]# echo $?
3
The actual hibernation command returns code 1 due to kernel incompatibility:
[root@VM-0-94-opencloudos patch]# systemctl hibernate
Call to Hibernate failed: Sleep verb 'hibernate' is not
configured or configuration is not supported by kernel
[root@VM-0-94-opencloudos patch]# echo $?
1
This confirms exit code 1 indicates actual hibernation support
failure in this scenario.
2. Regarding mode_supported logic:
I understand your confusion about the check in
systemd_supports_mode(mode, &local_err). Another experiment
illustrates the issue:
On a system with hibernation enabled:
[opencloudos@VM-23-13-opencloudos ~]$ grep -i HIBERNATION
/boot/config-6.6.92+
CONFIG_ARCH_HIBERNATION_POSSIBLE=y
CONFIG_ARCH_HIBERNATION_HEADER=y
CONFIG_HIBERNATION=y
CONFIG_HIBERNATION_SNAPSHOT_DEV=y
/sys/power/state includes "disk":
[opencloudos@VM-23-13-opencloudos ~]$ cat /sys/power/state
freeze mem disk
However, "systemctl status systemd-hibernate" still returns code 3
regardless of actual support:
[opencloudos@VM-23-13-opencloudos ~]$ sudo systemctl status systemd-hibernate
○ systemd-hibernate.service - Hibernate
Loaded: loaded
(/usr/lib/systemd/system/systemd-hibernate.service; static)
Active: inactive (dead)
Docs: man:systemd-hibernate.service(8)
[output truncated for brevity]
[opencloudos@VM-23-13-opencloudos ~]$ echo $?
3
This shows "systemctl status" isn't reliable for determining
support. The actual execution result of "systemctl hibernate" is
necessary to accurately verify suspend-to-disk capability.
Thanks again for your patience and guidance. I'll revise the patch
accordingly and resubmit as v2.
Best regards,
panzerzheng
Kostiantyn Kostiuk <kkostiuk@redhat.com> 于2025年8月4日周一 16:18写道：
>
>
> On Mon, Aug 4, 2025 at 10:47 AM <zhengnici@gmail.com> wrote:
>
>> From: panzerzheng <panzerzheng@tencent.com>
>>
>> When "/sys/power/state" does not contain "disk", the operating system
>> does not support hibernation. Executing "sudo systemctl hibernate" will
>> return an exit code of 1 with the stderr output: "Call to Hibernate
>> failed:
>> Sleep verb 'hibernate' is not configured or configuration is not
>> supported by kernel".
>>
>
>> This patch adds handling for exit code 1 in the systemd_suspend function,
>> setting appropriate error messages when the kernel does not support
>> standby.
>> It also adds local_err handling in the guest_suspend function to set
>> mode_supported to false.
>>
>> Without these fixes, libvirt would hold the lock indefinitely.
>>
>> Signed-off-by: panzerzheng <panzerzheng@tencent.com>
>> ---
>> qga/commands-linux.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
>> index 9e8a934b9a..26229396c3 100644
>> --- a/qga/commands-linux.c
>> +++ b/qga/commands-linux.c
>> @@ -1300,6 +1300,12 @@ static void systemd_suspend(SuspendMode mode,
>> Error **errp)
>> return;
>> }
>>
>> + if ((status == 1) && !local_err) {
>>
>
> Is there any documentation that explains that exit code = 1 is an error?
> If you look into systemd_supports_mode, you can see that exit code = 1 is
> detected
> as a supported mode.
>
>
>> + error_setg(errp, "'systemctl %s' not suspend",
>> + systemctl_args[mode]);
>> + return;
>> + }
>> +
>> if (local_err) {
>> error_propagate(errp, local_err);
>> } else {
>> @@ -1428,6 +1434,8 @@ static void guest_suspend(SuspendMode mode, Error
>> **errp)
>>
>> if (!local_err) {
>> return;
>> + } else {
>> + mode_supported = false;
>> }
>>
>
> This is very confusing. We check that the mode is supported, but after
> running suspend,
> we find that it is unsupported. I think we should fix
> systemd_supports_mode instead.
>
> Also, setting mode_supported = false does not change anything.
> In the next step, we will check pmutils_supports_mode and
> linux_sys_state_supports_mode,
> and I think this check will pass, so we set mode_supported = true again.
>
> Best Regards,
> Kostiantyn Kostiuk.
>
>
>> }
>>
>> --
>> 2.43.7
>>
>>
[-- Attachment #2: Type: text/html, Size: 7594 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread