On Wed, Sep 24, 2014 at 06:19:14PM -0400, John Ferlan wrote: > > > On 09/22/2014 05:23 PM, Eric Blake wrote: > > On 09/22/2014 02:49 PM, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1141621 > >> > >> Using qemu-attach to attach to a qemu created process and then > >> attempting to virsh detach-interface caused a libvirtd crash > >> since the assumption was that device aliases were in place and > >> that assumption is not necessarily true for the qemu-attach environment. > > > > Hmm. I'm wondering whether it is better to patch qemu-attach to populate > > alias information at attach time rather than having to chase all the > > places that will later fail when the assumption is broken. While your > > patch is certainly more appealing for being smaller, it makes me wonder > > if it is the only such place bitten by the assumptions. > > > > I took this approach mainly because it seemed that generating aliases in > the qemu-attach path was explicitly not done for a reason while it was > done for the qemuConnectDomainXMLToNative and qemuDomainQemuAttach > paths. For each of the other attach paths, there are the separate calls > to the specific qemuAssignDevice*Alias functions. So it does seem that > this qemu-attach path is the only current one without the aliases set. The difficulty in assigning aliases is that the QEMU cli args may be in a format we don't expect. That said, when attaching we already split the args into two groups - those we can parse into something we understand and those we just leave as passthrough args. I guess we could make setting an alias be a prerequisite for the former group. > If there is some path in the future that doesn't set the alias properly, > then at the very least we don't have a libvirtd crash at least for this > path. I think it is a good idea to be paranoid about missing alias values, though I wouldn't be surprised if there are many, many cases where a missing alias could cause us trouble. > > FWIW: If I modify the patch as follows: > > - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && > - detach->info.alias) { > + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > + if (!detach->info.alias) { > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + ("device alias not found: cannot detach net " > + "device not properly attached")); > + qemuDomainObjExitMonitor(driver, vm); > + goto cleanup; > + } > > then rerun the sequence of steps, I get: > > error: Failed to detach interface > error: operation failed: device alias not found: cannot detach net > device not properly attached > > > If I then add qemuAssignDeviceAliases() as described below and rerun, > the error changes to: > > error: Failed to detach interface > error: operation failed: detaching net0 device failed: Device 'net0' not > found > > > Since the original start didn't use the "-netdev/-device" syntax that > would be expected if the DRIVE capability was set. So no worse than we > were before. > > Should I just post the additional patch calling qemuAssignDeviceAliases > and go from there? So you're testing with a setup where the user had used legacy -net setup. It is reasonable to say we will never expect that to succeeed at hotunplug, so from that point of view it doesn't matter whether we try assigning aliases or just catch the missing alias. If, however, we look at it from the POV of a user who *has* used the correct -netdev + -device syntax with qemu-attach, then it is reasonable to expect that hotunplug should succeeed. So for that scenario, calling the function qemuAssignDeviceAliases is the only approach that will work. So I guess this is the approach we should take, since it makes the good scenarios work and is no worse for the bad scenarios. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list