linux-ext4 - Re: [PATCH] Unify log messages in ext2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 17 Nov 2009 22:23:14 +0100
From:	Jan Kara <jack@...e.cz>
To:	Alexey Fisher <bug-track@...her-privat.net>
Cc:	Eric Sandeen <sandeen@...hat.com>,
	Andreas Dilger <adilger@....com>, linux-ext4@...r.kernel.org,
	Theodore Tso <tytso@....edu>
Subject: Re: [PATCH] Unify log messages in ext2

  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@...her-privat.net>
> > Reviewed-by: Andreas Dilger <adilger@....com>
> > ---
> >  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.

> > @@ -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
-- 
Jan Kara <jack@...e.cz>
SuSE CR Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html