Patchwork [JFFS2] jffs2_start_garbage_collect_thread() return value cleanup

login
register
mail settings
Submitter Gerard Lledo
Date June 2, 2009, 12:11 p.m.
Message ID <1243944686-7039-2-git-send-email-gerard.lledo@gmail.com>
Download mbox | patch
Permalink /patch/27996/
State New
Headers show

Comments

Gerard Lledo - June 2, 2009, 12:11 p.m.
There is no user of this return value in the kernel.  Change it to return void
instead.
---
 fs/jffs2/background.c |    7 +------
 fs/jffs2/os-linux.h   |    2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)
Andrew Morton - June 2, 2009, 10:11 p.m.
On Tue,  2 Jun 2009 15:11:26 +0300
Gerard Lledo <gerard.lledo@gmail.com> wrote:

> There is no user of this return value in the kernel.  Change it to return void
> instead.

Both these patches were missing your signed-off-by:.  As your first
version of the first patch wasn't missing it, I added it to both these
patches, OK?
Gerard Lledo - June 3, 2009, 6:09 a.m.
On Tue, Jun 02, 2009 at 03:11:12PM -0700, Andrew Morton wrote:
> On Tue,  2 Jun 2009 15:11:26 +0300
> Gerard Lledo <gerard.lledo@gmail.com> wrote:
> 
> > There is no user of this return value in the kernel.  Change it to return void
> > instead.
> 
> Both these patches were missing your signed-off-by:.  As your first
> version of the first patch wasn't missing it, I added it to both these
> patches, OK?
>

Thats fine.  I just forgot to add it.  Thanks, and sorry for the trouble/noise.
David Woodhouse - June 24, 2009, 10:09 a.m.
On Tue, 2009-06-02 at 15:11 +0300, Gerard Lledo wrote:
> There is no user of this return value in the kernel.  Change it to return void
> instead.

NAK. I hate this type of patch.

A function _should_ return an error value indicating success or failure,
if there's _any_ chance that it (or a future rewrite of it) may fail.

It's up to the _callers_ to act on that result, or not, as they see fit.

If you don't do that, you end up with whole stacks of functions which
neither handle nor propagate errors -- and it's a _complete_ pain when
you later find that you _do_ have to make one of the innermost functions
return an error.

Gerard, if you persist in sending this kind of patch, please could I ask
you to send me a cloth doll of yourself? I would like to stick pins in
it next time I have to retrofit error handling to a stack of functions
that lack it.

It's not even as if this patch is going to give any real performance
improvement. It's a single register load in a once-per-mount code path.

There _may_ be some places where this kind of patch is reasonable -- but
the commit comment should have a clear explanation of why the return
value will _never_ be needed, rather than merely observing that it is
not _currently_ used. Or show that there is a _significant_ improvement
obtained by doing so, perhaps.

Andrew, may I suggest that you look for such justification in future
patches of this type?
Andrew Morton - June 25, 2009, 3 a.m.
On Wed, 24 Jun 2009 11:09:01 +0100 David Woodhouse <dwmw2@infradead.org> wrote:

> On Tue, 2009-06-02 at 15:11 +0300, Gerard Lledo wrote:
> > There is no user of this return value in the kernel.  Change it to return void
> > instead.
> 
> NAK. I hate this type of patch.
> 
> A function _should_ return an error value indicating success or failure,
> if there's _any_ chance that it (or a future rewrite of it) may fail.
> 
> It's up to the _callers_ to act on that result, or not, as they see fit.

True.

> Andrew, may I suggest that you look for such justification in future
> patches of this type?

eh, I sometimes don't even look at them.  I just save them up in case of
maintainer fumblage.

I might retain this one as a "jffs2 fails to check
jffs2_start_garbage_collect_thread() return value" bug report :)

Patch

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 3ff50da..3ae7e5c 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -30,10 +30,9 @@  void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c)
 }
 
 /* This must only ever be called when no GC thread is currently running */
-int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c)
+void jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c)
 {
 	struct task_struct *tsk;
-	int ret = 0;
 
 	BUG_ON(c->gc_task);
 
@@ -44,15 +43,11 @@  int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c)
 	if (IS_ERR(tsk)) {
 		printk(KERN_WARNING "fork failed for JFFS2 garbage collect thread: %ld\n", -PTR_ERR(tsk));
 		complete(&c->gc_thread_exit);
-		ret = PTR_ERR(tsk);
 	} else {
 		/* Wait for it... */
 		D1(printk(KERN_DEBUG "JFFS2: Garbage collect thread is pid %d\n", tsk->pid));
 		wait_for_completion(&c->gc_thread_start);
-		ret = tsk->pid;
 	}
-
-	return ret;
 }
 
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c)
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index 2228380..f3184ac 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -151,7 +151,7 @@  static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
 }
 
 /* background.c */
-int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
+void jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);