[PATCH] qga: Fix error handling in system suspend mode causing libvirt to hold lock indefinitely

[PATCH] qga: Fix error handling in system suspend mode causing libvirt to hold lock indefinitely

[zhengnici]

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) {
+        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;
         }
     }
 
-- 
2.43.7

Re: [PATCH] qga: Fix error handling in system suspend mode causing libvirt to hold lock indefinitely

[Kostiantyn Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

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: 3631 bytes --]

Re: [PATCH] qga: Fix error handling in system suspend mode causing libvirt to hold lock indefinitely

[nici zheng]

[-- 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 --]