Re: [libvirt] [PATCH 1/7] hostdev: Add reattachPCIDevices()

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?