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