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

[Erik Skultety]

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 at 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...

> +            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.

Reviewed-by: Erik Skultety <eskultet at redhat.com>