linux-kernel - Re: [PATCH] Remove errors caught by checkpatch.pl in kernel/kallsyms.c

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <499984C1.6020004@s5r6.in-berlin.de>
Date:	Mon, 16 Feb 2009 16:22:41 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Sam Ravnborg <sam@...nborg.org>,
	Manish Katiyar <mkatiyar@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] Remove errors caught by checkpatch.pl in	kernel/kallsyms.c

Ingo Molnar wrote:
> We routinely mention Sparse, lockdep, Coverity, Coccinelle, kmemleak, 
> ftrace, kmemcheck and other tools as well when it motives to fix a bug 
> or uncleanliness. [...] It is absolutely fine to
> mention checkpatch when it catches uncleanliness in code that already 
> got merged. I dont understand your point.

I wrote "don't mention checkpatch" but I really meant "think about what
the effect of the patch is and describe this".

It's not really a hard problem to mention checkpatch --- it is a problem
to make it the main point or, like in this case, the only point of the
changelog.  (Furthermore, it is also a problem to do something routinely
*if* doing it does not make sense.  There routinely appear coccinelle
metapatch sources in changelogs.  That does not make sense at all, and
doing it routinely is not a justification in itself.)

So, "don't mention checkpatch" is simply a rule of thumb; read it as "I
mentioned checkpatch in the changelog --- wait, I have possibly written
a changelog that is besides the point; I should think about it once
more".  :-)

Now, when this particular patch is updated to get a good changelog, then
the title could become e.g.:
	kernel/kallsyms: change initcall level; adjust whitespace
and anything more than that is just fluff and wasted electrons. Actually
the changelog should rather contain a note on why device_initcall is
supposed to be the correct initcall level.

Fixes due to reports from sparse, lockdep, coverity, coccinelle, etc.
are the in this respect the same as fixes due to reports from
checkpatch:  Patch titles should for example be
  - "fix potential deadlock..."
  - "fix use-after free..."
  - "use XYZ helper..."
  - "adjust whitespace..."
and *not* something like "fix lockdep backtrace" or whatever.

A difference would be a patch title like "add sparse annotations"
because this is indeed about what the patch does, not by which means it
was created.

Why do I make a fuzz?  Well, because many of our changelogs really suck
and we need to become better in general.
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/
--
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/