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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list