[libvirt] [PATCH 1/7] hostdev: Add reattachPCIDevices()
Daniel P. Berrange
berrange at redhat.com
Fri Jan 22 16:58:07 UTC 2016
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 :|
More information about the libvir-list
mailing list