[libvirt] [PATCH] conf: Fix initialization value of 'multi' in PCI address

[Laine Stump]

On 08/10/2016 11:34 AM, Andrea Bolognani wrote:
> On Wed, 2016-08-10 at 17:16 +0200, Boris Fiuczynski wrote:
>>>>>        for (i = 0; i < nAddrNodes; i++) {
>>>>> -        virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 };
>>>>> +        virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
>>>>   
>>>> Honestly, I have no idea what preferences we have for such
>>>> initializations, but I for one prefer initialization to '{0}' which
>>>> guarantees everything to be zeroed anyway.  And will be readable the
>>>> same way even when we change the structure.  Would that work for you as
>>>> well?
>>>   
>>> I think it should either be 0 (as the structure member is
>>> defined as int) or VIR_TRISTATE_SWITCH_ABSENT (as it is used
>>> as virTristateSwitch, according to the comment and other bits
>>> of code). false definitely seems out of place.
>>   
>> Actually this fix was about aligning three code occurrences.
>> These three initialisations can be found here:
>>   
>> qemu/qemu_domain_address.c
>> 1099:            virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
>>   
>> conf/node_device_conf.c
>> 1166:        virPCIDeviceAddress addr = { 0, 0, 0, 0, 0 };
>>   
>> conf/domain_addr.c
>> 572:    virPCIDeviceAddress a = { 0, 0, 0, 0, false };
>>   
>> Setting the VIR_TRISTATE_SWITCH_ABSENT make sense from the data type
>> point of view. Looking at it from the code readability point of view you
>> would have to know that the default of the multifunction is Off and with
>> that in mind it made more sense setting it to false.
> The default is not OFF, though, it's ABSENT :)
>
> In fact, as far as I can tell, OFF isn't ever used explicitly
> either for assignment or comparison. And false is plain wrong
> from a datatype point of view.

(a bit off-topic, but while we're talking about the multifunction option).

For that matter, I've always thought the mere existence of the 
multifunction attribute was completely superfluous. Its proper setting 
can be completely 100% determined from context (I verified this with 
others who know more about PCI than I do).

1) It must not ever be set on a device with non-0 function.
2) It must not ever be set on function 0 if there are no other functions 
in use on the slot
3) It must *always* be set on function 0 if other functions of the slot 
are also used.

For static configuration, we of course know whether or not there are 
other functions in use on the slot, and even once we do support hotplug 
of multiple functions, these multiple functions will *always* be 
plugged/unplugged as a single unit. So really, libvirt should just be 
setting the option according to these rules, and just logging an error 
if someone attempts to set it manually and gets it wrong.