[PATCH] qemuProcessSetupDisksTransientHotplug: Add checking set-action capability

Masayoshi Mizuma posted 1 patch 2 years, 7 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210825230846.9949-1-msys.mizuma@gmail.com
src/qemu/qemu_process.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] qemuProcessSetupDisksTransientHotplug: Add checking set-action capability
Posted by Masayoshi Mizuma 2 years, 7 months ago
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

The VM is terminated abnormally when <transient shareBacking='yes'/>
is set to the disk option and the qemu doesn't have set-action capability.

  # virsh start guest01
  error: Failed to start domain 'guest01'
  error: internal error: qemu unexpectedly closed the monitor

  #

Add checking the capability before system_reset QMP command is sent
so that the VM can stop correctly when the qemu doesn't have the cap.

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 src/qemu/qemu_process.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3b4af61bf8..4bedd04679 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7010,6 +7010,9 @@ qemuProcessSetupDisksTransientHotplug(virDomainObj *vm,
     if (hasHotpluggedDisk) {
         int rc;
 
+        if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)))
+            return -1;
+
         if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0)
             return -1;
 
-- 
2.27.0

Re: [PATCH] qemuProcessSetupDisksTransientHotplug: Add checking set-action capability
Posted by Peter Krempa 2 years, 7 months ago
On Wed, Aug 25, 2021 at 19:08:46 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> The VM is terminated abnormally when <transient shareBacking='yes'/>
> is set to the disk option and the qemu doesn't have set-action capability.
> 
>   # virsh start guest01
>   error: Failed to start domain 'guest01'
>   error: internal error: qemu unexpectedly closed the monitor
> 
>   #
> 
> Add checking the capability before system_reset QMP command is sent
> so that the VM can stop correctly when the qemu doesn't have the cap.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> ---
>  src/qemu/qemu_process.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3b4af61bf8..4bedd04679 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7010,6 +7010,9 @@ qemuProcessSetupDisksTransientHotplug(virDomainObj *vm,
>      if (hasHotpluggedDisk) {
>          int rc;
>  
> +        if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)))
> +            return -1;

This should be in one of the validation functions checking support for
transient disks and with an appropriate error message.

Re: [PATCH] qemuProcessSetupDisksTransientHotplug: Add checking set-action capability
Posted by Ján Tomko 2 years, 7 months ago
On a Wednesday in 2021, Masayoshi Mizuma wrote:
>From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>
>The VM is terminated abnormally when <transient shareBacking='yes'/>
>is set to the disk option and the qemu doesn't have set-action capability.
>
>  # virsh start guest01
>  error: Failed to start domain 'guest01'
>  error: internal error: qemu unexpectedly closed the monitor
>
>  #
>
>Add checking the capability before system_reset QMP command is sent
>so that the VM can stop correctly when the qemu doesn't have the cap.
>

 From the commit message it's hard to see the connection between missing
QEMU_CAPS_SET_ACTION and not calling the system_reset command, since
the command was present long before set-action.

Is this the same issue?
https://listman.redhat.com/archives/libvir-list/2021-July/msg00119.html

If I understand correctly, even after Peter's lifecycle event series
we cannot support the combination of:
* -no-reboot on the command line (which is still done for QEMUs without
    SET_ACTION, i.e. older than 6.0)
* transient shareBacking disks

In that case, this can be rejected much sooner in qemuValidate, before
even trying to start the QEMU process.

>Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>---
> src/qemu/qemu_process.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index 3b4af61bf8..4bedd04679 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -7010,6 +7010,9 @@ qemuProcessSetupDisksTransientHotplug(virDomainObj *vm,
>     if (hasHotpluggedDisk) {
>         int rc;
>
>+        if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)))

The error message is missing.

Jano

>+            return -1;
>+
>         if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0)
>             return -1;
>
>-- 
>2.27.0
>