Re: [libvirt] [PATCH 3/4] util: use nlmsg_find_attr() instead of an open-coded loop

On 1/10/19 10:38 AM, Erik Skultety wrote: > On Thu, Jan 10, 2019 at 09:34:35AM -0500, Laine Stump wrote: > > On 1/10/19 9:09 AM, Erik Skultety wrote: > > > On Wed, Jan 09, 2019 at 12:43:14PM -0500, Laine Stump wrote: > > > > This is about the same number of code lines, but is simpler, and more > > > > consistent with what will be added to check another attribute in a > > > > coming patch. > > > > > > > > As a side effect, it > > > > > > > > Resolves: https://bugzilla.redhat.com/1583131 > > > > Signed-off-by: Laine Stump <laine(a)laine.org&gt; > > > > --- > > > > src/util/virnetdevip.c | 53 ++++++++++++++++++------------------------ > > > > 1 file changed, 23 insertions(+), 30 deletions(-) > > > > > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > > > index 72048e4b45..c032ecacfc 100644 > > > > --- a/src/util/virnetdevip.c > > > > +++ b/src/util/virnetdevip.c > > > > @@ -529,49 +529,42 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > > > > void *opaque) > > > > { > > > > struct rtmsg *rtmsg = NLMSG_DATA(resp); > > > > - int accept_ra = -1; > > > > - struct rtattr *rta; > > > > struct virNetDevIPCheckIPv6ForwardingData *data = opaque; > > > > - int len = RTM_PAYLOAD(resp); > > > > - int oif = -1; > > > > + struct rtattr *rta_attr; > > > > + int accept_ra = -1; > > > > + int ifindex = -1; > > > > VIR_AUTOFREE(char *) ifname = NULL; > > > > > > > > /* Ignore messages other than route ones */ > > > > if (resp->nlmsg_type != RTM_NEWROUTE) > > > > return 0; > > > > > > > > - /* Extract a device ID attribute */ > > > > - VIR_WARNINGS_NO_CAST_ALIGN > > > > - for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { > > > > - VIR_WARNINGS_RESET > > > > - if (rta->rta_type == RTA_OIF) { > > > > - oif = *(int *)RTA_DATA(rta); > > > > - > > > > - /* Should never happen: netlink message would be broken */ > > > > - if (ifname) { > > > > - VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif); > > > > - VIR_WARN("Single route has unexpected 2nd interface " > > > > - "- '%s' and '%s'", ifname, ifname2); > > > > - break; > > > > - } > > > > - > > > > - if (!(ifname = virNetDevGetName(oif))) > > > > - return -1; > > > > - } > > > > - } > > > > - > > > > /* No need to do anything else for non RA routes */ > > > > if (rtmsg->rtm_protocol != RTPROT_RA) > > > > return 0; > > > > > > > > - data->hasRARoutes = true; > > > > + rta_attr = (struct rtattr *)nlmsg_find_attr(resp, sizeof(struct rtmsg), RTA_OIF); > > > > + if (rta_attr) { > > > > + /* This is a single path route, with interface used to reach > > > > + * nexthop in the RTA_OIF attribute. > > > > + */ > > > > + ifindex = *(int *)RTA_DATA(rta_attr); > > > > + ifname = virNetDevGetName(ifindex); > > > > > > > > - /* Check the accept_ra value for the interface */ > > > > - accept_ra = virNetDevIPGetAcceptRA(ifname); > > > > - VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); > > > > + if (ifname) > > > I'd put > > > > > > if (!ifname) > > > return -1; > > > > > > ^ here instead, since having (null) in the DEBUG output doesn't really help > > > anyone and... > > > > I disagree with that. Having a null ifname means that the ifindex sent as > > RTA_OIF couldn't be resolved to a proper name. Allowing the code to make it > > through to the VIR_DEBUG and print out the offending ifindex (along with > > "(null)") will give us more info to further investigate. > In which case it qualifies as a warning, I am not entirely convinced we want > that information to be lost in the flood of DEBUG messages. If virNetDevGetName() fails to convert, it logs an error. Of course then you just end up with an error message saying that virNetDevGetName() failed to convert ifindex "blah" to a name, and no context about what was happening when that occurred. I still think that additionally having the debug log available regardless of whether or not virNetDevGetName() fails could be useful some day, and it's not taking any extra effort to do so, but if it really rubs you the wrong way, then I'll adjust it :-)