Re: [libvirt] [PATCH 08/11] conf: Prefer pcie-to-pci-bridge to dmi-to-pci-bridge
On 04/04/2018 05:04 AM, Andrea Bolognani wrote:
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...
I guess part of me is thinking of the "next" bridge that gets added
where someone just copies what you did and creates a needXXXToPCIBridge
boolean, adds another setting to true, another setting to false, and
then needs to decide which to prefer and thus has to muck with setting
the others based on the new one.
In the long run - it seems the setting of the bool is based on need,
then the second part is to decide what type of thing to add. That's why
I thought it would be easier to read with what I presented.
Again I'm fine with the way you had it, so it's your call.
John