On Tue, 2018-04-03 at 19:13 -0400, John Ferlan wrote:
[...]
> > needDMIToPCIBridge = true;
> > + needPCIeToPCIBridge = true;
> > for (i = 0; i < addrs->nbuses; i++) {
> > if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
> > needDMIToPCIBridge = false;
> > + needPCIeToPCIBridge = false;
> > break;
> > }
> > }
> > - if (needDMIToPCIBridge && add == 1) {
> > +
> > + /* Prefer pcie-to-pci-bridge, fall back to dmi-to-pci-bridge */
> > + if (addrs->isPCIeToPCIBridgeSupported)
> > + needDMIToPCIBridge = false;
> > + else
> > + needPCIeToPCIBridge = false;
>
> The above seems a bit extra work and is a bit hard to read... Could the
> previous for loop change to use a new bool "needXToPCIBridge"...
>
> Then, I think it would just be:
>
> if (addrs->isPCIeToPCIBridgeSupported)
> needPCIeToPCIBridge = needXToPCIBridge;
> else
> needDMIToPCIBridge = needXToPCIBridge;
>
> with the following just being if (needXToPCIBridge && add == 1)
>
> What you have works, just seems to be overkill or maybe I'm missing
> something subtle ;-).
I don't think you missed something, both version should work just
as fine. I happen to find my version easier to read than yours, so
I'd stick with that if you're okay with it - I guess it's just a
matter of preference at the end of the day...
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
