linux-kernel - Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass compiling

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: <20130826221240.GA12075@roeck-us.net>
Date:	Mon, 26 Aug 2013 15:12:40 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Chen Gang <gang.chen@...anux.com>
Cc:	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Yoshinori Sato <ysato@...rs.sourceforge.jp>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] h8300/kernel/setup.c: add "linux/initrd.h" to pass
 compiling

On Mon, Aug 26, 2013 at 07:19:38PM +0800, Chen Gang wrote:
> On 08/26/2013 07:08 PM, Geert Uytterhoeven wrote:
> > On Mon, Aug 26, 2013 at 1:06 PM, Chen Gang <gang.chen@...anux.com> wrote:
> >> On 08/26/2013 07:00 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Aug 26, 2013 at 12:31 PM, Chen Gang <gang.chen@...anux.com> wrote:
> >>>> --- a/arch/h8300/kernel/setup.c
> >>>> +++ b/arch/h8300/kernel/setup.c
> >>>> @@ -47,6 +47,9 @@
> >>>>  #include <asm/regs267x.h>
> >>>>  #endif
> >>>>
> >>>> +#if defined(CONFIG_BLK_DEV_INITRD)
> >>>
> >>> Why have you added the #ifdef?
> >>>
> >>
> >> The related code is below (maybe we need add additional related
> >> comments in the patch for it ?).
> >>
> >> in arch/h8300/kernel/setup.c
> >>
> >>  94 void __init setup_arch(char **cmdline_p)
> >>  95 {
> >>  96         int bootmap_size;
> >>  97
> >>  98         memory_start = (unsigned long) &_ramstart;
> >>  99
> >> 100         /* allow for ROMFS on the end of the kernel */
> >> 101         if (memcmp((void *)memory_start, "-rom1fs-", 8) == 0) {
> >> 102 #if defined(CONFIG_BLK_DEV_INITRD)
> >> 103                 initrd_start = memory_start;
> >> 104                 initrd_end = memory_start += be32_to_cpu(((unsigned long *) (memory_start))[2]);
> >> 105 #else
> >> 106                 memory_start += be32_to_cpu(((unsigned long *) memory_start)[2]);
> >> 107 #endif
> >> 108         }
> > 
> > Sure, it's used conditionally. But it doesn't harm to always include it.
> > That means less #ifdefs in the code.
> > 
> 
> Hmm... I feel, add "#ifdefs" can make the code more clearer (consistent
> with the "#ifdefs" 'for initrd_start' and 'end').
> 
The goal in the Linux kernel is to reduce the amount of ifdefs in the code
by moving conditional code as much as possible into header files. That actually
has a practical advantage - it ensures that all code is compiled.

You may agree or disagree with this approach, but you should follow it and not
add new ifdefs when you write kernel code, much less unnecessary ones.
If anything, you might try to remove existing ifdefs while you are at it.

> For C code readers, more code doesn't mean more complex, if it can make
> things clearer after add some more lines (and be sure of no negative
> effect with performance), normally I prefer to add some more lines.
> 
The Linux kernel community tends to think otherwise. For my part I don't
see how ifdefs, and much more so unecessary ones, make anything clearer.
I would argue you'll end up not seeing the forest because of all the trees.

> And this file has already had an area for all "#ifdefs include", I just
> add it in this area, so at least, it can mach this file well, is it ?
> 
Your argument is along the line of "the code is bad anyway, so it is ok
to make it worse". Not really a good argument if you want to get your code
accepted.

I would suggest to read Documentation/SubmittingPatches, section 2.2,
before you continue with your line of argument.

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