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