Re: [patch 2/2] move init_ext4_proc() last and add cleanup call exit_ext4_proc()

[Mark Nelson <markn@au1.ibm.com>]

Badari Pulavarty wrote:
> On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com wrote:
>> plain text document attachment (ext4-move-init_ext4_proc-add-
>> cleanup.patch)
>> The first problem that is addressed is that the addition of init_ext4_proc()
>> means that the code doesn't clean up after itself on a failure of
>> init_ext4_xattr(), and the second is that usually init_*_proc() should be
>> the last thing called, so we move it to the end and add the cleanup call to
>> exit_ext4_proc().
>>
>> Signed-off-by: Mark Nelson <markn@au1.ibm.com>
>> ---
>>  fs/ext4/super.c |    9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> Index: ext4/fs/ext4/super.c
>> ===================================================================
>> --- ext4.orig/fs/ext4/super.c
>> +++ ext4/fs/ext4/super.c
>> @@ -2999,10 +2999,6 @@ static int __init init_ext4_fs(void)
>>  {
>>  	int err;
>>  
>> -	err = init_ext4_proc();
>> -	if (err)
>> -		return err;
>> -
>>  	err = init_ext4_xattr();
>>  	if (err)
>>  		return err;
>> @@ -3012,7 +3008,12 @@ static int __init init_ext4_fs(void)
>>  	err = register_filesystem(&ext4dev_fs_type);
>>  	if (err)
>>  		goto out;
>> +	err = init_ext4_proc();
>> +	if (err)
>> +		goto out_proc;
>>  	return 0;
>> +out_proc:
>> +	exit_ext4_proc();
>>  out:
>>  	destroy_inodecache();
>>  out1:
> 
> Nope. You can not call exit_ext4_proc() if there is a failure
> in init_ext4_proc(). If the kmem_cache_create() for ext4_pspace_cachep
> fails, you would end up calling kmem_cache_destory(NULL).
> 

Oh yes, you're exactly right. Sorry, I meant to call unregister_filesystem.
(connection between brain and hands temporarily lost while typing...)

What I should have had was the following:


Subject: move init_ext4_proc() last and add unregister_filesystem() cleanup 

The addition of init_ext4_proc() means that the code doesn't clean up
after itself on a failure of init_ext4_xattr(), so move the call to
init_ext4_proc() last and then add cleanup call to unregister_filesystem().

Signed-off-by: Mark Nelson <markn@au1.ibm.com>
---
 fs/ext4/super.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: ext4/fs/ext4/super.c
===================================================================
--- ext4.orig/fs/ext4/super.c
+++ ext4/fs/ext4/super.c
@@ -2999,10 +2999,6 @@ static int __init init_ext4_fs(void)
 {
 	int err;
 
-	err = init_ext4_proc();
-	if (err)
-		return err;
-
 	err = init_ext4_xattr();
 	if (err)
 		return err;
@@ -3012,7 +3008,12 @@ static int __init init_ext4_fs(void)
 	err = register_filesystem(&ext4dev_fs_type);
 	if (err)
 		goto out;
+	err = init_ext4_proc();
+	if (err)
+		goto out_filesystem;
 	return 0;
+out_filesystem:
+	unregister_filesystem(&ext4dev_fs_type);
 out:
 	destroy_inodecache();
 out1:




Sorry about the patch noise Andrew...


Thanks Badari!

Mark.


If helping turns to hindering, let me know and I'll leave you ext4 experts in peace :)

> The best way is to make init_ext4_proc() cleanup itself, in case
> of an error. Hmm.. we are not handling proc_mkdir(EXT4_ROOT) 
> failures.
> 
> Thanks,
> Badari
>