Re: [PATCH 1/1] NTFS: Logging clean-up

[Joe Perches <joe@perches.com>]

On Sun, 2014-03-16 at 15:53 +0000, Anton Altaparmakov wrote:
> Hi,
> 
> Looks good, thanks.  You can add my Acked-by if you like.  Can I assume you have test it builds?
> 
> Andrew, can you please merge this via your patch series?  Thanks!

I think a few things need fixing first

> On 16 Mar 2014, at 12:27, Fabian Frederick <fabf@skynet.be> wrote:
> 
> > -All printk(KERN_foo converted to pr_foo().
> > -Add pr_fmt and remove redundant prefixes.

I'm not sure there are "redundant prefixes"

This changes prefixes from "NTFS-fs" to "ntfs"
and should be at a minimum mentioned in the
changelog.

The va_end location needs moving.

Converting printk(KERN_DEBUG -> pr_debug will
not always emit that message now.  Now, only if
DEBUG is #defined or dynamic_debugging is enabled
for the build _and_ the message is specifically
enabled will the message be output.

So, the debugging output has been silenced by default.

That's not necessarily good.

> > diff --git a/fs/ntfs/debug.c b/fs/ntfs/debug.c
[]
> > @@ -59,17 +53,15 @@ void __ntfs_warning(const char *function, const struct super_block *sb,
> > #endif
> > 	if (function)
> > 		flen = strlen(function);
> > -	spin_lock(&err_buf_lock);
> > 	va_start(args, fmt);
> > -	vsnprintf(err_buf, sizeof(err_buf), fmt, args);
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;

> > 	va_end(args);

This va_end should be moved after the pr_warns.

> > 	if (sb)
> > -		printk(KERN_ERR "NTFS-fs warning (device %s): %s(): %s\n",
> > -				sb->s_id, flen ? function : "", err_buf);
> > +		pr_warn("(device %s): %s(): %pV\n",
> > +			sb->s_id, flen ? function : "", &vaf);
> > 	else
> > -		printk(KERN_ERR "NTFS-fs warning: %s(): %s\n",
> > -				flen ? function : "", err_buf);
> > -	spin_unlock(&err_buf_lock);
> > +		pr_warn("%s(): %pV\n", flen ? function : "", &vaf);
> > }
> > 
> > /**
> > @@ -94,6 +86,7 @@ void __ntfs_warning(const char *function, const struct super_block *sb,
> > void __ntfs_error(const char *function, const struct super_block *sb,
> > 		const char *fmt, ...)
> > {
> > +	struct va_format vaf;
> > 	va_list args;
> > 	int flen = 0;
> > 
> > @@ -103,17 +96,15 @@ void __ntfs_error(const char *function, const struct super_block *sb,
> > #endif
> > 	if (function)
> > 		flen = strlen(function);
> > -	spin_lock(&err_buf_lock);
> > 	va_start(args, fmt);
> > -	vsnprintf(err_buf, sizeof(err_buf), fmt, args);
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > 	va_end(args);

Here too

> > 	if (sb)
> > -		printk(KERN_ERR "NTFS-fs error (device %s): %s(): %s\n",
> > -				sb->s_id, flen ? function : "", err_buf);
> > +		pr_err("(device %s): %s(): %pV\n",
> > +		       sb->s_id, flen ? function : "", &vaf);
> > 	else
> > -		printk(KERN_ERR "NTFS-fs error: %s(): %s\n",
> > -				flen ? function : "", err_buf);
> > -	spin_unlock(&err_buf_lock);
> > +		pr_err("%s(): %pV\n", flen ? function : "", &vaf);