linux-kernel - Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN

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: <1331730336.27389.70.camel@joe2Laptop>
Date:	Wed, 14 Mar 2012 06:05:36 -0700
From:	Joe Perches <joe@...ches.com>
To:	Ted Ts'o <tytso@....edu>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andy Whitcroft <apw@...dowen.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

On Wed, 2012-03-14 at 08:34 -0400, Ted Ts'o wrote:
> On Tue, Mar 13, 2012 at 08:01:14PM -0700, Joe Perches wrote:
> > > That's a debug message which is never by anyone other than ext4
> > > developers.  Your patch also hacked the Makefile to enable it by
> > > default,
> > 
> > It's just an example and no it didn't.
> > That output is still in an #ifdef EXT4FS_DEBUG
> > block and is unchanged.
> 
> I looked at your patch, and nearly all of them were in debug code.  I
> know, because in practice the messages that come up with any kind of
> regularity are all properly prefixed.

Not really, some are prefixed with EXT4-fs, others EXT4,
some with colons, some without, some with no prefix,
some with function names only.

The idea is to be consistent and allow a mechanical
comprehensive dmesg grep with "EXT4-fs:" or some
other appropriate subsystem name.

$ grep -rP --include=*.[ch] "\bprintk\s*\(\s*KERN_[A-Z]+\s*\"[^\":]*" fs/ext4/

>       http://patchwork.ozlabs.org/project/linux-ext4/list/

Patchwork queues are pretty useless when patches entered
do not have their status updated for long periods.

The patch I sent in August 2011 shows "new" rather than
have an appropriate status.

There are patches in that queue from 2008 marked as "new"
that will never be applied or looked at again.

If you actually use patchwork, though it seems you don't,
I think you should just mark every patch that's new as
rejected and start over.

> Very few other people review patches, and even patches that survive
> review, I've found problems that could potentially lead to data loss
> or system instability.  This is not like your average device driver,
> where if the machine panics once a week, "oh well", and you reboot.
> Linus would get very cranky if he lost data as a result of a bad patch
> slipping through.  Hence, patches don't go in until after significant
> review and testing.
> 
> As a result, #1, patches that are don't add value, and are large,
> simply won't get applied.  Period.  Avoiding the downside of lots of
> people losing data is ****far**** more than your OCD wanting me to use
> pr_warn(...) instead of printk(KERN_WARN, ...).

I believe it's less OCD than you do.

Using a facility to prefix dmesg output consistently
per subsystem adds value in my opinion.

> If you want your style patches to go in, break them into smaller
> chunks, or I *will* ignore them.

OK, I'll resubmit it as micropatches.

cheers, Joe

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