On 10/08/2014 12:08 PM, Michal Privoznik wrote: > On 25.09.2014 17:00, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1141621 >> >> The crash in this case was because the qemu-attach code did not create >> aliases for qemu cli args. When the detach-interface code was called >> it assumed aliases were set resulting in a core when dereferencing the >> still NULL alias. >> >> Adding a call to qemuAssignDeviceAliases() resolves the path for >> qemu-attach; however, to prevent future issues an additional check >> for a NULL value is made and an error provided in the deatch path. >> >> Add some more verbiage to the virsh man page. >> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> >> v1 is here: >> http://www.redhat.com/archives/libvir-list/2014-September/msg01331.html >> >> Changes since v1: >> - Add the call to qemuAssignDeviceAliases() in qemuDomainQemuAttach(). >> - Move the check for NULL alias inside the CAPS_DEVICE check and emit >> an error rather than trying to remove as an "else" condition. >> >> src/qemu/qemu_driver.c | 3 +++ >> src/qemu/qemu_hotplug.c | 7 +++++++ >> tools/virsh.pod | 5 +++-- >> 3 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 117138a..ef4ecd2 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -14746,6 +14746,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; >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index d631887..daebe82 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -3521,6 +3521,13 @@ qemuDomainDetachNetDevice(virConnectPtr conn, >> >> qemuDomainObjEnterMonitor(driver, vm); >> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { >> + if (!detach->info.alias) { >> + virReportError(VIR_ERR_OPERATION_FAILED, "%s", >> + _("device alias not found: cannot delete the " >> + "net device")); >> + qemuDomainObjExitMonitor(driver, vm); >> + goto cleanup; >> + } > > Is there a reason to not check this upfront rather than this late in the > process? And I don't think that net devices are the only thing affected > here. For instance a virtio disks seems vulnerable too (well, at first > glance on the code). > Recall the environment - 'qemu-attach' and the bz... The bz is specific for network attach/detach, so I wasn't considering disks, controllers, hostdevs, and chrdevs when first working through this particular problem. Although based on some follow-up work (more in a bit) I've learned a bit more! Note that the check is specific to when the CAPS_DEVICE is set (IOW, using the newer device naming syntax). In this case, someone used -net/-net instead of -netdev/-device and the assumption in the code once we get to this point that the alias would filled in which without the qemuAssignDeviceAliases in qemuDomainQemuAttach wouldn't be the case. A side note is that qemuParseCommandLine() which is what qemu-attach will use when parsing the command line of the pid that's being attached to doesn't even handle -netdev, but that's a different issue. Anyway, even though the alias is there "now" the concern was the effect if some other path called into here without the alias. Since it only mattered when CAPS_DEVICE was set, that's why the check is where it is. The question relating to why not in some other caller is valid; however, not fool proof since none of the qemuDomainDetach* API's in qemu_hotplug.c are static. Making the check earlier in the caller of all those API's qemuDomainDetachDeviceLive is possible, but would have to avoid the check for DEVICE_LEASE since it doesn't use info.alias. As for disks (and perhaps other devices) - funny you should mention that... There's another bz (844845) that I have some patches I'm working on which actually allow a qemu using the 'id="drive-<alias>"' to work. As of now, starting qemu using the "id=" instead of "index=" will not allow the qemu-attach to succeed. And yes, I do believe given what I now know about the code, sure the disk, controller, hostdev, and chrdev paths could all be affected by the assumption of info.alias being set (although I don't have working examples of such a detachment). This is the long winded way of noting yes, it's possible; however, the example or bz doesn't exist. I can use this opportunity to make those checks (in much the same manner) if desired though... Also I'd need to know if there should be a separate patch for each (turning a 1 patch change into 5 or 6). John >> if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { > > The other possibility would be to make qemuMonitorDelDevice fail on > @devalias == NULL. > >> qemuDomainObjExitMonitor(driver, vm); >> virDomainAuditNet(vm, detach, NULL, "detach", false); >> 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>... >> > > Michal > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list