[TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Hareesh Nagarajan]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

The existing code in init_tmpfs() in mm/tiny-shmem.c does not handle the 
cases when the calls to register_filesystem() and kern_mount() fail. 
This patch adds those checks.

Signed-off-by: Hareesh Nagarajan <hnagar2@gmail.com>


[-- Attachment #2: tiny-shmmem-fix.patch --]
[-- Type: text/x-patch, Size: 623 bytes --]

--- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
+++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 00:13:10.532652000 -0500
@@ -31,12 +31,27 @@
 
 static int __init init_tmpfs(void)
 {
-	register_filesystem(&tmpfs_fs_type);
+	int error;
+
+	error = register_filesystem(&tmpfs_fs_type);
+	if (error) {
+		goto out2;
+	}
+
 #ifdef CONFIG_TMPFS
 	devfs_mk_dir("shm");
 #endif
 	shm_mnt = kern_mount(&tmpfs_fs_type);
+	if (IS_ERR(shm_mnt)) {
+		error = PTR_ERR(shm_mnt);
+		goto out1;
+	}
+
 	return 0;
+out1:
+	unregister_filesystem(&tmpfs_fs_type);
+out2:
+	return error;
 }
 module_init(init_tmpfs)
 

Re: [TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Matt Mackall]

On Mon, Oct 24, 2005 at 12:29:45AM -0500, Hareesh Nagarajan wrote:
> The existing code in init_tmpfs() in mm/tiny-shmem.c does not handle the 
> cases when the calls to register_filesystem() and kern_mount() fail. 
> This patch adds those checks.

Hmm. Did you actually encounter this?

I'd rather use BUG_ON. Passing up errors is only useful when the code
above can and will do something useful with the information. 

Here, we're talking about a built-in getting initialized at boot that
can quietly fail and the upper layer will simply shrug and go on,
leaving the system in a possibly useless state. Which is worse than
not attempting error handling at all, because we've added complexity
for no gain.

And what could the higher level, which is simply looping through init
functions, do to handle the error? Retry? Print a warning? Better to
stop everything outright when we encounter a problem we expect should
never happen so it doesn't go by undiagnosed.

> --- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
> +++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 00:13:10.532652000 -0500
> @@ -31,12 +31,27 @@
>  
>  static int __init init_tmpfs(void)
>  {
> -	register_filesystem(&tmpfs_fs_type);
> +	int error;
> +
> +	error = register_filesystem(&tmpfs_fs_type);
> +	if (error) {
> +		goto out2;
> +	}
> +
>  #ifdef CONFIG_TMPFS
>  	devfs_mk_dir("shm");
>  #endif
>  	shm_mnt = kern_mount(&tmpfs_fs_type);
> +	if (IS_ERR(shm_mnt)) {
> +		error = PTR_ERR(shm_mnt);
> +		goto out1;
> +	}
> +
>  	return 0;
> +out1:
> +	unregister_filesystem(&tmpfs_fs_type);
> +out2:
> +	return error;
>  }
>  module_init(init_tmpfs)
>  


-- 
Mathematics is the supreme nostalgia of our time.

Re: [TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Hareesh Nagarajan]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Matt Mackall wrote:
> On Mon, Oct 24, 2005 at 12:29:45AM -0500, Hareesh Nagarajan wrote:
>> The existing code in init_tmpfs() in mm/tiny-shmem.c does not handle the 
>> cases when the calls to register_filesystem() and kern_mount() fail. 
>> This patch adds those checks.
> 
> Hmm. Did you actually encounter this?

No, I haven't. I was just reading the source code when I chanced upon 
these trivial error checking omissions.

> I'd rather use BUG_ON. Passing up errors is only useful when the code
> above can and will do something useful with the information. 

[ Snip ]

> And what could the higher level, which is simply looping through init
> functions, do to handle the error? Retry? Print a warning? Better to
> stop everything outright when we encounter a problem we expect should
> never happen so it doesn't go by undiagnosed.

Makes sense. New patch attached.

Signed-off-by: Hareesh Nagarajan <hnagar2@gmail.com>

[-- Attachment #2: tiny-shmmem-fix-ver2.patch --]
[-- Type: text/x-patch, Size: 507 bytes --]

--- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
+++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 03:43:38.614071000 -0500
@@ -31,12 +31,18 @@
 
 static int __init init_tmpfs(void)
 {
-	register_filesystem(&tmpfs_fs_type);
+	int error;
+
+	error = register_filesystem(&tmpfs_fs_type);
+	BUG_ON(error);
+
 #ifdef CONFIG_TMPFS
 	devfs_mk_dir("shm");
 #endif
 	shm_mnt = kern_mount(&tmpfs_fs_type);
-	return 0;
+	BUG_ON(IS_ERR(shm_mnt));
+
+	return error;
 }
 module_init(init_tmpfs)
 

Re: [TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Matt Mackall]

On Mon, Oct 24, 2005 at 03:56:47AM -0500, Hareesh Nagarajan wrote:
> Matt Mackall wrote:
> >On Mon, Oct 24, 2005 at 12:29:45AM -0500, Hareesh Nagarajan wrote:
> >>The existing code in init_tmpfs() in mm/tiny-shmem.c does not handle the 
> >>cases when the calls to register_filesystem() and kern_mount() fail. 
> >>This patch adds those checks.
> >
> >Hmm. Did you actually encounter this?
> 
> No, I haven't. I was just reading the source code when I chanced upon 
> these trivial error checking omissions.
> 
> >I'd rather use BUG_ON. Passing up errors is only useful when the code
> >above can and will do something useful with the information. 
> 
> [ Snip ]
> 
> >And what could the higher level, which is simply looping through init
> >functions, do to handle the error? Retry? Print a warning? Better to
> >stop everything outright when we encounter a problem we expect should
> >never happen so it doesn't go by undiagnosed.
> 
> Makes sense. New patch attached.

A couple more comments..

> Signed-off-by: Hareesh Nagarajan <hnagar2@gmail.com>

> --- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
> +++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 03:43:38.614071000 -0500
> @@ -31,12 +31,18 @@
>  
>  static int __init init_tmpfs(void)
>  {
> -	register_filesystem(&tmpfs_fs_type);
> +	int error;
> +
> +	error = register_filesystem(&tmpfs_fs_type);
> +	BUG_ON(error);

Can we just do BUG_ON(register_filesystem() != 0)?

Strictly speaking, the != 0 is redundant, but as this goes slightly
against the grain of normal usage, it's a good indicator of intent.

> +
>  #ifdef CONFIG_TMPFS
>  	devfs_mk_dir("shm");
>  #endif
>  	shm_mnt = kern_mount(&tmpfs_fs_type);
> -	return 0;
> +	BUG_ON(IS_ERR(shm_mnt));
> +
> +	return error;

We can never return non-zero here. Returning error implies we can, so
it's confusing.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Muli Ben-Yehuda]

On Mon, Oct 24, 2005 at 01:45:18PM -0700, Matt Mackall wrote:

> > --- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
> > +++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 03:43:38.614071000 -0500
> > @@ -31,12 +31,18 @@
> >  
> >  static int __init init_tmpfs(void)
> >  {
> > -	register_filesystem(&tmpfs_fs_type);
> > +	int error;
> > +
> > +	error = register_filesystem(&tmpfs_fs_type);
> > +	BUG_ON(error);
> 
> Can we just do BUG_ON(register_filesystem() != 0)?

It seems a little risky to me to rely on a macro always evaluating its
arguments, even though this one does on every kernel version I
checked.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Matt Mackall]

On Mon, Oct 24, 2005 at 11:00:47PM +0200, Muli Ben-Yehuda wrote:
> On Mon, Oct 24, 2005 at 01:45:18PM -0700, Matt Mackall wrote:
> 
> > > --- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
> > > +++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 03:43:38.614071000 -0500
> > > @@ -31,12 +31,18 @@
> > >  
> > >  static int __init init_tmpfs(void)
> > >  {
> > > -	register_filesystem(&tmpfs_fs_type);
> > > +	int error;
> > > +
> > > +	error = register_filesystem(&tmpfs_fs_type);
> > > +	BUG_ON(error);
> > 
> > Can we just do BUG_ON(register_filesystem() != 0)?
> 
> It seems a little risky to me to rely on a macro always evaluating its
> arguments, even though this one does on every kernel version I
> checked.

You must have missed my patch on 1 April that allows turning off all
kernel bugs. It makes sure the arguments are still evaluated.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [TRIVIAL] Error checks omitted in init_tmpfs() in mm/tiny-shmem.c

[Hareesh Nagarajan]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

Matt Mackall wrote:
> On Mon, Oct 24, 2005 at 03:56:47AM -0500, Hareesh Nagarajan wrote:
>> Matt Mackall wrote:
>>> On Mon, Oct 24, 2005 at 12:29:45AM -0500, Hareesh Nagarajan wrote:
>>>> The existing code in init_tmpfs() in mm/tiny-shmem.c does not handle the 
>>>> cases when the calls to register_filesystem() and kern_mount() fail. 
>>>> This patch adds those checks.

[ Snip ]

> A couple more comments..
> 
>> Signed-off-by: Hareesh Nagarajan <hnagar2@gmail.com>
> 
>> --- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
>> +++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 03:43:38.614071000 -0500
>> @@ -31,12 +31,18 @@
>>  
>>  static int __init init_tmpfs(void)
>>  {
>> -	register_filesystem(&tmpfs_fs_type);
>> +	int error;
>> +
>> +	error = register_filesystem(&tmpfs_fs_type);
>> +	BUG_ON(error);
> 
> Can we just do BUG_ON(register_filesystem() != 0)?
> 
> Strictly speaking, the != 0 is redundant, but as this goes slightly
> against the grain of normal usage, it's a good indicator of intent.

It shows intent well. That goes into my book for good programming 
practices. No more BUG_ON(foo) :)

>>  #ifdef CONFIG_TMPFS
>>  	devfs_mk_dir("shm");
>>  #endif
>>  	shm_mnt = kern_mount(&tmpfs_fs_type);
>> -	return 0;
>> +	BUG_ON(IS_ERR(shm_mnt));
>> +
>> +	return error;
> 
> We can never return non-zero here. Returning error implies we can, so
> it's confusing.

Makes sense again. Patch follows.

Signed-off-by: Hareesh Nagarajan <hnagar2@gmail.com>

[-- Attachment #2: tiny-shmmem-fix-ver3.patch --]
[-- Type: text/x-patch, Size: 462 bytes --]

--- linux-2.6.13.4/mm/tiny-shmem.c	2005-10-10 13:54:29.000000000 -0500
+++ linux-2.6.13.4-edit/mm/tiny-shmem.c	2005-10-24 19:05:28.058897000 -0500
@@ -31,11 +31,14 @@
 
 static int __init init_tmpfs(void)
 {
-	register_filesystem(&tmpfs_fs_type);
+	BUG_ON(register_filesystem(&tmpfs_fs_type) != 0);
+
 #ifdef CONFIG_TMPFS
 	devfs_mk_dir("shm");
 #endif
 	shm_mnt = kern_mount(&tmpfs_fs_type);
+	BUG_ON(IS_ERR(shm_mnt));
+
 	return 0;
 }
 module_init(init_tmpfs)