Re: [libvirt] [PATCH 6/8] Pass activeDevs and inactiveDevs to virPCIDeviceUnbindFromStub and virPCIDeviceBindToStub

On Fri, 2015-10-30 at 05:00 +0530, Shivaprasad G Bhat wrote: > The inactiveDevs need to be selectively altered for more than one > device in case of vfio devices. So, pass the whole list. > > Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; > --- > src/util/virpci.c | 38 +++++++++++++++++++++----------------- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/src/util/virpci.c b/src/util/virpci.c > index 2709ddd..6c24a81 100644 > --- a/src/util/virpci.c > +++ b/src/util/virpci.c > @@ -1129,7 +1129,9 @@ static int virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev, char *driver, char > } > > static int > -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) > +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev, > + virPCIDeviceListPtr activeDevs ATTRIBUTE_UNUSED, > + virPCIDeviceListPtr inactiveDevs) > { > int result = -1; > char *drvdir = NULL; > @@ -1177,6 +1179,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) > reprobe: > if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0) > goto cleanup; > + /* Steal the dev from list inactiveDevs */ > + if (inactiveDevs) > + virPCIDeviceListDel(inactiveDevs, dev); > > result = 0; > > @@ -1195,7 +1200,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) > > static int > virPCIDeviceBindToStub(virPCIDevicePtr dev, > - const char *stubDriverName) > + const char *stubDriverName, > + virPCIDeviceListPtr activeDevs, > + virPCIDeviceListPtr inactiveDevs) > { > int result = -1; > bool reprobe = false; > @@ -1317,6 +1324,14 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, > goto cleanup; > } > > + /* Add *a copy of* the dev into list inactiveDevs, if > + * it's not already there. > + */ Just close the comment in the previous line. > + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && > + virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) { Really weird alignment here... > + result = -1; > + } > + > cleanup: > VIR_FREE(stubDriverPath); > VIR_FREE(driverLink); > @@ -1324,7 +1339,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev, > > if (result < 0) { > VIR_FREE(newDriverName); > - virPCIDeviceUnbindFromStub(dev); > + virPCIDeviceUnbindFromStub(dev, activeDevs, NULL); Why pass NULL instead of inactiveDevs here?

> } else { > VIR_FREE(dev->stubDriver); > dev->stubDriver = newDriverName; > @@ -1371,16 +1386,9 @@ virPCIDeviceDetach(virPCIDevicePtr dev, > return -1; > } > > - if (virPCIDeviceBindToStub(dev, dev->stubDriver) < 0) > - return -1; > - > - /* Add *a copy of* the dev into list inactiveDevs, if > - * it's not already there. > - */ > - if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev) && > - virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) { > + if (virPCIDeviceBindToStub(dev, dev->stubDriver, > + activeDevs, inactiveDevs) < 0) > return -1; > - } > > return 0; > } > @@ -1396,13 +1404,9 @@ virPCIDeviceReattach(virPCIDevicePtr dev, > return -1; > } > > - if (virPCIDeviceUnbindFromStub(dev) < 0) > + if (virPCIDeviceUnbindFromStub(dev, activeDevs, inactiveDevs) < 0) > return -1; > > - /* Steal the dev from list inactiveDevs */ > - if (inactiveDevs) > - virPCIDeviceListDel(inactiveDevs, dev); > - > return 0; > } Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team