linux-kernel - Re: [PATCH] sysfs: kill an extra put in sysfs_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 Jul 2007 21:36:37 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Tejun Heo" <htejun@...il.com>
Cc:	"Gabriel C" <nix.or.die@...glemail.com>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	"Christoph Lameter" <clameter@....com>, gregkh@...e.de,
	miles.lane@...il.com
Subject: Re: [PATCH] sysfs: kill an extra put in sysfs_create_link() failure path

On 7/18/07, Tejun Heo <htejun@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/