Re: [PATCH] checkpatch: Suggest pr_<level> over printk(KERN_<LEVEL>

[Joe Perches]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/