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

[Laine Stump]

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


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.


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


(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 :-)


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