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@xxxxxxxxx> > > > --- > > > 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. > > > > > > > + accept_ra = virNetDevIPGetAcceptRA(ifname); > > > > > > - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > > > - return -1; > > > + VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d", > > > + ifname, ifindex, accept_ra); > > > + > > > + if (!ifname || > > ... we'd return failure here anyway. > > > > > + (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)) { > > > + return -1; > > > + } > > > + > > > + data->hasRARoutes = true; > > > + return 0; > > I think ^this return should be part of the next patch where it IMHO makes more > > sense. > > > I included it here because the code is still correct with it in, and it > makes the next patch more self-contained (the only code added is the code > directly related to checking the nexthop interfaces). True, I thought of a scenario where someone reads through the git history in which case it looks weird, lacking the "look-ahead" context that there was a follow up patch which cleared it up, but whatever, I think too much. > > > (truthfully, this started out as a single patch, and I split it into parts > to make it easier to review. I'd be just as happy to turn it back into a > single patch :-) One can tell that it was split for purposes of the review :). Your call with the adjustments. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
