Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
To: Joe Perches <joe@perches.com>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Cc: "Pali Rohár" <pali@kernel.org>
Subject: RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block
Date: Thu, 27 Aug 2020 16:14:21 +0000	[thread overview]
Message-ID: <83acd2652133437c8d9f62fcc37ad5e4@paragon-software.com> (raw)
In-Reply-To: <1ad67130d11ae089fbc46fd373e1e019e1de06f8.camel@perches.com>

From: Joe Perches <joe@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.


  reply	other threads:[~2020-08-27 16:14 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <caddbe41eaef4622aab8bac24934eed1@paragon-software.com>
2020-08-21 17:35 ` Randy Dunlap
2020-08-27 16:04   ` Konstantin Komarov
2020-08-21 19:39 ` Joe Perches
2020-08-27 16:14   ` Konstantin Komarov [this message]
2020-08-23  9:55 ` Pali Rohár
2020-08-27 16:20   ` Konstantin Komarov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83acd2652133437c8d9f62fcc37ad5e4@paragon-software.com \
    --to=almaz.alexandrovich@paragon-software.com \
    --cc=joe@perches.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pali@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).