On Fri, 2016-01-22 at 15:00 +0000, Daniel P. Berrange wrote:
IMHO you should leave virHostdevReattachPCIDevice alone, and just make this new method call that one. In later patches you are calling this reattachPCIDevices() method with a single device, forcing you to put it into a temporary virPCIDeviceListPtr before calling it. If you keep virHostdevReattachPCIDevice then you can call it directly and avoid creating temporary lists.
When I started splitting off this code (which, as explained in the cover letter, is something I'm doing in preparation of an upcoming series) I planned to use the device list for more than just iterating through its members. Turns out that I won't need to do that after all, so having the loop in the caller makes more sense. I'll change it.
@@ -883,7 +917,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); I'm inclined to say that all the changes above this point should have been a separate commit from the commit that introduces the reattachPCIDevices method, as this is really mixing 2 sets of unrelated changes in one commit.
I concede that I could have done a better job at isolating independent changes, I'll try to improve with v2 :) In that regard, would you rather have the comments dealt with in a separate commit, even if that would mean have them not reflect the code mid-series? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team