Re: [PATCH v3 1/2] Simplify XFS logging methods.

[Petr Mladek <pmladek@suse.com>]

On Wed 2022-03-30 10:54:41, Dave Chinner wrote:
> On Fri, Mar 25, 2022 at 10:19:45AM -0700, Jonathan Lassoff wrote:
> > Rather than have a constructor to define many nearly-identical
> > functions, use preprocessor macros to pass down a kernel logging level
> > to a common function.
> > 
> > Signed-off-by: Jonathan Lassoff <jof@thejof.com>
> 
> Mostly looks good, mainly just whitespace/formatting consistency
> issues now.
> 
> ....
> > -define_xfs_printk_level(xfs_emerg, KERN_EMERG);
> > -define_xfs_printk_level(xfs_alert, KERN_ALERT);
> > -define_xfs_printk_level(xfs_crit, KERN_CRIT);
> > -define_xfs_printk_level(xfs_err, KERN_ERR);
> > -define_xfs_printk_level(xfs_warn, KERN_WARNING);
> > -define_xfs_printk_level(xfs_notice, KERN_NOTICE);
> > -define_xfs_printk_level(xfs_info, KERN_INFO);
> > -#ifdef DEBUG
> > -define_xfs_printk_level(xfs_debug, KERN_DEBUG);
> > -#endif
> > +void xfs_printk_level(
> > +	const char *kern_level,
> > +	const struct xfs_mount *mp,
> > +	const char *fmt, ...)
> 
> Use the same format as __xfs_printk() and xfs_alert_tag():
> 
> void
> xfs_printk_level(
> 	const char		*kern_level,
> 	const struct xfs_mount	*mp,
> 	const char		*fmt, ...)
> 
> > +{
> > +	struct va_format	vaf;
> > +	va_list			args;
> > +	int			level;
> > +
> > +	va_start(args, fmt);
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +
> > +	__xfs_printk(kern_level, mp, &vaf);
> > +
> > +	va_end(args);
> > +
> > +	if (!kstrtoint(kern_level, 0, &level) &&
> > +	    level <= LOGLEVEL_ERR &&
> > +	    xfs_error_level >= XFS_ERRLEVEL_HIGH)
> > +		xfs_stack_trace();
> > +}
> >  
> >  void
> >  xfs_alert_tag(
> > diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
> > index bb9860ec9a93..2f609800e806 100644
> > --- a/fs/xfs/xfs_message.h
> > +++ b/fs/xfs/xfs_message.h
> > @@ -6,33 +6,36 @@
> >  
> >  struct xfs_mount;
> >  
> > -extern __printf(2, 3)
> > -void xfs_emerg(const struct xfs_mount *mp, const char *fmt, ...);
> > -extern __printf(2, 3)
> > -void xfs_alert(const struct xfs_mount *mp, const char *fmt, ...);
> >  extern __printf(3, 4)

^^^^^^^

> > -void xfs_alert_tag(const struct xfs_mount *mp, int tag, const char *fmt, ...);
> > -extern __printf(2, 3)
> > -void xfs_crit(const struct xfs_mount *mp, const char *fmt, ...);
> > -extern __printf(2, 3)
> > -void xfs_err(const struct xfs_mount *mp, const char *fmt, ...);
> > -extern __printf(2, 3)
> > -void xfs_warn(const struct xfs_mount *mp, const char *fmt, ...);
> > -extern __printf(2, 3)
> > -void xfs_notice(const struct xfs_mount *mp, const char *fmt, ...);
> > -extern __printf(2, 3)
> > -void xfs_info(const struct xfs_mount *mp, const char *fmt, ...);
> > -
> > +void xfs_printk_level(
> > +	const char *kern_level,
> > +	const struct xfs_mount *mp,
> > +	const char *fmt, ...);
> 
> This still needs the __printf() attribute because we still want the
> compiler to check the printf format string for issues. Also the
> format for function prototypes should follow the ones that got
> removed:

It is actually there. But it is hidden in many removed lines.

BTW: I missed it when reading the patch as well. I was surprised
     when I saw it after applying the patch ;-)

Best Regards,
Petr