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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list