Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Maz The Northener <mazziesaccount@gmail.com>]

On Wed, Nov 2, 2011 at 6:21 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Wed, 02 Nov 2011 09:09:51 +0200
> Matti Vaittinen <matti.vaittinen@nsn.com> wrote:
>

>> +
>> +
>
> Gratuitous unnecessary whitespace added.
>

I will fix the whitespace errors.

>> +     int allow_create = 1;
>> +     int replace_required = 0;
>> +
>> +
>
> Personally, I dislike boolean flag variables, it is often a sign
> of poorly executed logic flow
>
>

I tend to agree to some level. However sometimes well named variables
make following code easier. And I do not claim the logic flow couldn't
be improved, but I'm not the one going to make big changes to FIB
handling. I would probably end up breaking something.

>>       __be32  dir = 0;
>>       __u32   sernum = fib6_new_sernum();
>>
>>       RT6_TRACE("fib6_add_1\n");
>>
>> +     if (NULL != info &&
>> +         NULL != info->nlh &&
>> +         (info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
>> +             replace_required = 1;
>> +     }
>> +     if (NULL != info &&
>> +         NULL != info->nlh &&
>> +         !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
>> +                     allow_create = 0;
>> +     }
>
> I would move the flag calculation out to the caller and keep fib6_add_1
> clean.

Can be done, I just didn't want to introduce two more parameters in
function call. But I'll do that.


>>                       sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
>>                                       sizeof(struct in6_addr), rt->rt6i_src.plen,
>> -                                     offsetof(struct rt6_info, rt6i_src));
>> +                                     offsetof(struct rt6_info, rt6i_src),
>> +                                     info);
>>
>> +                     if (-ENOENT == PTR_ERR(sn)) {
>> +                             err = -EINVAL;
>
> This is not how to use PTR_ERR; the more common convention is:
>
>                if (IS_ERR(sn)) {
>                        err = PTR_ERR(sn);
> ...

Makes sense.

>
>
>> +                             sn = NULL;
>> +                     }
>>                       if (sn == NULL)
>>                               goto st_failure;
>>               }
>> diff -uNr linux-3.1-rc4.orig/net/ipv6/route.c linux-3.1-rc4.new/net/ipv6/route.c
>> --- linux-3.1-rc4.orig/net/ipv6/route.c       2011-11-01 14:01:55.000000000 +0200
>> +++ linux-3.1-rc4.new/net/ipv6/route.c        2011-10-27 10:45:05.000000000 +0300
>> @@ -1223,9 +1223,18 @@
>>       if (cfg->fc_metric == 0)
>>               cfg->fc_metric = IP6_RT_PRIO_USER;
>>
>> -     table = fib6_new_table(net, cfg->fc_table);
>> +     err = -ENOBUFS;
>> +     if (NULL != cfg->fc_nlinfo.nlh &&
>> +         !(cfg->fc_nlinfo.nlh->nlmsg_flags&NLM_F_CREATE)) {
>> +             table = fib6_get_table(net, cfg->fc_table);
>> +             if (table == NULL) {
>> +                     printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
>> +                     table = fib6_new_table(net, cfg->fc_table);
>> +             }
>> +     } else {
>> +             table = fib6_new_table(net, cfg->fc_table);
>> +     }
>>       if (table == NULL) {
>> -             err = -ENOBUFS;
>>               goto out;
>>       }
>
> This could be a separate patch

Allright. I'll break up the patch. Thanks for taking the time to check
this. I'll send new patches soonish.

--Matti Vaittinen.