| Message ID | 1243944686-7039-2-git-send-email-gerard.lledo@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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?
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.
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?
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 :)
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);