[libvirt] [PATCH] util: fix botched check for new netlink request filters

[Laine Stump]

On 12/21/2012 04:00 PM, Eric Blake wrote:
> On 12/21/2012 01:48 PM, Laine Stump wrote:
>> This is an adjustment to the fix for
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=889319
>>
>> to account for two bonehead mistakes I made.
>>
>> commit ac2797cf2af2fd0e64c58a48409a8175d24d6f86 attempted to fix a
>> problem with netlink in newer kernels requiring an extra attribute
>> with a filter flag set in order to receive an IFLA_VFINFO_LIST from
>> netlink. Unfortunately, the #ifdef that protected against compiling it
>> in on systems without the new flag went a bit too far, assuring that
>> the new code would *never* be compiled.
>>
>> The first problem was that, while some IFLA_* enum values are also
>> not, so checking to see if it's #defined is not a valid method of
> Grammar.  Did you miss a word somewhere in there?

Yes :-)

>
> <rant>Why do the kernel developers behave so inconsistently? I much
> prefer interfaces where an enum value is ALSO added as a define to
> itself, to make it easy to probe which features are available in the
> current set of headers.</rant>

The really frustrating part is the *some of* the enum values in this set
have #defines, and some of them don't.

>
>> determining whether or not to add the attribute. Fortunately, the flag
>> that is being set (RTEXT_FILTER_VF) *is* #defined, and it is never
>> present if IFLA_EXT_MASK isn't, so it's sufficient to just check for
>> that flag.
> My bad in the first review for not actually looking up the kernel
> headers for those values.
>
>> And to top it off, due to the code not actually compiling when I
>> thought it did, I didn't realize that I'd been given the wrong arglist
>> to nla_put() - you can't just send a const value to nla_put, you have
>> to send it a pointer to memory containing what you want to add to the
>> message, along with the length of that memory.
>>
>> This time I've actually sent the patch over to the other machine
>> that's expriencing the problem, applied it to the branch being used
> s/expriencing/experiencing/

Right.

>
>> (0.10.2) and verified that it works properly, i.e. it does fix the
>> problem it's supposed to fix. :-/
>> ---
>>  src/util/virnetdev.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index e5a0d6d..898e77a 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1275,13 +1275,17 @@ virNetDevLinkDump(const char *ifname, int ifindex,
>>              goto buffer_too_small;
>>      }
>>  
>> -# if defined(IFLA_EXT_MASK) && defined(RTEXT_FILTER_VF)
>> +# if defined(RTEXT_FILTER_VF)
> Now that you are down to one term, it might be simpler to write:
>
> # ifdef RTEXT_FILTER_VF

Okay.

>
>>      /* if this filter exists in the kernel's netlink implementation,
>>       * we need to set it, otherwise the response message will not
>>       * contain the IFLA_VFINFO_LIST that we're looking for.
>>       */
>> -    if (nla_put(nl_msg, IFLA_EXT_MASK, RTEXT_FILTER_VF) < 0)
>> +    {
>> +    uint32_t ifla_ext_mask = RTEXT_FILTER_VF;
>> +
>> +    if (nla_put(nl_msg, IFLA_EXT_MASK, sizeof(ifla_ext_mask), &ifla_ext_mask) < 0)
>>         goto buffer_too_small;
>> +    }
> Thanks to the extra {} scoping, you need to indent the body four more
> spaces (and probably wrap a long line).

Ah, right. I forgot to set libvirt indent style on the buffer, and am
not used to extra braces like this, so didn't even notice that the
indentation was wrong.
>
>>  # endif
>>  
>>      if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen,
>>
> ACK with those things fixed.
>

Okay, I've fixed those and pushed.

Thanks for the last minute review!