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!