linux-kernel - Re: [announce] new tree: "fix all build warnings, on all configs"

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]

Message-ID: <20081022101013.GA25283@logfs.org>
Date:	Wed, 22 Oct 2008 12:10:13 +0200
From:	Jörn Engel <joern@...fs.org>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Roland Dreier <rdreier@...co.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"David S. Miller" <davem@...emloft.net>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	David Howells <dhowells@...hat.com>
Subject: Re: [announce] new tree: "fix all build warnings, on all configs"

On Wed, 22 October 2008 11:47:59 +0200, Ingo Molnar wrote:
> * Jörn Engel <joern@...fs.org> wrote:
> > On Mon, 20 October 2008 21:21:10 +0200, Ingo Molnar wrote:
> > >  
> > >  /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
> > >  #define fops_get(fops) \
> > > -	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
> > > +	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
> > >  #define fops_put(fops) \
> > > -	do { if (fops) module_put((fops)->owner); } while(0)
> > > +	do { if (fops != NULL) module_put((fops)->owner); } while(0)
> > 
> > This, I would argue, makes the code worse.
> 
> In this specific case the issue is that the 'fops' parameter can 
> occasionally be a constant pointer (turning the test into always-true) 
> so the compiler is at least minimally correct at asking the "are you 
> sure you want this" question - which we answer in the affirmative via 
> the explicit NULL check. But these are really nuances.

#define fops_put(fops) do {			\
	if (fops != NULL)			\
		module_put((fops)->owner);	\
} while 0

If the code was written like this, I wouldn't mind your patch at all.
But in the one-liner form, the ' != NULL' makes the difference between
fluent reading and having to actively sort out which piece belongs
where.

And I bet that if it hadn't been a macro but an inline function, you
and checkpatch would have complained about the formatting long ago. ;)

Jörn

-- 
To announce that there must be no criticism of the President, or that we
are to stand by the President, right or wrong, is not only unpatriotic
and servile, but is morally treasonable to the American public.
-- Theodore Roosevelt, Kansas City Star, 1918
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/