On Sun, Dec 12, 2010 at 08:43:11PM -0500, Lachlan McIlroy wrote:
>
> ----- "Dave Chinner" <david@xxxxxxxxxxxxx> wrote:
>
> > On Fri, Dec 10, 2010 at 08:29:35AM -0500, Christoph Hellwig wrote:
> > > On Fri, Dec 03, 2010 at 03:38:46PM +1100, Dave Chinner wrote:
> > > > FWIW, while these macros are the best way to make a simple
> > backport
> > > > is possible, I just discovered that mainline has a %pV format
> > > > operator that allows an implementation like:
> > > >
> > > > void
> > > > xfs_fs_cmn_err(
> > > > const char *lvl,
> > > > struct xfs_mount *mp,
> > > > const char *fmt,
> > > > ...)
> > > > {
> > > > struct va_format vaf;
> > > > va_list args;
> > > >
> > > > va_start(args, fmt);
> > > > vaf.fmt = fmt;
> > > > vaf.va = &args;
> > > >
> > > > printk("%sFilesystem %s: %pV", lvl, mp->m_fsname, &vaf);
> > > > va_end(args);
> > > >
> > > > BUG_ON(strncmp(lvl, KERN_EMERG, strlen(KERN_EMERG)) == 0);
> > > > }
> > >
> > > With this we can also keep the existing integer-based CE_ values
> > > and do trivial array lookup. That also avoids having to do a strcmp
> > for
> > > every message printed.
> >
> > Very true.
> >
> > Ok, so how do we want to process for this? I'm happy to drop the
> > macro-ised patch and only use it as a backportable fix for old
> > kernels - mainline appears to have a lot more functionality in this
> > area than even recent distro kernels. Lachlan - is that an
> > acceptable approach for you?
>
> Yes, that works for me. We don't have much choice for RHEL4/5 and
> there's no reason that should compromise a better solution upstream.
Ok, I'll get that ball moving...
> > It seems to me that the above is a much better way to start cleaning
> > up the mainline code base, in the following way:
> >
> > 1. the above patch to remove the local message buffer +
> > lock.
> > 2. rationalise all the differences in error reporting down
> > to a common interface
> > 3. convert the common interface to use kernel log levels
> > directly.
> >
> > The current trend seems to be to move towards logging interfaces
> > with the following template:
> >
> > {sub_sys}_pr_{level}(priv, fmt, ...)
> >
> > Which in this case would give us:
> >
> > xfs_pr_debug(struct xfs_mount *mp, char *fmt, ...);
> > xfs_pr_note(struct xfs_mount *mp, char *fmt, ...);
> > ....
> > xfs_pr_alert(struct xfs_mount *mp, char *fmt, ...);
> > xfs_pr_emergency(struct xfs_mount *mp, char *fmt, ...);
> >
> > And a variant for the panic mask tagged version of xfs_pr_alert()
> > of:
> >
> > xfs_pr_alert_tag(struct xfs_mount *mp, int tag, char *fmt, ...);
> >
> > I can't see any particular reason for needing to keep the separate
> > parameter for the log level, nor for keeping all the different
> > logging variants we currently have.
>
> Wont you need to reintroduce the log level parameter when each of these
> new functions calls the common interface?
Ah, what I meant was that the above xfs_pr_*() functions form the
common logging interface, instead of cmn_err, xfs_cmn_err,
xfs_fs_cmn_err, etc. So the second step above would be to convert
all the existing logging calls to use this, but still using the
current cmn_err() back end for the custom XFS log messages. i.e. the
callers change API, but they still use the same CE_* levels.
> ie most of the above functions
> will just be wrappers that convert the function name to a log level.
The third step is to change the implementation to something like
the dev_<level>() printk implementation, or possibly even using the
pr_<level> macros directly via creative abuse of the pr_fmt() macro.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
