On Sun, Jan 20, 2013 at 7:52 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
>
> You've now conflated two completely different lock paths into a single
> unlock.
We have that elsewhere too. And it's what we used to have before too.
So the simple fact is that commit 1fb9341ac348 just introduced this
bug, and moving the goto target around is the obvious fix for it, and
makes it match the old code that was simply incorrectly modified.
The suggested patch instead has *some* cleanup inside the
if-statement, and some at the goto target. That makes no sense to
humans, and just makes it harder for the compiler to generate better
code.
> mutex_bug_cleanup() should really lock internally, but doesn't
> so we wrap it. And that mutex_unlock of yours has nothing to do with
> cleaning up ddebug, so the labels misnamed, at best.
Bah, humbug. It's called "ddebug_cleanup" because it's called after
the debug setup, so it needs to clean up the state set up by that.
The fact that it needs to unlock is secondary, and is simply because
the lock is taken at that point, so needs to be released. The naming
is not wonderful, but it's not hugely illogical, and again, that's
what it used to (except "ddebug" has been renamed to
"ddebug_cleanup"). You could rename it if you want to (we used to have
a target called "unlock" at that point), but that's *still* no excuse
for just creating code that does cleanup in two totally unrelated
places.
> Not that it matters much: this is going to change for next merge window.
Now, agreed, that looks better, although I suspect you could have
taken the "split that ugly function up" further still.
Linus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
