On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
This ensures the behavior for reattach is consistent, no matter how it was triggered (eg. 'virsh nodedev-reattach', 'virsh detach-device' or shutdown of a domain that is configured to use hostdevs).
Similar to 2/9 - using virHostdevOnlyReattachPCIDevice will result in a call to virPCIDeviceGetStubDriver and virPCIDeviceWaitForCleanup - for this path I assume this is thus desired.
--- src/util/virhostdev.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 586937e..f40d636 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1637,10 +1637,24 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, return ret; }
+/** + * virHostdevPCINodeDeviceReAttach:
^^ Oy ReAttach vs. Reattach is an eye test ;-)
+ * @hostdev_mgr: hostdev manager + * @pci: PCI device
Perhaps better to indicate a "new"ly generated PCI device that does not track the internal reattach states and other state information such as the stub driver. IOW: this is not a copy of an [in]activePCIHostdevs element
+ * + * Reattach a specific PCI device to the host. + * + * This function makes sure the device is not in use before reattaching it + * to the host; if the device has already been reattached to the host, the + * operation is considered successful. + * + * Returns: 0 on success, <0 on failure + */ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr pci) { + virPCIDevicePtr actual; struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, false}; int ret = -1; @@ -1651,10 +1665,23 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto out;
- virPCIDeviceReattachInit(pci); + /* We want this function to be idempotent, so if the device has already + * been removed from the inactive list (and is not active either, as + * per the check above) just return right away. We also need to retrieve + * the actual device from the inactive list because that's the one that + * contains state information such as the stub driver */ + if (!(actual = virPCIDeviceListFind(hostdev_mgr->inactivePCIHostdevs, + pci))) {
As noted in my 1/9 review - this code path passes the 'actual' dev onlist... The call we're about to make is going to repeat this search. Something we could avoid if performance of list searches were a concern. No other issues - John
+ VIR_DEBUG("PCI device %s is already attached to the host", + virPCIDeviceGetName(pci)); + ret = 0; + goto out; + }
- if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + /* Reattach the device. We don't want to skip unmanaged devices in + * this case, because the user explicitly asked for the device to + * be reattached to the host driver */ + if (virHostdevOnlyReattachPCIDevice(hostdev_mgr, actual, false) < 0) goto out;
ret = 0;