[Kernel-janitors] [PATCH] linux-2.6.7-rc2/fs/cifs/cifsfs.c

[Kernel-janitors] [PATCH] linux-2.6.7-rc2/fs/cifs/cifsfs.c

[MJK]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1703 bytes --]


	Compiles. I thought the error handling could use a little
improvement. :)


--MJK

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=

Description  : Audits kernel_thread() and does a cosmetic change to the
way errors are handled.

Signed-off-by: MJK <mkemp@cs.nmsu.edu>

--- old.linux-2.6.7-rc2/fs/cifs/cifsfs.c	2004-06-02 02:58:58.000000000 -0600
+++ new.linux-2.6.7-rc2/fs/cifs/cifsfs.c	2004-06-06 02:32:27.494962953 -0600
@@ -761,25 +761,51 @@

 	rc = cifs_init_inodecache();
 	if (!rc) {
-		rc = cifs_init_mids();
-		if (!rc) {
-			rc = cifs_init_request_bufs();
-			if (!rc) {
-				rc = register_filesystem(&cifs_fs_type);
-				if (!rc) {
-					kernel_thread(cifs_oplock_thread, NULL,
-						CLONE_FS | CLONE_FILES | CLONE_VM);
-					return rc; /* Success */
-				} else
-					cifs_destroy_request_bufs();
-			}
-			cifs_destroy_mids();
-		}
-		cifs_destroy_inodecache();
+		printk(KERN_ERR "cifs_init_inodecache: error initilizing.\n");
+		goto error_init;
 	}
+
+	rc = cifs_init_mids();
+	if (!rc) {
+		goto error_init_mids;
+	}
+
+	rc = cifs_init_request_bufs();
+	if (!rc) {
+		goto error_request;
+	}
+
+	rc = register_filesystem(&cifs_fs_type);
+	if (!rc) {
+		printk(KERN_ERR "Couldn't register cifs.\n");
+		goto error_reg_fs;
+	}
+
+	rc = kernel_thread(cifs_oplock_thread, NULL, CLONE_FS | CLONE_FILES | CLONE_VM);
+	if (!rc) {
+		printk(KERN_ERR "Couldn't spawn cifs thread.\n");
+		goto error_kt;
+	}
+
+	return rc;
+
+error_kt:
+
+error_reg_fs:
+	cifs_destroy_request_bufs();
+
+error_request:
+	cifs_destroy_mids();
+
+error_init_mids:
+	cifs_destroy_inodecache();
+
+error_init:
+
 #ifdef CONFIG_PROC_FS
 	cifs_proc_clean();
 #endif
+
 	return rc;
 }


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] linux-2.6.7-rc2/fs/cifs/cifsfs.c

[Zach Brown]

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


> +	rc = register_filesystem(&cifs_fs_type);
> +	if (!rc) {
> +		printk(KERN_ERR "Couldn't register cifs.\n");

If you're going to go to the trouble of printing anything you should
include the return code.  It would drive my batty if I had chased that
error down into the source and found that it missed the opportunity to
give me a hint as to why things failed.

>  #ifdef CONFIG_PROC_FS
>  	cifs_proc_clean();
>  #endif

(in other news, there might be an opportunity to lose this ifdef by
defining a no-op cifs_proc_clean in a header)

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] linux-2.6.7-rc2/fs/cifs/cifsfs.c

[MJK]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 488 bytes --]


	I was wondering about that. :)

	But I wasn't sure what to say. That is why on some there are some
messages and none on others. I'll go back and try to improve the messages.

Thanks!
MJK

On Mon, 7 Jun 2004, Zach Brown wrote:

> If you're going to go to the trouble of printing anything you should
> include the return code.  It would drive my batty if I had chased that
> error down into the source and found that it missed the opportunity to
> give me a hint as to why things failed.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] linux-2.6.7-rc2/fs/cifs/cifsf

[Zach Brown]

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

 (Walter Harms) wrote:
> hi zach,
> i have rewritten the ini already. what do you mean nop cifs_proc_clean?
> somelike that ?

Something like that, yeah..

#ifdef CONFIG_PROC_FS
void cifs_proc_clean(void); /* usual prototype */
#else
#define cifs_proc_clean() do { } while (0)
#endif

This might not make sense if cifs_proc_clean() is in the same file.  I'm
not actually looking at the code, or anything :)

The thing to take away is that the #ifdef garbage should be dealt with
by the function and it's prototype in the headers, not by callers.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors