linux-kernel - Re: [PATCH 2/9] ext4: Use pr_fmt and pr

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: <20120320014707.GD14363@thunk.org>
Date:	Mon, 19 Mar 2012 21:47:07 -0400
From:	Ted Ts'o <tytso@....edu>
To:	Joe Perches <joe@...ches.com>
Cc:	David Miller <davem@...emloft.net>, anca.emanuel@...il.com,
	adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/9] ext4: Use pr_fmt and pr_<level>

On Mon, Mar 19, 2012 at 06:33:22PM -0700, Joe Perches wrote:
> They _were_ doubly prefixed.
> 
> from ext4#dev
> 
> commit 2504a4a9c0c096e11bcc24691b85bf6d942df9fe
> Author: Joe Perches <joe@...ches.com>
> Date:   Mon Mar 19 00:12:00 2012 -0400
> 
>     ext4: remove redundant "EXT4-fs: " from uses of ext4_msg
>     
>     ext4_msg adds "EXT4-fs: " to the messsage output.
>     Remove the redundant bits from uses.
>     
>     Signed-off-by: Joe Perches <joe@...ches.com>
>     Signed-off-by: "Theodore Ts'o" <tytso@....edu>

Yes, and I accepted that patch.  I was referring to your complaints of
printk's such as this:

#ifdef EXT4FS_DEBUG
			WARN_ON(ret <= 0);
			printk(KERN_ERR "%s: ext4_ext_map_blocks "
				    "returned error inode#%lu, block=%u, "
				    "max_blocks=%u", __func__,
				    inode->i_ino, map.m_lblk, max_blocks);
#endif

... and in that case, even if I was going to fix it, I wouldn't be
fixing it via pr_err().  I'd be fixing using ext4_msg().  Why?
Because then the message would include the block device and not just
the EXT4-fs prefix.

> > Yes, but we can't do structured notifications with the current
> > pr_<foo>.  So why change literally tens of thousands of callsites when
> > in order to really realize the full promise of structured
> > notifications, we'll have to change them *again*?
> 
> So that they are consistent and extensible and can use
> something like pr_<level>_notify, just like pr_<level>_once
> and pr_<level>_ratelimited, or some other similar form.

Changing to pr_err() is pointless, because it doesn't do anything
functional.  You *have* to have an interface like ext4_msg(sb, ...) if
you're going to send a semi-structured notification, or include
relevant information about which ext4 file system was responsible for
issuing the warning.

If you're going to change huge numbers of lines of code, you might as
well do it in a way that significantly improves things.  The change to
pr_foo() is just syntactic sugar, and that's a whitespace-level change
in my book.  Adding a struct super * or or a struct block device *,
which gets passed to the notification functions?  That's ***far***
more interesting.

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