linux-kernel - Re: [PATCH 07/16] GFS2: Directory handling

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: <20060905111926.GB8195@elte.hu>
Date:	Tue, 5 Sep 2006 13:19:26 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Jan Engelhardt <jengelh@...ux01.gwdg.de>
Cc:	Steven Whitehouse <swhiteho@...hat.com>,
	linux-kernel@...r.kernel.org,
	Russell Cattelan <cattelan@...hat.com>,
	David Teigland <teigland@...hat.com>, hch@...radead.org
Subject: Re: [PATCH 07/16] GFS2: Directory handling


* Jan Engelhardt <jengelh@...ux01.gwdg.de> wrote:

> >> > >+	if ((char *)cur + cur_rec_len >= bh_end) {
> >> > >+		if ((char *)cur + cur_rec_len > bh_end) {
> >> > >+			gfs2_consist_inode(dip);
> >> > >+			return -EIO;
> >> > >+		}
> >> > >+		return -ENOENT;
> >> > >+	}
> >> > 
> >> > if((char *)cur + cur_rec_len > bh_end) {
> >> > 	gfs2_consist_inode(dip);
> >> > 	return -EIO;
> >> > } else if((char *)cur + cur_rec_len == bh_end)
> >> > 	return -ENOENT;
> >> > 
> >> ok
> >
> >this one is not OK! Firstly, Jan, and i mentioned this before, please 
> >stop using 'if(', it is highly inconsistent and against basic taste. We 
> >only use this construct for function calls (and macros), not for C 
> >statements.
> 
> Now there is no rule in CodingStyle for this yet. [...]

as i told you before, it is basic taste. Show me the CodingStyle rules 
that forbids this construct:

	if((((i+-+-+-+-+1-1))))
		var=0;;;;;;;

instead of:

	if (i)
		var = 0;

?

You will find no explicit rule in CodingStyle that explicitly forbids 
any aspect of this line, because CodingStyle mainly concentrates on the 
harder to follow rules. So please use common sense in combination with 
CodingStyle.

(And even if you dont agree with the taste, "if ( )" is done by 98% of 
core code, so for the sake of consistency it's the thing to follow.)

> 11:17 gwdg-wb04A:~/linux > grep -Pri '(if|for|while)\(' . | wc -l
> 24242
> 
> [To be honest, I also present the other number:]
> 11:17 gwdg-wb04A:~/linux > grep -Pri '(if|for|while) \(' . | wc -l
> 380895
> 
> Although a minority, it does not seem so uncommon.

What is your point?

ugly code is not at all uncommon, sadly - but i'm glad that it's only 6% 
in this particular case (and 1.9% for core kernel code).

> [...] Plus, I was wanting to show how to reorder the construct, so 
> change in whitespace between "if" and "(" is outoftopic.

it is not offtopic, because you _are_ nitpicking over style issues 
(which is OK and desirable, if not overdone), while still seeming to 
believe that "if(" is fine :-) The one who judges others is judged by 
higher standards. In other words, i'm trying to fix the guy who is 
fixing others ;-)

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