Hi,
Am Dienstag, den 17.11.2009, 22:23 +0100 schrieb Jan Kara:
> Hi,
>
> overall I like the patch. A few comments are below. When you fix them,
> I can take care of merging the patch (please CC me on the posting so
> that I don't miss it in the mailing list).
>
> > Am Donnerstag, den 05.11.2009, 08:33 +0100 schrieb Alexey Fisher:
> > > make messages produced by ext2 more unified. It should be
> > > easy to parse.
> > >
> > > dmesg before patch:
> > > [ 4893.684892] reservations ON
> > > [ 4893.684896] xip option not supported
> > > [ 4893.684961] EXT2-fs warning: mounting ext3 filesystem as ext2
> > > [ 4893.684964] EXT2-fs warning: maximal mount count reached, running
> > > e2fsck is recommended
> > > [ 4893.684990] EXT II FS: 0.5b, 95/08/09, bs=1024, fs=1024, gc=2,
> > > bpg=8192, ipg=1280, mo=80010]
> > >
> > > dmesg after patch:
> > > [ 4893.684892] EXT2-fs (loop0): reservations ON
> > > [ 4893.684896] EXT2-fs (loop0): xip option not supported
> > > [ 4893.684961] EXT2-fs (loop0): warning: mounting ext3 filesystem as
> > > ext2
> > > [ 4893.684964] EXT2-fs (loop0): warning: maximal mount count reached,
> > > running e2fsck is recommended
> > > [ 4893.684990] EXT2-fs (loop0): 0.5b, 95/08/09, bs=1024, fs=1024, gc=2,
> > > bpg=8192, ipg=1280, mo=80010]
> > >
> > > Signed-off-by: Alexey Fisher <bug-track@xxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Andreas Dilger <adilger@xxxxxxx>
> > > ---
> > > fs/ext2/ext2.h | 2 +
> > > fs/ext2/super.c | 151 +++++++++++++++++++++++++++++++------------------------
> > > 2 files changed, 88 insertions(+), 65 deletions(-)
> > >
> > > diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> > > index 9a8a8e2..29f65c5 100644
> > > --- a/fs/ext2/ext2.h
> > > +++ b/fs/ext2/ext2.h
> > > @@ -142,6 +142,8 @@ struct dentry *ext2_get_parent(struct dentry *child);
> > > /* super.c */
> > > extern void ext2_error (struct super_block *, const char *, const char *, ...)
> > > __attribute__ ((format (printf, 3, 4)));
> > > +extern void ext2_msg(struct super_block *, const char *, const char *, ...)
> > > + __attribute__ ((format (printf, 3, 4)));
> > > extern void ext2_warning (struct super_block *, const char *, const char *, ...)
> > > __attribute__ ((format (printf, 3, 4)));
> > > extern void ext2_update_dynamic_rev (struct super_block *sb);
> > > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> > > index 1a9ffee..105fcd1 100644
> > > --- a/fs/ext2/super.c
> > > +++ b/fs/ext2/super.c
> > > @@ -58,26 +58,38 @@ void ext2_error (struct super_block * sb, const char * function,
> > > }
> > >
> > > va_start(args, fmt);
> > > - printk(KERN_CRIT "EXT2-fs error (device %s): %s: ",sb->s_id, function);
> > > + ext2_msg(sb, KERN_CRIT, "error %s", function);
> So this will be:
> EXT2-fs (sda6): error function_foo
> error message
> Not quite what we want I think. Besides that I'd like a bit more
> messages like:
> EXT2-fs error (device): function: message
>
> And for other warnings:
> EXT2-fs warning (device): function: message
>
> It's still easy to parse by grep and IMHO nicer to read.
Hmm... there is currently 3 functions to inform user: ext2_error,
ext2_msg and ext2_warning
ext2_error used only on critical errors, so it will remount ro or panic
ext2_warning will print way you like "EXT2-fs warning (device):
function: message"
ext2_msg is sort all around, you can warn or inform but this do not
include __function__ and this will looks like: "EXT2-fs warning
(device): warning: message"
IMHO not all warnings need function to be printed, for example:
"warning: mounting unchecked fs, running e2fsck is recommended" so if i
use ext2_msg this looks like "EXT2-fs (sda6): warning: mounting
unchecked fs, running e2fsck is recommended"
So the question is: do we actually need print __function__ in warnings?
If not, we do not need ext2_warning but this will not look the way you
like ;)
instead of
EXT2-fs error (device): function: message
EXT2-fs warning (device): function: message
will be:
EXT2-fs (device): error: message
EXT2-fs (device): warning: message
EXT2-fs (device): message
If we do ext2_warning we can make all messages consequent. Some of them
will use ext2_warning
EXT2-fs warning (device): function: message
and some ext2_msg
EXT2-fs (device): warning: message
> > > @@ -420,7 +432,7 @@ static const match_table_t tokens = {
> > > };
> > >
> > > static int parse_options (char * options,
> > > - struct ext2_sb_info *sbi)
> > > + struct ext2_sb_info *sbi, struct super_block *sb)
> > > {
> > > char * p;
> > > substring_t args[MAX_OPT_ARGS];
> When you pass in 'sb', there's no need to pass in 'sbi' since than can
> be obtained as EXT2_SB(sb).
>
> > > cantfind_ext2:
> > > if (!silent)
> > > - printk("VFS: Can't find an ext2 filesystem on dev %s.\n",
> > > - sb->s_id);
> > > + ext2_msg(sb, KERN_ERR,
> > > + "VFS: Can't find an ext2 filesystem");
> > > goto failed_mount;
> This message will be strange:
> EXT2-fs (device): VFS: Can't find an ext2 filesystem
>
> I'd maybe drop the "VFS:" and leave "on dev ..." in the message. I
> know the device name will be duplicated but without that the sentence
> just doesn't quite make sence.
>
> Honza
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
