2:53 a.m.
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
2:53 a.m.
New subject: [PATCH 1/3] qemu: Expose qemuChrIsPlatformDevice outside from qemu_command
Then it can be used from qemu_hotplug.
Signed-off-by: Martin Kletzander <mkletzan(a)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
2:53 a.m.
New subject: [PATCH 2/3] qemu_hotplug: Report better error message for platform serial devices
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(a)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
3:01 a.m.
New subject: [PATCH 2/3] qemu_hotplug: Report better error message for
platform serial devices
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(a)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
2:53 a.m.
New subject: [PATCH 3/3] qemu_hotplug: Do not report error for hot-unplugging non-existing device
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(a)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
3:10 a.m.
New subject: [PATCH 3/3] qemu_hotplug: Do not report error for hot-unplugging
non-existing device
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(a)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.
3:11 a.m.
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(a)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(a)redhat.com>
5:50 a.m.
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(a)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(a)redhat.com>
12
days inactive
12
days old
7 comments
2 participants
participants (2)
-
Martin Kletzander -
Peter Krempa