[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 laine org>
> > > ---
> > >   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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]