Messages in this thread | | | Date | Wed, 18 Jul 2007 21:36:37 +0530 | From | "Satyam Sharma" <> | Subject | Re: [PATCH] sysfs: kill an extra put in sysfs_create_link() failure path |
| |
On 7/18/07, Tejun Heo <htejun@gmail.com> wrote: > Satyam Sharma wrote: > >> Well, I dunno. Probably my taste just sucks. Please feel free to > >> submit patches and/or suggest better ideas. > > > > OK, for example: > > > > sysfs_find_dirent() -- to check for -EEXIST -- should be called > > *before* we create the new dentry for the to-be-created symlink > > in the first place. [ It's weird to grab a reference on the target > > for ourselves (and in fact even allocate the new dirent for the > > to-be-created symlink) and /then/ check for erroneous usage, > > and then go about undoing all that we should never have done > > at all. ] So this test could, and should, be made earlier, IMHO. > > Locking.
Well s/sysfs_find_dirent/sysfs_get_dirent/ then. And then simply put down the reference later.
> Otherwise, why would the code look like that in the first place?
> > And some similar others ... so attached (sorry, Gmail web > > interface) please find an attempt to make sysfs_create_link look > > a trifle more like what it should look like, IMHO. The code cleanup > > also leads to fewer LOC, smaller kernel image (lesser by 308 bytes), > > and even speeding up the no-error common case of this function, > > apart from the obvious readability benefits ... it's diffed on _top_ of > > your bugfix here, but not the other patch. [ Compile-tested only. ] > > Compounded if-else vs. flattened if () with common error path is pretty > much matter of being accustomed to. I prefer the latter because it > scales better (less nesting and less need for extra intelligence as > error case grows). As I'm already used to it, it's also easier on my > eyes.
Umm, I don't see any compounded if-else that I added that wasn't there already ... if any are, they only make the code clearly obvious as to what it's doing in the first place. And we've still got a common error path. Just that the error paths do not *need* to share any other code than the simple "return error;" precisely because it's been cleaned up. The existing code was just horrible, IMHO.
> So, unless you have more to offer, I'm not really sure whether > the patch improves the situation noticeably.
Readability, fewer LOC, 308 lesser bytes in kernel image and faster for the common case -- not good enough for you?! Oh, well.
Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |