linux-kernel - Re: [PATCH] update checkpatch.pl to version 0.03

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: <46692AB6.1090604@gmail.com>
Date:	Fri, 08 Jun 2007 12:08:54 +0200
From:	Rene Herman <rene.herman@...il.com>
To:	Andy Whitcroft <apw@...dowen.org>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Randy Dunlap <rdunlap@...otime.net>,
	Joel Schopp <jschopp@...tin.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] update checkpatch.pl to version 0.03

On 06/08/2007 11:31 AM, Andy Whitcroft wrote:

> Ok, the latest version 0.04 is released and I have also put up the
> complete script at:
> 
>   http://www.shadowen.org/~apw/public/checkpatch/checkpatch.pl-0.04
> 
> Previous versions are also there.

Thank you. False positive:

	do not use assignment in condition
	#809: FILE: drivers/cdrom/mitsumi.c:766:
	+       while ((req = elv_next_request(q))) {

Doing an assignment in a while (or for) condition like that makes perfect 
sense. The check should probably be limited to if conditions -- you can 
always split those.

Next:

	struct mutex definition without comment
	#173: FILE: drivers/cdrom/mitsumi.c:130:
	+	struct mutex mutex;

Going overboard. It's the only locking primitive in the driver; the only 
comment that could be added is something like "used for mutual exclusion" 
which firmly falls into the "i++; /* increase i */" category. A bit further 
on up in the driver, a:

	/*
	 * LOCKING: mutex_lock(&mcd->mutex)
	 */

is present just before the functions that need to be called with it held 
(all, basically). I'd suggest simply dropping this check as its intention is 
not something that can be usefully automated I feel. The tree would end up 
with tons of useless comments that are there just to shut up the script.

And next a ton of:

	labels should not be indented
	#249: FILE: drivers/cdrom/mitsumi.c:206:
	+  out:

This driver is putting two spaces in front of labels, as explained here:

	http://lkml.org/lkml/2007/6/5/61

I do agree that putting them level aligned is a _significantly_ different 
style, so perhaps it wants to warn if a label is not within the first indent 
level (column 0-7) but if even that's contentious then this check should 
perhaps go completely as well. One or two spaces, four for all I care, can 
be considered a personal preference I feel.

Rene.

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