This function was called in three places, and in each the call was qualified by a slightly different conditional. In reality, this function should only be called for a hostdev if all of the following are true: 1) mode='subsystem' 2) type='pci' 3) there is a parent device definition which is an <interface> (VIR_DOMAIN_DEVICE_NET) We can simplify the callers and make them more consistent by checking these conditions at the top ov qemuDomainHostdevNetConfigRestore and returning 0 if one of them isn't satisfied. The location of the call to qemuDomainHostdevNetConfigRestore() has also been changed in the hot-plug case - it is moved into the caller of its previous location (i.e. from qemuDomainRemovePCIHostDevice() to qemuDomainRemoveHostDevice()). This was done to be more consistent about which functions pay attention to whether or not this is one of the special <interface> hostdevs or just a normal hostdev - qemuDomainRemoveHostDevice() already contained a call to networkReleaseActualDevice() and virDomainNetDefFree(), so it makes sense for it to also handle the resetting of the device's MAC address and vlan tag (which is what's done by qemuDomainHostdevNetConfigRestore()). --- src/qemu/qemu_hostdev.c | 33 ++++++++++++++------------------- src/qemu/qemu_hotplug.c | 13 ++++--------- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 1f7bf56..57ab28c 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -466,6 +466,15 @@ qemuDomainHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, bool port_profile_associate = false; int isvf; + /* This is only needed for PCI devices that have been defined + * using <interface type='hostdev'>. For all others, it is a NOP. + */ + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI || + hostdev->parent.type != VIR_DOMAIN_DEVICE_NET || + !hostdev->parent.data.net) + return 0; + isvf = qemuDomainHostdevIsVirtualFunction(hostdev); if (isvf <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -799,14 +808,9 @@ inactivedevs: } resetvfnetconfig: - for (i = 0; last_processed_hostdev_vf != -1 && - i < last_processed_hostdev_vf; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET && - hostdev->parent.data.net) { - qemuDomainHostdevNetConfigRestore(hostdev, cfg->stateDir); - } - } + for (i = 0; + last_processed_hostdev_vf != -1 && i < last_processed_hostdev_vf; i++) + qemuDomainHostdevNetConfigRestore(hostdevs[i], cfg->stateDir); reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { @@ -1270,17 +1274,8 @@ qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver, * For SRIOV net host devices, unset mac and port profile before * reset and reattach device */ - for (i = 0; i < nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - continue; - if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET && - hostdev->parent.data.net) { - qemuDomainHostdevNetConfigRestore(hostdev, cfg->stateDir); - } - } + for (i = 0; i < nhostdevs; i++) + qemuDomainHostdevNetConfigRestore(hostdevs[i], cfg->stateDir); for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b6ae218..a6a1591 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2519,18 +2519,10 @@ qemuDomainRemovePCIHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; virPCIDevicePtr pci; virPCIDevicePtr activePci; - /* - * For SRIOV net host devices, unset mac and port profile before - * reset and reattach device - */ - if (hostdev->parent.data.net) - qemuDomainHostdevNetConfigRestore(hostdev, cfg->stateDir); - virObjectLock(driver->activePciHostdevs); virObjectLock(driver->inactivePciHostdevs); pci = virPCIDeviceNew(subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, @@ -2551,7 +2543,6 @@ qemuDomainRemovePCIHostDevice(virQEMUDriverPtr driver, virObjectUnlock(driver->inactivePciHostdevs); qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); - virObjectUnref(cfg); } static void @@ -2587,6 +2578,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainNetDefPtr net = NULL; virDomainEventPtr event; size_t i; @@ -2618,6 +2610,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainAuditHostdev(vm, hostdev, "detach", true); + qemuDomainHostdevNetConfigRestore(hostdev, cfg->stateDir); + switch ((enum virDomainHostdevSubsysType) hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: qemuDomainRemovePCIHostDevice(driver, vm, hostdev); @@ -2646,6 +2640,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, networkReleaseActualDevice(net); virDomainNetDefFree(net); } + virObjectUnref(cfg); } -- 1.8.3.1

This patch (and the two patches that precede it) resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1005682 When libvirt was changed to delay the final cleanup of device removal until the qemu process had signaled it with a DEVICE_DELETED event for that device, the hostdev removal function (qemuDomainRemoveHostDevice()) was written to properly handle the removal of a hostdev that was actually an SRIOV virtual function (defined with <interface type='hostdev'>). However, the function used to search for a device matching the alias name provided in the DEVICE_DELETED message (virDomainDefFindDevice()) would search through the list of netdevs before hostdevs, so qemuDomainRemoveHostDevice() was never called; instead the netdev function, qemuDomainRemoveNetDevice() (which *doesn't* properly cleanup after removal of <interface type='hostdev'>), was called. (As a reminder - each <interface type='hostdev'> results in a virDomainNetDef which contains a virDomainHostdevDef having a parent type of VIR_DOMAIN_DEVICE_NET, and parent.data.net pointing back to the virDomainNetDef; both Defs point to the same device info object (and the info contains the device's "alias", which is used by qemu to identify the device). The virDomainHostdevDef is added to the domain's hostdevs list *and* the virDomainNetDef is added to the domain's nets list, so searching either list for a particular alias will yield a positive result.) This function modifies the qemuDomainRemoveNetDevice() to short circuit itself and call qemu DomainRemoveHostDevice() instead when the actual device is a VIR_DOMAIN_NET_TYPE_HOSTDEV (similar logic to what is done in the higher level qemuDomainDetachNetDevice()) Note that even if virDomainDefFindDevice() changes in the future so that it finds the hostdev entry first, the current code will continue to work properly. --- 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 ddc80a1..e9424d9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2593,6 +2593,12 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virDomainEventPtr event; size_t i; + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* this function handles all hostdev and netdev cleanup */ + qemuDomainRemoveHostDevice(driver, vm, virDomainNetGetActualHostdev(net)); + return; + } + VIR_DEBUG("Removing network interface %s from domain %p %s", net->info.alias, vm, vm->def->name); -- 1.8.3.1