linux-kernel - Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h

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:	Fri, 7 Feb 2014 17:02:02 -0800 (PST)
From:	David Rientjes <rientjes@...gle.com>
To:	Josh Triplett <josh@...htriplett.org>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Rashika Kheria <rashika.kheria@...il.com>,
	linux-kernel@...r.kernel.org, Rik van Riel <riel@...hat.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Jiang Liu <jiang.liu@...wei.com>,
	Michel Lespinasse <walken@...gle.com>, linux-mm@...ck.org
Subject: Re: [PATCH 9/9] mm: Remove ifdef condition in include/linux/mm.h

On Fri, 7 Feb 2014, Josh Triplett wrote:

> > Why??  If CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID then, yes, we need it to be 
> > global.  Otherwise it's perfectly fine just being static in file scope.  
> > This causes the compilation unit to break when you compile it, not wait 
> > until vmlinux and find undefined references.
> > 
> > I see no reason it can't be done like this in mm/page_alloc.c:
> > 
> > 	#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > 	extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> 
> No, a .c file should not have an extern declaration in it.  This should
> live in an appropriate header file, to be included in both page_alloc.c
> and any arch file that defines an overriding function.
> 

Ok, so you have religious beliefs about extern being used in files ending 
in .c and don't mind the 2900 occurrences of it in the kernel tree and 
desire 14 line obfuscation in header files with comments to what is being 
defined in .c files such as "please see mm/page_alloc.c" as mm.h has.  
Good point.

> > Both of these options look much better than
> > 
> > 	include/linux/mm.h:
> > 
> > 	#if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
> > 	    !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
> > 	static inline int __early_pfn_to_nid(unsigned long pfn)
> > 	{
> > 	        return 0;
> > 	}
> > 	#else
> > 	/* please see mm/page_alloc.c */
> > 	extern int __meminit early_pfn_to_nid(unsigned long pfn);
> > 	#ifdef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > 	/* there is a per-arch backend function. */
> > 	extern int __meminit __early_pfn_to_nid(unsigned long pfn);
> > 	#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > 	#endif
> > 
> > where all this confusion is originating from.
> 
> The proposal is to first simplify those ifdefs by eliminating the inner
> one in the #else; I agree with Andrew that we ought to go ahead and take
> that step given the patch at hand, and then figure out if there's an
> additional simplification possible.
> 

If additional simplification is possible?  Yeah, it's __weak which is 
designed for this purpose.
--
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/