On Thu, Dec 02, 2010 at 10:51:32PM -0500, Lachlan McIlroy wrote:
> Dave, overall it looks good - just a few minor points below.
> Thanks for doing this.
>
> ----- "Dave Chinner" <david@xxxxxxxxxxxxx> wrote:
[snip]
> > -void
> > -cmn_err(register int level, char *fmt, ...)
> > -{
> > - char *fp = fmt;
> > - int len;
> > - ulong flags;
> > - va_list ap;
> > -
> > - level &= XFS_ERR_MASK;
> > - if (level > XFS_MAX_ERR_LEVEL)
> > - level = XFS_MAX_ERR_LEVEL;
> > - spin_lock_irqsave(&xfs_err_lock,flags);
> > - va_start(ap, fmt);
> > - if (*fmt == '!') fp++;
> > - len = vsnprintf(message, sizeof(message), fp, ap);
> > - if (len >= sizeof(message))
> > - len = sizeof(message) - 1;
> > - if (message[len-1] == '\n')
> > - message[len-1] = 0;
^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > - printk("%s%s\n", err_level[level], message);
> > - va_end(ap);
> > - spin_unlock_irqrestore(&xfs_err_lock,flags);
> > - BUG_ON(level == CE_PANIC);
> > -}
[snip]
> > +#define CE_DEBUG KERN_DEBUG
> > +#define CE_CONT KERN_INFO
> > +#define CE_NOTE KERN_NOTICE
> > +#define CE_WARN KERN_WARNING
> > +#define CE_ALERT KERN_ALERT
> > +#define CE_PANIC KERN_EMERG
> > +
> > +#define cmn_err(lvl, fmt, args...) \
> > + do { \
> > + printk(lvl fmt, ## args); \
>
> The old cmn_err() routine would append a newline if one was not supplied.
> As far as I know printk() will not do the same so either we need to fix
> all calls to cmn_err() to supply a '\n' or add it here (at the risk of
> having two newlines) - maybe:
See above - I think you have it the wrong way around - it looks to
me like the old cmn_err() stripped the newline character if it
existed, then added one itself.
> printk(lvl fmt "\n", ## args);
printk() is actually pretty tolerant of missing newlines - if it
detects the previous printk() was not newline terminated and the
next one starts with a log level that is not KERN_CONT, it will
print the new message on a new line automatically. This is the code
in vprintk() that handles it:
/* Do we have a loglevel in the string? */
if (p[0] == '<') {
unsigned char c = p[1];
if (c && p[2] == '>') {
switch (c) {
case '0' ... '7': /* loglevel */
current_log_level = c - '0';
/* Fallthrough - make sure we're on a new line */
case 'd': /* KERN_DEFAULT */
if (!new_text_line) {
emit_log_char('\n');
new_text_line = 1;
}
/* Fallthrough - skip the loglevel */
case 'c': /* KERN_CONT */
p += 3;
break;
}
}
}
So we are probably OK not to need additional newlines. Indeed, I
didn't notice the output being screwed up without them. ;)
I can add them if you want, though.
>
> > + BUG_ON(strncmp(lvl, KERN_EMERG, strlen(KERN_EMERG)) == 0); \
> > + } while (0)
> > +
> > +#define xfs_fs_cmn_err(lvl, mp, fmt, args...) \
> > + do { \
> > + printk(lvl "Filesystem %s: " fmt, (mp)->m_fsname, ## args); \
>
> printk(lvl "Filesystem %s: " fmt "\n", (mp)->m_fsname, ## args);
>
> > + BUG_ON(strncmp(lvl, KERN_EMERG, strlen(KERN_EMERG)) == 0); \
> > + } while (0)
> > +
> > +/* All callers to xfs_cmn_err use CE_ALERT, so don't bother testing
> > lvl */
> > +#define xfs_cmn_err(panic_tag, lvl, mp, fmt, args...) \
> > + do { \
> > + int panic = 0; \
> > + if (xfs_panic_mask && (xfs_panic_mask & panic_tag)) { \
> > + printk(KERN_ALERT "XFS: Transforming an alert into a BUG."); \
> > + panic = 1; \
> > + } \
> > + printk(KERN_ALERT "Filesystem %s: " fmt, (mp)->m_fsname, ## args);
> > \
> > + BUG_ON(panic); \
> > + } while (0)
>
> I think we can simplify this case a bit and remove the panic variable,
> like this:
>
> do { \
> printk(KERN_ALERT "Filesystem %s: " fmt "\n", (mp)->m_fsname, ## args);
> if (xfs_panic_mask && (xfs_panic_mask & panic_tag)) { \
> printk(KERN_ALERT "XFS: Transforming an alert into a BUG.\n"); \
> BUG_ON(1); \
> } \
> } while (0)
>
> This also reorders the messages which I think makes more sense.
Definitely. That's a vast improvement. ;)
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
