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

[Eric Blake]

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?

<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>

> 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/

> (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

>      /* 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).

>  # endif
>  
>      if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen,
> 

ACK with those things fixed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121221/62c7e614/attachment-0001.sig>