9:25 a.m.
While trying to make live detach of <console/> work, I've accumulated
couple of patches. The most reasonable ones made it into this patch set
as paches 1-4. And after nearly losing my sanity, I've decided it's not
worth supporting live detach of <console/> since <serial/> works just
fine. And that's what patch 5 does. If somebody thinks it's too harsh,
I'm more than happy to review their patches that fix the problem.
What problem? Well, for starters:
https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c4
https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c8
Michal Prívozník (5):
qemuDomainChrInsertPreAlloced: Fix adding implicit console
qemuDomainChrRemove: Don't leak vmdef->consoles[0]
qemuAssignDeviceChrAlias: Fix a crasher during <console/> hotplug
qemuDomainRemoveChrDevice: Deal with qemuDomainChrRemove() failure
qemu_hotplug: Deny live detach of <console/>
src/qemu/qemu_alias.c | 1 +
src/qemu/qemu_hotplug.c | 43 +++++++++++++++++++++++++++++------------
2 files changed, 32 insertions(+), 12 deletions(-)
--
2.39.2
9:25 a.m.
New subject: [PATCH 1/5] qemuDomainChrInsertPreAlloced: Fix adding implicit console
When hotpluging a <serial/> device, we might need to add a
<console/> device with it (because of some crazy backcompat).
Now, hotplugging is done in several phases. In one of them,
qemuDomainChrPreInsert() allocates space for both devices, and
then qemuDomainChrInsertPreAlloced() actually inserts the device
into domain definition and sets up the <console/> device with it.
Except, the condition that checks whether to create the aliased
<console/> is wrong as it compares nconsoles against 0.
Surprisingly, qemuDomainChrInsertPreAllocCleanup() doesn't suffer
from the same error.
Fixes: daf51be5f1b0f7b41c0813d43d6b66edfbe4f6d9
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3f45a48393..f517646c55 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1875,9 +1875,8 @@ qemuDomainChrInsertPreAlloced(virDomainDef *vmdef,
virDomainChrDef *chr)
{
virDomainChrInsertPreAlloced(vmdef, chr);
- if (vmdef->nserials == 1 && vmdef->nconsoles == 0 &&
+ if (vmdef->nserials == 1 && vmdef->nconsoles == 1 &&
chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) {
- vmdef->nconsoles = 1;
/* Create an console alias for the serial port */
vmdef->consoles[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
--
2.39.2
7:48 a.m.
New subject: [PATCH 1/5] qemuDomainChrInsertPreAlloced: Fix adding implicit console
On a Wednesday in 2023, Michal Privoznik wrote:
When hotpluging a <serial/> device, we might need to add a
<console/> device with it (because of some crazy backcompat).
Now, hotplugging is done in several phases. In one of them,
qemuDomainChrPreInsert() allocates space for both devices, and
then qemuDomainChrInsertPreAlloced() actually inserts the device
into domain definition and sets up the <console/> device with it.
Except, the condition that checks whether to create the aliased
<console/> is wrong as it compares nconsoles against 0.
Surprisingly, qemuDomainChrInsertPreAllocCleanup() doesn't suffer
from the same error.
Fixes: daf51be5f1b0f7b41c0813d43d6b66edfbe4f6d9
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:25 a.m.
New subject: [PATCH 2/5] qemuDomainChrRemove: Don't leak vmdef->consoles[0]
When removing the compat console from domain defintion, removing
it from the vmdef->consoles array is good, but not sufficient.
The console definition might have been fully allocated (after
daemon restarted and reloaded the status XML). Use
virDomainChrDefFree() to free also the definition.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f517646c55..a6407f074b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1937,8 +1937,10 @@ qemuDomainChrRemove(virDomainDef *vmdef,
return NULL;
}
- if (removeCompat)
+ if (removeCompat) {
+ virDomainChrDefFree(vmdef->consoles[0]);
VIR_DELETE_ELEMENT(vmdef->consoles, 0, vmdef->nconsoles);
+ }
return ret;
}
--
2.39.2
9:25 a.m.
New subject: [PATCH 3/5] qemuAssignDeviceChrAlias: Fix a crasher during <console/> hotplug
For a running guest, a <serial/> device can be hotunplugged. This
will then remove also aliased <console/>. Trying to hotplug a
<console/> device then, libvirtd crashed because it dereferences
def->consoles while there's none.
Fixes: 42d53ac799a1d7f1414737caa4deb73871876992
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_alias.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 85db7fbfe3..161d30cf72 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -95,6 +95,7 @@ qemuAssignDeviceChrAlias(virDomainDef *def,
if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL &&
def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
+ def->nconsoles &&
def->consoles[0] == chr &&
def->nserials &&
def->serials[0]->info.alias) {
--
2.39.2
9:25 a.m.
New subject: [PATCH 4/5] qemuDomainRemoveChrDevice: Deal with qemuDomainChrRemove() failure
When cleaning up after removed device, qemuDomainChrRemove() is
called. But this may fail, in which case we successfully ignore
the failure and virDomainChrDefFree() the device anyway. While it
decreases our memory consumption, it's a bit too far, especially
if the next step is 'virsh dumpxml'. Then our memory consumption
decreases all the way down to zero as we crash.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a6407f074b..d9a102191f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4695,6 +4695,7 @@ qemuDomainRemoveChrDevice(virQEMUDriver *driver,
virObjectEvent *event;
g_autofree char *charAlias = NULL;
qemuDomainObjPrivate *priv = vm->privateData;
+ virDomainChrDef *chrRemoved = NULL;
int rc = 0;
VIR_DEBUG("Removing character device %s from domain %p %s",
@@ -4728,17 +4729,24 @@ qemuDomainRemoveChrDevice(virQEMUDriver *driver,
if (qemuDomainNamespaceTeardownChardev(vm, chr) < 0)
VIR_WARN("Unable to remove chr device from /dev");
- qemuDomainReleaseDeviceAddress(vm, &chr->info);
- qemuDomainChrRemove(vm->def, chr);
+ if (!(chrRemoved = qemuDomainChrRemove(vm->def, chr))) {
+ /* At this point, we only have bad options. The device
+ * was successfully removed from QEMU, denied in CGropus,
+ * etc. and yet, we failed to remove it from domain
+ * definition. */
+ VIR_WARN("Unable to remove chr device from domain definition");
+ } else {
+ qemuDomainReleaseDeviceAddress(vm, &chrRemoved->info);
- /* The caller does not emit the event, so we must do it here. Note
- * that the event should be reported only after all backend
- * teardown is completed.
- */
- event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
+ /* The caller does not emit the event, so we must do it here. Note
+ * that the event should be reported only after all backend
+ * teardown is completed.
+ */
+ event = virDomainEventDeviceRemovedNewFromObj(vm, chrRemoved->info.alias);
+ virObjectEventStateQueue(driver->domainEventState, event);
- virDomainChrDefFree(chr);
+ virDomainChrDefFree(chrRemoved);
+ }
return 0;
}
--
2.39.2
9:25 a.m.
New subject: [PATCH 5/5] qemu_hotplug: Deny live detach of <console/>
I've tried, then I've tried even harder, but still wasn't able to
make sense of our console backcompat code in all its fine
details. Since I value my sanity, let's just forbid hotunplug of
<console/>, especially since detaching of corresponding <serial/>
works.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d9a102191f..5072798cb7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5693,6 +5693,16 @@ qemuDomainDetachDeviceChr(virQEMUDriver *driver,
goto cleanup;
}
+ 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 ||
+ tmpChr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)) {
+ /* Raise this limitation once device removal works. */
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("detaching of <console/> is unsupported. Try
corresponding <serial/> instead"));
+ goto cleanup;
+ }
+
/* guestfwd channels are not really -device rather than
* -netdev. We need to treat them slightly differently. */
guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
--
2.39.2
7:54 a.m.
On a Wednesday in 2023, Michal Privoznik wrote:
While trying to make live detach of <console/> work, I've
accumulated
couple of patches. The most reasonable ones made it into this patch set
as paches 1-4. And after nearly losing my sanity, I've decided it's not
worth supporting live detach of <console/> since <serial/> works just
fine. And that's what patch 5 does. If somebody thinks it's too harsh,
I'm more than happy to review their patches that fix the problem.
What problem? Well, for starters:
https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c4
https://bugzilla.redhat.com/show_bug.cgi?id=2156300#c8
Michal Prívozník (5):
qemuDomainChrInsertPreAlloced: Fix adding implicit console
qemuDomainChrRemove: Don't leak vmdef->consoles[0]
qemuAssignDeviceChrAlias: Fix a crasher during <console/> hotplug
qemuDomainRemoveChrDevice: Deal with qemuDomainChrRemove() failure
qemu_hotplug: Deny live detach of <console/>
src/qemu/qemu_alias.c | 1 +
src/qemu/qemu_hotplug.c | 43 +++++++++++++++++++++++++++++------------
2 files changed, 32 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
583
days inactive
589
days old
7 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik