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

[Laine Stump]

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


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