Re: [libvirt] [PATCH] util: fix botched check for new netlink request filters
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!