[PATCH] fix for idiocy in mount_root cleanups.

[PATCH] fix for idiocy in mount_root cleanups.

[Alexander Viro]

	Grr...  OK, that should be a lesson - never do obvious change
just before sending a patch.

	Change in question was s/do_mount/mount/.  Which is almost
the same thing, except for one little detail:  mount(2) in case of
error returns -1 and stores the error in errno.  do_mount(9), OTOH...

IOW, please apply the following and pass me a brown paperbag ;-/

--- C1-pre7/init/do_mounts.c	Fri Dec  7 20:48:43 2001
+++ linux/init/do_mounts.c	Sat Dec  8 06:29:20 2001
@@ -351,8 +351,9 @@
 		mount("devfs", ".", "devfs", 0, NULL);
 retry:
 	for (p = fs_names; *p; p += strlen(p)+1) {
-		err = mount(name,"/root",p,root_mountflags,root_mount_data);
-		switch (err) {
+		errno = 0;
+		mount(name,"/root",p,root_mountflags,root_mount_data);
+		switch (-errno) {
 			case 0:
 				goto done;
 			case -EACCES:

Re: [PATCH] fix for idiocy in mount_root cleanups.

[Linus Torvalds]

On Sat, 8 Dec 2001, Alexander Viro wrote:
>
> 	Change in question was s/do_mount/mount/.  Which is almost
> the same thing, except for one little detail:  mount(2) in case of
> error returns -1 and stores the error in errno.  do_mount(9), OTOH...

Don't use errno in the kernel. EVER. This is a bug, we should get rid of
it.

Please fix "mount()" instead.

"errno" is one of those few _really_ bad stupidities in UNIX. Linux has
fixed it, and doesn't use it internally, and never shall. Too bad that
user space has to fix up the _correct_ error code returning that the
kernel does, and turn it into the "errno" stupidity for backwards
compatibility.

(If you care why "errno" is stupid, just think about threading and
performance)

		Linus

Re: [PATCH] fix for idiocy in mount_root cleanups.

[Alexander Viro]

On Sat, 8 Dec 2001, Linus Torvalds wrote:

> "errno" is one of those few _really_ bad stupidities in UNIX. Linux has
> fixed it, and doesn't use it internally, and never shall. Too bad that
> user space has to fix up the _correct_ error code returning that the
> kernel does, and turn it into the "errno" stupidity for backwards
> compatibility.
> 
> (If you care why "errno" is stupid, just think about threading and
> performance)

Oh, I know why errno is stupid and normally my reaction would be the
same.  However, in this case I would prefer to add

static int errno;

to do_mounts.c and be done with that.  Reasons:

a) eventually it's going to userland, at which point the issue becomes moot
(we won't share memory with anybody and don't do signal handlers).

b) within this file we have at most two threads of execution (do_linuxrc()
and the rest).  mount_root() is called only while do_linuxrc() is either
not started yet or had already exited.  IOW, even in kernel it's safe.

c) when it's in userland we will be really better off with _syscall5 for
mount() - we can rewrite it by hands to return the value in sane manner,
but that means bringing a _lot_ of arch-specific inline assmebler into
the file.  Which we might have to do anyway, OTOH...

Dunno.  At this stage I'd prefer the patch I've posted + static int errno;
in do_mounts.c.  Alternatively, we can replace mount() with sys_mount() in
that place (everything else simply doesn't give a damn for errno) and deal
with that when we move code to userland.

Re: [PATCH] fix for idiocy in mount_root cleanups.

[Alexander Viro]

	OK, for the time being we can simply go with

diff -urN C1-pre7/init/do_mounts.c C1-pre7-fix/init/do_mounts.c
--- C1-pre7/init/do_mounts.c	Fri Dec  7 20:48:43 2001
+++ C1-pre7-fix/init/do_mounts.c	Sat Dec  8 15:54:46 2001
@@ -351,7 +351,8 @@
 		mount("devfs", ".", "devfs", 0, NULL);
 retry:
 	for (p = fs_names; *p; p += strlen(p)+1) {
-		err = mount(name,"/root",p,root_mountflags,root_mount_data);
+		int err;
+		err = sys_mount(name,"/root",p,root_mountflags,root_mount_data);
 		switch (err) {
 			case 0:
 				goto done;

Is that OK with you?

Re: [PATCH] fix for idiocy in mount_root cleanups.

[Pavel Machek]

Hi!

> 	OK, for the time being we can simply go with
> 
> diff -urN C1-pre7/init/do_mounts.c C1-pre7-fix/init/do_mounts.c
> --- C1-pre7/init/do_mounts.c	Fri Dec  7 20:48:43 2001
> +++ C1-pre7-fix/init/do_mounts.c	Sat Dec  8 15:54:46 2001
> @@ -351,7 +351,8 @@
>  		mount("devfs", ".", "devfs", 0, NULL);
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  retry:
>  	for (p = fs_names; *p; p += strlen(p)+1) {
> -		err = mount(name,"/root",p,root_mountflags,root_mount_data);
> +		int err;
> +		err = sys_mount(name,"/root",p,root_mountflags,root_mount_data);
>  		switch (err) {
>  			case 0:
>  				goto done;
> 
> Is that OK with you?

Why do you *need* to declare err, when you did not need it before?

[Calling sys_mount is indeed right way to do this. Ouch, and look 4
lines above that. Do I see "mount()" without checking error return?]

								Pavel
-- 
"I do not steal MS software. It is not worth it."
                                -- Pavel Kankovsky

Re: [PATCH] fix for idiocy in mount_root cleanups.

[Pavel Machek]

Hi!

> diff -urN C1-pre7/init/do_mounts.c C1-pre7-fix/init/do_mounts.c
> --- C1-pre7/init/do_mounts.c	Fri Dec  7 20:48:43 2001
> +++ C1-pre7-fix/init/do_mounts.c	Sat Dec  8 15:54:46 2001
> @@ -351,7 +351,8 @@
>  		mount("devfs", ".", "devfs", 0, NULL);
>  retry:
>  	for (p = fs_names; *p; p += strlen(p)+1) {
> -		err = mount(name,"/root",p,root_mountflags,root_mount_data);
> +		int err;
> +		err = sys_mount(name,"/root",p,root_mountflags,root_mount_data);
>  		switch (err) {
>  			case 0:
>  				goto done;
> 
> Is that OK with you?

Also you should put space after , -- code around it uses that, and
this really looks ugly to my eyes.

							Pavel
-- 
"I do not steal MS software. It is not worth it."
                                -- Pavel Kankovsky

Re: [PATCH] fix for idiocy in mount_root cleanups.

[Alexander Viro]

On Sun, 9 Dec 2001, Pavel Machek wrote:

> [Calling sys_mount is indeed right way to do this. Ouch, and look 4
> lines above that. Do I see "mount()" without checking error return?]

Yes, you do and yes, it is the right thing.  It is called only if we know
that fs is there and the only possible error here is -ENOMEM.  And in
that case we will die since mount() attempts in the loop will also fail.
Resulting in immediate panic().  Well deserved-one, at that - if you
manage to OOM before starting init...