From: Joe Perches <j...@perches.com>
Sent: Friday, August 21, 2020 10:39 PM
>
> On Fri, 2020-08-21 at 16:25 +0000, Konstantin Komarov wrote:
> > Initialization of super block for fs/ntfs3
> []
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> []
> > +
> > +/**
> > + * ntfs_trace() - print preformated ntfs specific messages.
> > + */
> > +void __ntfs_trace(const struct super_block *sb, const char *level,
> > + const char *fmt, ...)
>
> This is a printk mechanism.
>
> I suggest renaming this __ntfs_trace function to ntfs_printk
> as there is a naming expectation conflict with the tracing
> subsystem.
>
> > +{
[]
> > + else
> > + printk("%sntfs3: %s: %pV", level, sb->s_id, &vaf);
> > + va_end(args);
> > +}
>
> Also it would be rather smaller overall object code to
> change the macros and uses to embed the KERN_<LEVEL> into
> the format and remove the const char *level argument.
>
> Use printk_get_level to retrieve the level from the format.
>
> see fs/f2fs/super.c for an example.
>
> This could be something like the below with a '\n' addition
> to the format string to ensure that messages are properly
> terminated and cannot be interleaved by other subsystems
> content that might be in another simultaneously running
> thread starting with KERN_CONT.
>
> void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
> {
> struct va_format vaf;
> va_list args;
> int level;
>
> va_start(args, fmt);
>
> level = printk_get_level(fmt);
> vaf.fmt = printk_skip_level(fmt);
> vaf.va = &args;
> if (!sb)
> printk("%c%cntfs3: %pV\n",
> KERN_SOH_ASCII, level, &vaf);
> else
> printk("%c%cntfs3: %s: %pV\n",
> KERN_SOH_ASCII, level, sbi->sb->s_id, &vaf);
>
> va_end(args);
> }
>
> > +
> > +/* prints info about inode using dentry case if */
> > +void __ntfs_inode_trace(struct inode *inode, const char *level, const char
> > *fmt,
>
> ntfs_inode_printk
>
> > + ...)
> > +{
> > + struct super_block *sb = inode->i_sb;
> > + ntfs_sb_info *sbi = sb->s_fs_info;
> > + struct dentry *dentry;
> > + const char *name = "?";
> > + char buf[48];
> > + va_list args;
> > + struct va_format vaf;
> > +
> > + if (!__ratelimit(&sbi->ratelimit))
> > + return;
> > +
> > + dentry = d_find_alias(inode);
> > + if (dentry) {
> > + spin_lock(&dentry->d_lock);
> > + name = (const char *)dentry->d_name.name;
> > + } else {
> > + snprintf(buf, sizeof(buf), "r=%lx", inode->i_ino);
> > + name = buf;
> > + }
> > +
> > + va_start(args, fmt);
> > + vaf.fmt = fmt;
> > + vaf.va = &args;
> > + printk("%s%s on %s: %pV", level, name, sb->s_id, &vaf);
> > + va_end(args);
> > +
> > + if (dentry) {
> > + spin_unlock(&dentry->d_lock);
> > + dput(dentry);
> > + }
> > +}
>
> Remove level and use printk_get_level as above.
> Format string should use '\n' termination here too.
>
Thanks for pointing this out and for your effort with the patch, Joe. We will
rework logging in V3 so that it's more compliant with Kernel's approach.
