netdev - Re: [PATCH v2] net: #ifdef inet_bind_bucket::ib

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 12 Nov 2008 11:50:29 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Alexey Dobriyan <adobriyan@...il.com>
CC:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH v2] net: #ifdef inet_bind_bucket::ib_net

Alexey Dobriyan a écrit :
> On Tue, Nov 11, 2008 at 04:45:54PM -0800, David Miller wrote:
>> From: Alexey Dobriyan <adobriyan@...il.com>
>> Date: Tue, 11 Nov 2008 14:19:46 +0300
>>
>>> @@ -35,7 +35,9 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
>>>  	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
>>>  
>>>  	if (tb != NULL) {
>>> +#ifdef CONFIG_NET_NS
>>>  		tb->ib_net       = hold_net(net);
>>> +#endif
>>>  		tb->port      = snum;
>>>  		tb->fastreuse = 0;
>>>  		INIT_HLIST_HEAD(&tb->owners);
>> No, this is exactly what we don't want.
>>
>> If you have to add ifdefs to core C files, you're doing something
>> wrong.
> 
> It depends.
> 
>> All the details of ifdef this or ifdef that should be hidden in the
>> header files.
> 
> It depends.
> 
>> You cited an example where there are a ton of ifdefs in some header
>> fule inline function, but that is EXACTLY how this stuff should be
>> done.  Those header files are where such ugly implementation details
>> belong.
>>
>> When people read actual code, they should be concerning themselves
>> with control flow, what the code is trying to do, etc. rather then
>> being continually interrupted with ifdef this and ifdef that.
> 
> On the other hand, people are interrupted with ctags jumping when suddenly
> whole new file appears, so loss of context is even more. Distance between
> static inline pair tend to increase as people add more stuff in between.
> 
> And how this one line ifdef (in one place -- allocation) can interrupt
> control flow when you see it's start and immediately see it's end? Is it
> because ifdef starts at column one, and code on average is two-three tabs
> indented, so eye jumps to column one?
> 
> Whey you are suspicious of the code (say, look for a bug) these wrappers
> are nuisaince, because some very smart one may do completely unexpected
> thing wrt it's innocent name. And you check them all because you're
> suspicious.
> 
> Netdevices use dev_net_set(). Add ib_net_set() and forget about this hungarian
> pnet thing. const qualifier is also pointless, there maybe nothing wrong with
> it, be there is nothing right as well.
> 
> Well, yes, Linus starts blogging and hungarian notation in core networking. :-)

Take a look at include/net/net_namespace.h 
I made my best to choose a notation and found :

copy_net_ns(), __put_net(), net_alive(), get_net(), maybe_get_net(), 
put_net(), net_eq(), hold_net(), release_net()

What a mess.

I then decided to name my two helpers read_pnet() and write_pnet(), not
because I love hungarion notaion, but to keep existing practice in this
file.

I have no problem to clean the whole file and have a consistent notation.

I have no problem you take care of this.

Thank you

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