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!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
