linux-kernel - Re: [PATCH -mm] 1/2: MMCONFIG: validate against ACPI motherboard resources

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: <alpine.LFD.0.98.0705292125511.26602@woody.linux-foundation.org>
Date:	Tue, 29 May 2007 21:45:06 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Robert Hancock <hancockr@...w.ca>
cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jesse Barnes <jbarnes@...tuousgeek.org>
Subject: Re: [PATCH -mm] 1/2: MMCONFIG: validate against ACPI motherboard
 resources

On Tue, 29 May 2007, Robert Hancock wrote:
>
> This path adds validation of the MMCONFIG table against the ACPI reserved
> motherboard resources.

Please fix the formatting of your code.

"for" and "if" are not functions, and they have a space before the 
parenthesis.

And pretty much every single conditional in this patch is spread out over 
two or more lines and has at least three different indentations. There's 
something wrong here. Code can't look this bad and still be fine. Some of 
this looks like random whitespace noise:

+               if(is_acpi_reserved(cfg->address,
+                                   cfg->address + size - 1))
+                       printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
+                               "in ACPI motherboard resources\n",
+                               cfg->address);
+               else {

That's just horrid. Please try to make the code _look_ nicer.

For example, just making "is_acpi_reserved()" take a start/len thing 
instead, would allow you to at least do

	if (is_acpi_reserved(cfg->address, size)) {
		printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
			"in ACPI motherboard resources\n",
			cfg->address);
	} else {
		...

(and has the braces rigth too - don't pair an unbraced "if ()" with a 
braced "else" statement), and that together with making the body of the 
for-loop a separate function would possibly make that code read a lot 
better.

Same goes for this thing:

+                       if((pci_probe & PCI_PROBE_CONF1) &&
+                          e820_all_mapped(cfg->address,
+                                          cfg->address + size - 1,
+                                          E820_RESERVED))
+                               printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved in E820\n",
+                                       cfg->address);
+                       else
+                               goto reject;

there really is *not* a highly coveted prize for having the most different 
indentation in the fewest possible lines of code!

Yeah, I realize that maybe this is nit-picking, but trying to read this 
patch really does make you go blind. It violates so many coding standards 
that it's almost impossible to read the code itself. It's made worse by 
the fact that you then also used Thunderbird to send the patch, and had it 
set for

	Content-type: text/plain; charset=ISO-8859-1; format=flowed

where that "format=flowed" means that basically no mail-client will be 
able to read it sanely (because a lot of them will re-flow the text), and 
you have to save it to a file before you can even comment on it.

Gaah. See

	http://mbligh.org/linuxdocs/Email/Clients/Thunderbird

where the most important sentence is "Thunderbird is written by aged whore 
monkeys stoned on crack". But it also talks about how to disable that 
idiotic format=flowed for patches, and how to make sure it's not wrapping.

But as mentioned, your patch itself had some whitespace issues even aside 
from the regular Thunderbird breakage.

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