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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 3 Nov 2011 08:55:34 +0200
From:	Maz The Northener <mazziesaccount@...il.com>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	Matti Vaittinen <matti.vaittinen@....com>, davem@...emloft.net,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

On Wed, Nov 2, 2011 at 6:21 PM, Stephen Hemminger <shemminger@...tta.com> wrote:
> On Wed, 02 Nov 2011 09:09:51 +0200
> Matti Vaittinen <matti.vaittinen@....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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html