[PATCH 0/3] Do not try hot(un)plugging platform char devices
Just don't. Martin Kletzander (3): qemu: Expose qemuChrIsPlatformDevice outside from qemu_command qemu_hotplug: Report better error message for platform serial devices qemu_hotplug: Do not report error for hot-unplugging non-existing device src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 5 +++++ src/qemu/qemu_hotplug.c | 13 +++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) -- 2.47.0
Then it can be used from qemu_hotplug. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a5ff7695c375..0dbaa155fc28 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9041,7 +9041,7 @@ qemuBuildChrDeviceCommandLine(virCommand *cmd, } -static bool +bool qemuChrIsPlatformDevice(const virDomainDef *def, virDomainChrDef *chr) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 309e566f6cbe..71ce031f1ac0 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -217,6 +217,11 @@ virJSONValue *qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) virJSONValue *qemuBuildShmemBackendMemProps(virDomainShmemDef *shmem) ATTRIBUTE_NONNULL(1); +bool +qemuChrIsPlatformDevice(const virDomainDef *def, + virDomainChrDef *chr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + virJSONValue * qemuBuildShmemDevProps(virDomainDef *def, virDomainShmemDef *shmem); -- 2.47.0
This should be better than the current for both hotplug: error: internal error: Invalid target model for serial device and hot-unplug: error: An error occurred, but the cause is unknown which should not be reached at all. Resolves: https://issues.redhat.com/browse/RHEL-66222 Resolves: https://issues.redhat.com/browse/RHEL-66223 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1a7b69e5bb44..f856e26c1877 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2063,6 +2063,12 @@ qemuDomainAttachChrDevice(virQEMUDriver *driver, bool need_release = false; bool guestfwd = false; + if (qemuChrIsPlatformDevice(vmdef, chr)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Cannot hotplug platform device")); + return -1; + } + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { guestfwd = chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; @@ -6049,6 +6055,12 @@ qemuDomainDetachDeviceChr(virQEMUDriver *driver, goto cleanup; } + if (qemuChrIsPlatformDevice(vmdef, tmpChr)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Cannot detach platform device")); + return -1; + } + if (vmdef->os.type == VIR_DOMAIN_OSTYPE_HVM && tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && (tmpChr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || -- 2.47.0
On Mon, Nov 11, 2024 at 09:53:08 +0100, Martin Kletzander wrote:
This should be better than the current for both hotplug:
error: internal error: Invalid target model for serial device
and hot-unplug:
error: An error occurred, but the cause is unknown
which should not be reached at all.
Resolves: https://issues.redhat.com/browse/RHEL-66222 Resolves: https://issues.redhat.com/browse/RHEL-66223 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1a7b69e5bb44..f856e26c1877 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2063,6 +2063,12 @@ qemuDomainAttachChrDevice(virQEMUDriver *driver, bool need_release = false; bool guestfwd = false;
+ if (qemuChrIsPlatformDevice(vmdef, chr)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
I'd suggest VIR_ERR_OPERATION_UNSUPPORTED instead.
+ _("Cannot hotplug platform device")); + return -1; + } + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { guestfwd = chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD;
@@ -6049,6 +6055,12 @@ qemuDomainDetachDeviceChr(virQEMUDriver *driver, goto cleanup; }
+ if (qemuChrIsPlatformDevice(vmdef, tmpChr)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Cannot detach platform device")); + return -1; + } + if (vmdef->os.type == VIR_DOMAIN_OSTYPE_HVM && tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && (tmpChr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || -- 2.47.0
The code just does not match the comment above which says we should claim success. And it makes sense since a removal from qemu was requested and qemu could not find the device. Without this patch such codepath would lead to libvirt not removing the device from the XML and no error being set, but the API would still return an error value. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f856e26c1877..d23da0a553c0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -136,6 +136,7 @@ qemuDomainDeleteDevice(virDomainObj *vm, * domain XML is queried right after detach API the * device would still be there. */ VIR_DEBUG("Detaching of device %s failed and no event arrived", alias); + rc = 0; } } -- 2.47.0
On Mon, Nov 11, 2024 at 09:53:09 +0100, Martin Kletzander wrote:
The code just does not match the comment above which says we should claim success. And it makes sense since a removal from qemu was requested and qemu could not find the device. Without this patch such codepath would lead to libvirt not removing the device from the XML and no error being set, but the API would still return an error value.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f856e26c1877..d23da0a553c0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -136,6 +136,7 @@ qemuDomainDeleteDevice(virDomainObj *vm, * domain XML is queried right after detach API the * device would still be there. */ VIR_DEBUG("Detaching of device %s failed and no event arrived", alias); + rc = 0;
After doing this the function can't return -2, but it will still be documented as such. There is also a caller which explicitly checks for -2. I guess a better course of actions would be to add the '-2' check to qemuDomainDetachDeviceChr instead. Possibly also to 'qemuDomainHotplugDelVcpu' but there that shouldn't be possible to happen.
On Mon, Nov 11, 2024 at 09:53:06 +0100, Martin Kletzander wrote:
Just don't.
Martin Kletzander (3): qemu: Expose qemuChrIsPlatformDevice outside from qemu_command qemu_hotplug: Report better error message for platform serial devices
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
qemu_hotplug: Do not report error for hot-unplugging non-existing device
And if you agree with checking -2 in the callers instead also here: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Mon, Nov 11, 2024 at 10:11:36AM +0100, Peter Krempa wrote:
On Mon, Nov 11, 2024 at 09:53:06 +0100, Martin Kletzander wrote:
Just don't.
Martin Kletzander (3): qemu: Expose qemuChrIsPlatformDevice outside from qemu_command qemu_hotplug: Report better error message for platform serial devices
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
I pushed these two, thank you.
qemu_hotplug: Do not report error for hot-unplugging non-existing device
And if you agree with checking -2 in the callers instead also here:
To make sure I do that properly I'll send another version of this patch.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Martin Kletzander -
Peter Krempa