[libvirt] [PATCH v3 0/7] hotplug: Fix libvirtd crash on qemu-attached guest
v2 is here: http://www.redhat.com/archives/libvir-list/2014-September/msg01575.html As Michal pointed out in his review - other devices could have the same issue - so take care of each of them separately (I already knew the answer to my last question...) Totally different approach this time - rather than error out, follow the example I from the Controller Detach code which will generate an alias for the device (although it did miss one minor check to see if it was already set leading to a potential memory leak since the alias code would overwrite whatever was there). Note: In a way 1/7 was already ACK'd - I just hadn't separated it yet for a push and wanted to keep these closer together when/if they were pushed. Whether 7/7 is now necessary is debateable - I keep it only for completeness and environment setup in much the same way the start code handles aliases. John Ferlan (7): virsh: Adjust the text in man page regarding qemu-attach hotplug: Check for alias in controller detach hotplug: Check for alias in disk detach hotplug: Check for alias in hostdev detach hotplug: Check for alias in chrdev detach hotplug: Check for alias in net detach qemu-attach: Assign device aliases src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_hotplug.c | 28 +++++++++++++++++++++++++++- tools/virsh.pod | 5 +++-- 3 files changed, 33 insertions(+), 3 deletions(-) -- 1.9.3
Slight adjustment to the qemu-attach man page to note device hotplug and hot unplug may not work and that the environment should be considered read-only Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.pod | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index eae9195..bd17f68 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3698,8 +3698,9 @@ using the UNIX driver. Ideally the process will also have had the Not all functions of libvirt are expected to work reliably after attaching to an externally launched QEMU process. There may be -issues with the guest ABI changing upon migration, and hotunplug -may not work. +issues with the guest ABI changing upon migration and device hotplug +or hotunplug may not work. The attached environment should be considered +primarily read-only. =item B<qemu-monitor-command> I<domain> { [I<--hmp>] | [I<--pretty>] } I<command>... -- 1.9.3
In qemuDomainDetachControllerDevice if the info.alias already exists a call to qemuAssignDeviceControllerAlias would overwrite the existing so avoid this possibility. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1e504ec..9c0f6c9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3226,7 +3226,8 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + !detach->info.alias) { if (qemuAssignDeviceControllerAlias(detach) < 0) goto cleanup; } -- 1.9.3
If the QEMU_CAPS_DEVICE is set, then ensure the disk device alias has been properly set in prior to making the calls to detach the device. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9c0f6c9..3e8cdbf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2998,6 +2998,12 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, } } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + !detach->info.alias) { + if (qemuAssignDeviceDiskAlias(vm->def, detach, priv->qemuCaps) < 0) + goto cleanup; + } + qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); -- 1.9.3
If the QEMU_CAPS_DEVICE is set, then ensure the host device alias has been properly set before making the calls to detach the device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3e8cdbf..db39948 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3371,8 +3371,15 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr detach) { + qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + !detach->info->alias) { + if (qemuAssignDeviceHostdevAlias(vm->def, detach, -1) < 0) + return -1; + } + switch (detach->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: ret = qemuDomainDetachHostPCIDevice(driver, vm, detach); -- 1.9.3
If the QEMU_CAPS_DEVICE is set, then ensure the chr device alias has been properly set before making the calls to detach the device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index db39948..f79a37a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3696,6 +3696,12 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, return ret; } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + !tmpChr->info.alias) { + if (qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) + return ret; + } + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) return ret; -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1141621 If the QEMU_CAPS_DEVICE is set, then ensure the host device alias has been properly set before making the calls to detach the device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f79a37a..33241fb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3530,6 +3530,12 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, } } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && + !detach->info.alias) { + if (qemuAssignDeviceNetAlias(vm->def, detach, -1) < 0) + goto cleanup; + } + qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); -- 1.9.3
https://bugzilla.redhat.com/show_bug.cgi?id=1141621 As part of attach processing, assign the device aliases by calling qemuAssignDeviceAliases during qemuDomainQemuAttach once all the devices are found after the qemuParseCommandLinePid processing. This will alleviate a symptom that caused a libvirtd crash during an attempted device detach. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e2b356..57daa67 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14862,6 +14862,9 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, if (qemuCanonicalizeMachine(def, qemuCaps) < 0) goto cleanup; + if (qemuAssignDeviceAliases(def, qemuCaps) < 0) + goto cleanup; + if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) goto cleanup; -- 1.9.3
ping? Tks, John On 10/08/2014 08:08 PM, John Ferlan wrote:
v2 is here:
http://www.redhat.com/archives/libvir-list/2014-September/msg01575.html
As Michal pointed out in his review - other devices could have the same issue - so take care of each of them separately (I already knew the answer to my last question...)
Totally different approach this time - rather than error out, follow the example I from the Controller Detach code which will generate an alias for the device (although it did miss one minor check to see if it was already set leading to a potential memory leak since the alias code would overwrite whatever was there).
Note: In a way 1/7 was already ACK'd - I just hadn't separated it yet for a push and wanted to keep these closer together when/if they were pushed.
Whether 7/7 is now necessary is debateable - I keep it only for completeness and environment setup in much the same way the start code handles aliases.
John Ferlan (7): virsh: Adjust the text in man page regarding qemu-attach hotplug: Check for alias in controller detach hotplug: Check for alias in disk detach hotplug: Check for alias in hostdev detach hotplug: Check for alias in chrdev detach hotplug: Check for alias in net detach qemu-attach: Assign device aliases
src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_hotplug.c | 28 +++++++++++++++++++++++++++- tools/virsh.pod | 5 +++-- 3 files changed, 33 insertions(+), 3 deletions(-)
Ping again Tks, John On 10/08/2014 08:08 PM, John Ferlan wrote:
v2 is here:
http://www.redhat.com/archives/libvir-list/2014-September/msg01575.html
As Michal pointed out in his review - other devices could have the same issue - so take care of each of them separately (I already knew the answer to my last question...)
Totally different approach this time - rather than error out, follow the example I from the Controller Detach code which will generate an alias for the device (although it did miss one minor check to see if it was already set leading to a potential memory leak since the alias code would overwrite whatever was there).
Note: In a way 1/7 was already ACK'd - I just hadn't separated it yet for a push and wanted to keep these closer together when/if they were pushed.
Whether 7/7 is now necessary is debateable - I keep it only for completeness and environment setup in much the same way the start code handles aliases.
John Ferlan (7): virsh: Adjust the text in man page regarding qemu-attach hotplug: Check for alias in controller detach hotplug: Check for alias in disk detach hotplug: Check for alias in hostdev detach hotplug: Check for alias in chrdev detach hotplug: Check for alias in net detach qemu-attach: Assign device aliases
src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_hotplug.c | 28 +++++++++++++++++++++++++++- tools/virsh.pod | 5 +++-- 3 files changed, 33 insertions(+), 3 deletions(-)
On 09.10.2014 02:08, John Ferlan wrote:
v2 is here:
http://www.redhat.com/archives/libvir-list/2014-September/msg01575.html
As Michal pointed out in his review - other devices could have the same issue - so take care of each of them separately (I already knew the answer to my last question...)
Totally different approach this time - rather than error out, follow the example I from the Controller Detach code which will generate an alias for the device (although it did miss one minor check to see if it was already set leading to a potential memory leak since the alias code would overwrite whatever was there).
Note: In a way 1/7 was already ACK'd - I just hadn't separated it yet for a push and wanted to keep these closer together when/if they were pushed.
Whether 7/7 is now necessary is debateable - I keep it only for completeness and environment setup in much the same way the start code handles aliases.
John Ferlan (7): virsh: Adjust the text in man page regarding qemu-attach hotplug: Check for alias in controller detach hotplug: Check for alias in disk detach hotplug: Check for alias in hostdev detach hotplug: Check for alias in chrdev detach hotplug: Check for alias in net detach qemu-attach: Assign device aliases
src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_hotplug.c | 28 +++++++++++++++++++++++++++- tools/virsh.pod | 5 +++-- 3 files changed, 33 insertions(+), 3 deletions(-)
ACK Michal
On 10/28/2014 07:54 PM, Michal Privoznik wrote:
On 09.10.2014 02:08, John Ferlan wrote:
v2 is here:
http://www.redhat.com/archives/libvir-list/2014-September/msg01575.html
As Michal pointed out in his review - other devices could have the same issue - so take care of each of them separately (I already knew the answer to my last question...)
Totally different approach this time - rather than error out, follow the example I from the Controller Detach code which will generate an alias for the device (although it did miss one minor check to see if it was already set leading to a potential memory leak since the alias code would overwrite whatever was there).
Note: In a way 1/7 was already ACK'd - I just hadn't separated it yet for a push and wanted to keep these closer together when/if they were pushed.
Whether 7/7 is now necessary is debateable - I keep it only for completeness and environment setup in much the same way the start code handles aliases.
John Ferlan (7): virsh: Adjust the text in man page regarding qemu-attach hotplug: Check for alias in controller detach hotplug: Check for alias in disk detach hotplug: Check for alias in hostdev detach hotplug: Check for alias in chrdev detach hotplug: Check for alias in net detach qemu-attach: Assign device aliases
src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_hotplug.c | 28 +++++++++++++++++++++++++++- tools/virsh.pod | 5 +++-- 3 files changed, 33 insertions(+), 3 deletions(-)
ACK
Michal
Pushed - thanks John
participants (2)
-
John Ferlan -
Michal Privoznik