On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
We have 3 pieces of code that do slightly the same thing, but
it varies depending on where it is called:
- virPCIDeviceReattach(). This is where the actual re-attach
work happens;
- virHostdevReattachPCIDevice(). This is a static function from
virhostdev.c that calls virPCIDeviceReattach() after waiting
for device cleanup for the KVM PCI stub driver;
- virHostdevPCINodeDeviceReAttach(). This function also
calls virPCIDeviceReattach(), but setting some device properties
beforehand.
All these extra operations that are done before virPCIDeviceReattach()
can be moved inside the function itself, allowing for the
same re-attach behavior everywhere.
This patch consolidates all these pre-conditions inside the
body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice()
can be removed and the callers can use virPCIDeviceReattach()
directly instead.
Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
---
src/util/virhostdev.c | 43 +++++++++----------------------------------
src/util/virpci.c | 29 ++++++++++++++++++++++++++---
2 files changed, 35 insertions(+), 37 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 53aacb59b4..23be037a39 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,33 +613,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
}
-/*
- * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
- * are locked
- */
-static void
-virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
- virPCIDevicePtr actual)
-{
- /* Wait for device cleanup if it is qemu/kvm */
- if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
- int retries = 100;
- while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
- && retries) {
- usleep(100*1000);
- retries--;
- }
- }
-
- VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
- if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
- VIR_ERROR(_("Failed to re-attach PCI device: %s"),
- virGetLastErrorMessage());
- virResetLastError();
- }
-}
-
static void
virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
virHostdevManagerPtr mgr)
@@ -655,11 +628,17 @@ virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
continue;
- if (virPCIDeviceGetManaged(actual))
- virHostdevReattachPCIDevice(mgr, actual);
- else
+ if (virPCIDeviceGetManaged(actual)) {
+ if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+ VIR_ERROR(_("Failed to re-attach PCI device: %s"),
+ virGetLastErrorMessage());
+ virResetLastError();
+ }
+ } else {
VIR_DEBUG("Not reattaching unmanaged PCI device %s",
virPCIDeviceGetName(actual));
+ }
}
}
@@ -2050,10 +2029,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr,
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data))
goto cleanup;
- virPCIDeviceSetUnbindFromStub(pci, true);
- virPCIDeviceSetRemoveSlot(pci, true);
- virPCIDeviceSetReprobe(pci, true);
-
The real change here, is that these lines ^^
if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs,
mgr->inactivePCIHostdevs) < 0)
goto cleanup;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 75e8daadd5..4594643d3c 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
return 0;
}
+/*
+ * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
+ * are locked
+ */
int
virPCIDeviceReattach(virPCIDevicePtr dev,
virPCIDeviceListPtr activeDevs,
virPCIDeviceListPtr inactiveDevs)
{
+ int ret = -1;
+
if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not reattaching active device %s"), dev->name);
- return -1;
+ goto exit;
+ }
+
+ virPCIDeviceSetUnbindFromStub(dev, true);
+ virPCIDeviceSetRemoveSlot(dev, true);
+ virPCIDeviceSetReprobe(dev, true);
are moved here ^^^
+
+ /* Wait for device cleanup if it is qemu/kvm */
+ if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
+ int retries = 100;
+ while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
+ && retries) {
+ usleep(100*1000);
+ retries--;
+ }
}
and the wait is done here. I'm not against it, but maybe you can explain
more why do you think this change is needed? Did you run into any
troubles? Also, if we set these three attributes unconditionally, what
good is there to keep those APIs around? Also, you forgot to update
testVirPCIDeviceReattachSingle().
if (virPCIDeviceUnbindFromStub(dev) < 0)
- return -1;
+ goto exit;
/* Steal the dev from list inactiveDevs */
if (inactiveDevs) {
@@ -1529,7 +1549,10 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
virPCIDeviceListDel(inactiveDevs, dev);
}
- return 0;
+ ret = 0;
+
+ exit:
This is a useless label.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
