On Fri, Jan 22, 2016 at 05:39:33PM +0100, Andrea Bolognani wrote:
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?
I don't mind really. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|