Re: [PATCH] make cramfs look less hostile

Re: [PATCH] make cramfs look less hostile

[Pascal Schmidt]

On Sun, 15 Jun 2003 20:30:11 +0200, you wrote in linux.kernel:

> The only places where this should happen is mounting the rootfs.
> mount(8) has it's own filesystem type detection code and doesn't
> call mount(2) unless it found a matching filesystem type.

Not everybody uses mount(8) from util-linux... I don't think the
more simplicistic versions in embedded systems will even want to
do their own type detection. ;)

-- 
Ciao,
Pascal

Re: [PATCH] make cramfs look less hostile

[Christoph Hellwig]

On Sun, Jun 15, 2003 at 11:20:19PM +0200, Pascal Schmidt wrote:
> On Sun, 15 Jun 2003 20:30:11 +0200, you wrote in linux.kernel:
> 
> > The only places where this should happen is mounting the rootfs.
> > mount(8) has it's own filesystem type detection code and doesn't
> > call mount(2) unless it found a matching filesystem type.
> 
> Not everybody uses mount(8) from util-linux... I don't think the
> more simplicistic versions in embedded systems will even want to
> do their own type detection. ;)

These mount versions should at least pass down MS_VERBOSE then..

[PATCH] make cramfs look less hostile

[Jörn Engel]

Hi!

This thing has been biting me now and again.  "cramfs: wrong magic\n"
looks like an error condition to most people and thus creates bug
reports.  But there is no bug per se in having cramfs support in the
kernel and booting from a jffs2 rootfs.  So instead of teaching the
users over and over, how about this little one-liner?

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.5.71/fs/cramfs/inode.c~cramfs_message	2003-06-05 17:47:36.000000000 +0200
+++ linux-2.5.71/fs/cramfs/inode.c	2003-06-15 17:58:03.000000000 +0200
@@ -218,7 +218,7 @@
 		/* check at 512 byte offset */
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		if (super.magic != CRAMFS_MAGIC) {
-			printk(KERN_ERR "cramfs: wrong magic\n");
+			printk(KERN_INFO "cramfs: magic not found\n");
 			goto out;
 		}
 	}

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 12:46:26 -0400, Mark Hahn wrote:
> 
> > This thing has been biting me now and again.  "cramfs: wrong magic\n"
> > looks like an error condition to most people and thus creates bug
> > reports.  But there is no bug per se in having cramfs support in the
> > kernel and booting from a jffs2 rootfs.  So instead of teaching the
> > users over and over, how about this little one-liner?
> 
> good argument.  but I was expecting you to remove the message entirely,
> or else make it *really* explanatory like "OK, not cramfs then"

Well, some embedded people trying to get this shiny new hardware with
bad debugging interfaces up and running might still like that printk.
But it would make sense to wrap it behind CONFIG_CRAMFS_DEBUG or
#define DEBUG 1 inside inode.c

Comments?

Jörn

-- 
Optimizations always bust things, because all optimizations are, in
the long haul, a form of cheating, and cheaters eventually get caught.
-- Larry Wall 

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 11:57:10 -0500, Brian Jackson wrote:
> 
> What about making it "cramfs: magic not found or incorrect\n". That test could 
> result in incorrect magic (not just missing magic), couldn't it?

Yes, but truth is not my goal. :)

The only point of that output is to give someone an idea what might be
wrong when the kernel boots, but it never reaches userspace.  Keep it
short, crisp, and harmless to those eyes not needing it.  Will try to
code up something longer.

Jörn

-- 
Public Domain  - Free as in Beer
General Public - Free as in Speech
BSD License    - Free as in Enterprise
Shared Source  - Free as in "Work will make you..."

Re: [PATCH] make cramfs look less hostile

[Christoph Hellwig]

On Sun, Jun 15, 2003 at 06:05:24PM +0200, Jörn Engel wrote:
> Hi!
> 
> This thing has been biting me now and again.  "cramfs: wrong magic\n"
> looks like an error condition to most people and thus creates bug
> reports.  But there is no bug per se in having cramfs support in the
> kernel and booting from a jffs2 rootfs.  So instead of teaching the
> users over and over, how about this little one-liner?

Umm, cramfs_fill_super has a silent parameter that's true for
probing the root filesystem.  I'd suggest disabling the printk
completly if it's set.

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 18:26:42 +0100, Christoph Hellwig wrote:
> On Sun, Jun 15, 2003 at 06:05:24PM +0200, Jörn Engel wrote:
> > 
> > This thing has been biting me now and again.  "cramfs: wrong magic\n"
> > looks like an error condition to most people and thus creates bug
> > reports.  But there is no bug per se in having cramfs support in the
> > kernel and booting from a jffs2 rootfs.  So instead of teaching the
> > users over and over, how about this little one-liner?
> 
> Umm, cramfs_fill_super has a silent parameter that's true for
> probing the root filesystem.  I'd suggest disabling the printk
> completly if it's set.

Good idea, but only at first glance.  cramfs_fill_super() always gets
called with silent=1.  So if "(!silent) printk(...);" is functionally
equivalent to ";".

Jörn

-- 
The competent programmer is fully aware of the strictly limited size of
his own skull; therefore he approaches the programming task in full
humility, and among other things he avoids clever tricks like the plague. 
-- Edsger W. Dijkstra

Re: [PATCH] make cramfs look less hostile

[Christoph Hellwig]

On Sun, Jun 15, 2003 at 07:39:26PM +0200, Jörn Engel wrote:
> > Umm, cramfs_fill_super has a silent parameter that's true for
> > probing the root filesystem.  I'd suggest disabling the printk
> > completly if it's set.
> 
> Good idea, but only at first glance.  cramfs_fill_super() always gets
> called with silent=1.  So if "(!silent) printk(...);" is functionally
> equivalent to ";".

It's not.  The rootfs is mounted with the (grossly misnamed) MS_VERBOSE
flag which translates to the last argument of the fill_super callback
set to 1.

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 18:44:17 +0100, Christoph Hellwig wrote:
> On Sun, Jun 15, 2003 at 07:39:26PM +0200, Jörn Engel wrote:
> > > Umm, cramfs_fill_super has a silent parameter that's true for
> > > probing the root filesystem.  I'd suggest disabling the printk
> > > completly if it's set.
> > 
> > Good idea, but only at first glance.  cramfs_fill_super() always gets
> > called with silent=1.  So if "(!silent) printk(...);" is functionally
> > equivalent to ";".
> 
> It's not.  The rootfs is mounted with the (grossly misnamed) MS_VERBOSE
> flag which translates to the last argument of the fill_super callback
> set to 1.

Ok, you win this one.  Leaves this case:
$ mount /dev/somewhere /mnt
cramfs: wrong magic
$ echo $?
0
$

Here I consider the message evil as well.  The patch below on the
other hand might go a little too far.  All the other printk()s are
rare and might hold some useful information for normal users as well.

Any arguments for or against this patch?

Jörn

-- 
To recognize individual spam features you have to try to get into the
mind of the spammer, and frankly I want to spend as little time inside
the minds of spammers as possible.
-- Paul Graham

--- linux-2.5.71/fs/cramfs/inode.c~cramfs_message	2003-06-05 17:47:36.000000000 +0200
+++ linux-2.5.71/fs/cramfs/inode.c	2003-06-15 19:39:03.000000000 +0200
@@ -27,6 +27,14 @@
 
 #include <asm/uaccess.h>
 
+#define DEBUG 0	/* set to 1 for verbose messages during system bringup */
+
+#if DEBUG
+# define dprintk(fmt,args...) printk(fmt , ##args)
+#else
+# define dprintk(fmt,args...)
+#endif
+
 static struct super_operations cramfs_ops;
 static struct inode_operations cramfs_dir_inode_operations;
 static struct file_operations cramfs_directory_operations;
@@ -218,20 +226,20 @@
 		/* check at 512 byte offset */
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		if (super.magic != CRAMFS_MAGIC) {
-			printk(KERN_ERR "cramfs: wrong magic\n");
+			dprintk(KERN_INFO "cramfs: magic not found\n");
 			goto out;
 		}
 	}
 
 	/* get feature flags first */
 	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
-		printk(KERN_ERR "cramfs: unsupported filesystem features\n");
+		dprintk(KERN_ERR "cramfs: unsupported filesystem features\n");
 		goto out;
 	}
 
 	/* Check that the root inode is in a sane state */
 	if (!S_ISDIR(super.root.mode)) {
-		printk(KERN_ERR "cramfs: root is not a directory\n");
+		dprintk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
 	root_offset = super.root.offset << 2;
@@ -247,12 +255,12 @@
 	sbi->magic=super.magic;
 	sbi->flags=super.flags;
 	if (root_offset == 0)
-		printk(KERN_INFO "cramfs: empty filesystem");
+		dprintk(KERN_INFO "cramfs: empty filesystem");
 	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
 		 ((root_offset != sizeof(struct cramfs_super)) &&
 		  (root_offset != 512 + sizeof(struct cramfs_super))))
 	{
-		printk(KERN_ERR "cramfs: bad root offset %lu\n", root_offset);
+		dprintk(KERN_ERR "cramfs: bad root offset %lu\n", root_offset);
 		goto out;
 	}
 

Re: [PATCH] make cramfs look less hostile

[Christoph Hellwig]

On Sun, Jun 15, 2003 at 07:58:15PM +0200, Jörn Engel wrote:
> Ok, you win this one.  Leaves this case:
> $ mount /dev/somewhere /mnt
> cramfs: wrong magic
> $ echo $?
> 0
> $

Umm, please explain.  You got a sucessfull mount of a non-cramfs
filesystem type but an error from cramfs?  What version of mount
do you use?

> Here I consider the message evil as well.  The patch below on the
> other hand might go a little too far.  All the other printk()s are
> rare and might hold some useful information for normal users as well.
> 
> Any arguments for or against this patch?

It's not a debug printk.  Please leave the messages in, it's really
helpful to see what failed when mount doesn't succeed.

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 19:03:49 +0100, Christoph Hellwig wrote:
> On Sun, Jun 15, 2003 at 07:58:15PM +0200, Jörn Engel wrote:
> > Ok, you win this one.  Leaves this case:
> > $ mount /dev/somewhere /mnt
> > cramfs: wrong magic
> > $ echo $?
> > 0
> > $
> 
> Umm, please explain.  You got a sucessfull mount of a non-cramfs
> filesystem type but an error from cramfs?  What version of mount
> do you use?

Actually, that is what I expect to get tomorrow, when I have access to
hardware and can verify it.

> It's not a debug printk.  Please leave the messages in, it's really
> helpful to see what failed when mount doesn't succeed.

Yes, I agree.  It is any the "Cramfs didn't find it's magic number,
now we'll try another filesystem instead. Usually this should be
harmless and you can safely ignore this message." that bothers me,
because the current wording sounds more like "Something terrible has
happened, go and find someone to bug with this."

What do you propose instead?

Jörn

-- 
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

Re: [PATCH] make cramfs look less hostile

[Christoph Hellwig]

On Sun, Jun 15, 2003 at 08:14:24PM +0200, Jörn Engel wrote:
> Yes, I agree.  It is any the "Cramfs didn't find it's magic number,
> now we'll try another filesystem instead.

The only places where this should happen is mounting the rootfs.
mount(8) has it's own filesystem type detection code and doesn't
call mount(2) unless it found a matching filesystem type.

Re: [PATCH] make cramfs look less hostile

[Andries Brouwer]

On Sun, Jun 15, 2003 at 07:18:53PM +0100, Christoph Hellwig wrote:
> On Sun, Jun 15, 2003 at 08:14:24PM +0200, Jörn Engel wrote:
> > Yes, I agree.  It is any the "Cramfs didn't find it's magic number,
> > now we'll try another filesystem instead.
> 
> The only places where this should happen is mounting the rootfs.
> mount(8) has it's own filesystem type detection code and doesn't
> call mount(2) unless it found a matching filesystem type.

Too optimistic a description.
Any person who likes reliable results will give mount a -t option.
If someone likes to gamble, and doesnt mind system crashes, he'll
omit the -t and let mount guess what the type should have been.
Mount has a battery of heuristics for a handful of filesystems.
If any of these succeeds mount will try that type.
If none succeeds, mount will try consecutively all types listed
in /proc/filesystems for which no heuristic is present.

(Reality is more complicated, but the above is a good first
approximation.)

Andries

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 23:49:09 +0200, Andries Brouwer wrote:
> On Sun, Jun 15, 2003 at 07:18:53PM +0100, Christoph Hellwig wrote:
> > 
> > The only places where this should happen is mounting the rootfs.
> > mount(8) has it's own filesystem type detection code and doesn't
> > call mount(2) unless it found a matching filesystem type.
> 
> Too optimistic a description.
> Any person who likes reliable results will give mount a -t option.
> If someone likes to gamble, and doesnt mind system crashes, he'll
> omit the -t and let mount guess what the type should have been.
> Mount has a battery of heuristics for a handful of filesystems.
> If any of these succeeds mount will try that type.
> If none succeeds, mount will try consecutively all types listed
> in /proc/filesystems for which no heuristic is present.

Actually, I have one example of reality matching Christoph's
description, so he wins this fight as well.

Patch below is trivial and does what I need.  Any further complaints?

Jörn

-- 
With a PC, I always felt limited by the software available. On Unix, 
I am limited only by my knowledge.
-- Peter J. Schoenster

--- linux-2.5.71/fs/cramfs/inode.c~cramfs_message	2003-06-16 11:05:04.000000000 +0200
+++ linux-2.5.71/fs/cramfs/inode.c	2003-06-16 11:05:56.000000000 +0200
@@ -218,7 +218,8 @@
 		/* check at 512 byte offset */
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		if (super.magic != CRAMFS_MAGIC) {
-			printk(KERN_ERR "cramfs: wrong magic\n");
+			if (!silent)
+				printk(KERN_ERR "cramfs: wrong magic\n");
 			goto out;
 		}
 	}

Re: [PATCH] make cramfs look less hostile

[Andries Brouwer]

On Mon, Jun 16, 2003 at 11:12:15AM +0200, J?rn Engel wrote:
> On Sun, 15 June 2003 23:49:09 +0200, Andries Brouwer wrote:
> > On Sun, Jun 15, 2003 at 07:18:53PM +0100, Christoph Hellwig wrote:
> > > 
> > > The only places where this should happen is mounting the rootfs.
> > > mount(8) has it's own filesystem type detection code and doesn't
> > > call mount(2) unless it found a matching filesystem type.
> > 
> > Too optimistic a description.
> > Any person who likes reliable results will give mount a -t option.
> > If someone likes to gamble, and doesnt mind system crashes, he'll
> > omit the -t and let mount guess what the type should have been.
> > Mount has a battery of heuristics for a handful of filesystems.
> > If any of these succeeds mount will try that type.
> > If none succeeds, mount will try consecutively all types listed
> > in /proc/filesystems for which no heuristic is present.
> 
> Actually, I have one example of reality matching Christoph's
> description, so he wins this fight as well.

Please don't distribute misinformation.
If you doubt, read the mount(8) code first.

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Mon, 16 June 2003 13:21:28 +0200, Andries Brouwer wrote:
> 
> Please don't distribute misinformation.
> If you doubt, read the mount(8) code first.

Sorry about that.  When working from memory, my judgment of valid and
invalid information is sometimes borked.

Jörn

-- 
When you close your hand, you own nothing. When you open it up, you
own the whole world.
-- Li Mu Bai in Tiger & Dragon

RE: [PATCH] make cramfs look less hostile

[Robert White]

The "cut to the quick" version is that the message should be changed to
something both useful and non threatening.  Something to make the message
stand out as informational.

how about:

"cramfs: info: device is not a cramfs filesystem: wrong magic"

Then add a note to the docs that says: this message means the system was
trying to see if this was a cramfs.  In the absence of a later message
saying something like "could not mount device" this message is harmless.

There is then the "no-duh" factor to at least club people over the head with
for not doing their research.

This, of course, would make more sense if every filesystem mount attempt had
a similar message as well as a "thingyfs: file system mounted" too.  Of
course that would be ugly.

Since that isn't going to happen, stick to the simple human engineering
solution and make the message scream "info" duck the terseness... 8-)

Rob.

Re: [PATCH] make cramfs look less hostile

[Jörn Engel]

On Sun, 15 June 2003 18:05:24 +0200, Jörn Engel wrote:
> To:	quinlan@transmeta.com

And got a bounce.  Another free office over there, Linus?

Anyway, unless someone has a better patch, just remove him from
MAINTAINERS.

The sourceforge page is still informative, so I kept that in.

Jörn

-- 
I can say that I spend most of my time fixing bugs even if I have lots
of new features to implement in mind, but I give bugs more priority.
-- Andrea Arcangeli, 2000

--- linux-2.5.71/MAINTAINERS~cramfs_maintain	2003-06-15 16:04:45.000000000 +0200
+++ linux-2.5.71/MAINTAINERS	2003-06-15 19:23:29.000000000 +0200
@@ -426,10 +426,8 @@
 S:	Maintained
 
 CRAMFS FILESYSTEM
-P:	Daniel Quinlan
-M:	quinlan@transmeta.com
-W:	http://sourceforge.net/projects/cramfs/
-S:	Maintained
+W:     http://sourceforge.net/projects/cramfs/
+S:     Orphan
 
 CREDITS FILE
 P:	John A. Martin

Re: [PATCH] make cramfs look less hostile

[H. Peter Anvin]

Followup to:  <20030615172747.GG1063@wohnheim.fh-wedel.de>
By author:    =?iso-8859-1?Q?J=F6rn?= Engel <joern@wohnheim.fh-wedel.de>
In newsgroup: linux.dev.kernel
>
> On Sun, 15 June 2003 18:05:24 +0200, Jörn Engel wrote:
> > To:	quinlan@transmeta.com
> 
> And got a bounce.  Another free office over there, Linus?
> 
> Anyway, unless someone has a better patch, just remove him from
> MAINTAINERS.
> 
> The sourceforge page is still informative, so I kept that in.
> 

Dan's current address is quinlan@pathname.com.  He has it on his
webpage, which in fact was the first Google hit, so I presume it's not
secret :)

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
Architectures needed: ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64