[PATCH] qemuProcessSetupDisksTransientHotplug: Add checking set-action capability
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
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
On Thu, Aug 26, 2021 at 04:11:15PM +0200, Ján Tomko wrote:
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
I'm sorry I didn't make it clear enough. It's a similar issue, but it isn't the same issue. This patch tried to fix a new issue which was introduced the lifecycle event series.
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.
That seems a great idea, thanks! I'll post the v2 to add check in the validation. Like as follows: diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 8906aa52d9..47f12944aa 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3033,6 +3033,13 @@ qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, } if (disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SET_ACTION)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("transiend disk backing image sharing isn't supported by this QEMU binary (%s)"), + disk->dst); + return -1; + } + /* sharing the backing file requires hotplug of the disk in the qemu driver */ switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_USB: --- - Masa
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
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.
participants (3)
-
Ján Tomko -
Masayoshi Mizuma -
Peter Krempa