linux-kernel - Re: [PATCH 2/3] x86-64: use relative 32-bit pointers in exception tables

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: <20110218081446.GA21125@elte.hu>
Date:	Fri, 18 Feb 2011 09:14:46 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Jan Beulich <JBeulich@...ell.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Cc:	tony.luck@...el.com, tglx@...utronix.de, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, hpa@...or.com
Subject: Re: [PATCH 2/3] x86-64: use relative 32-bit pointers in exception
 tables


* Jan Beulich <JBeulich@...ell.com> wrote:

> >>> On 17.02.11 at 18:25, Ingo Molnar <mingo@...e.hu> wrote:
> 
> > Nice patch. I've got a really small code readability nitpick:
> > 
> >> +#ifndef ex_insn /* until all architectures have this accessor */
> >> +#define ex_insn(x) (x)->insn
> >> +#endif
> > 
> >> +#else
> >> +#define swap_ex NULL
> >> +#endif
> > 
> > In the x86 architecture we tend to write this as:
> > 
> >> +#else
> >> +# define swap_ex NULL
> >> +#endif
> > 
> > So that the conditional structure stands out more, visually. (There might be 
> > more 
> > such cases in these patches as well.)
> 
> I can certainly fix this, but got a comment from (I think) Andrew
> Morton to do exactly the opposite quite some time ago, with the
> rationale that this indentation leads to more involved patches
> when further conditionals get added around them.

Well, the patch impact argument is a valid concern, but by that logic we should also 
drop the visual structure of other conditionals such as:

	if (x) {
		if (y)
			z;
		else
			k;
	} else {
		l;
	}

and write:

	if (x) {
	if (y)
	z;
	else
	k;
	} else {
	l;
	}

? I don't think so.

There might be other cases where marking CPP code this way looks ugly but in this 
patch it's clearly helpful to readability.

So i think for consistency's (and eyeball health's) sake lets bring as much 
meaningful geometric structure into source code as possible. Future patch size 
worries are secondary IMO.

Thanks,

	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/