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 13:44:39 +0300
From:	Alexey Dobriyan <adobriyan@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	dada1@...mosbay.com, netdev@...r.kernel.org
Subject: Re: [PATCH v2] net: #ifdef inet_bind_bucket::ib_net

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