Re: [git pull] more vfs bits

[Linus Torvalds]

On Sat, Feb 21, 2015 at 4:51 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> Looking at that queue, it might make sense to hold back everything in that
> series past "fanotify: Fix up scripted S_ISDIR/S_ISREG/S_ISLNK conversions"
> for now

Hmm. Even I'd pull just that, quite frankly, I just think it's
*confusing* to have those badly named "helpers", that were introduced
earlier in that series.

These guys are currently all teh same thing, but even if they weren't,
the naming is not helpful, and not sane:
 - fs_inode
 - fs_inode_once
 - dentry_inode
 - dentry_inode_once

Let's walk through them:

 - dentry_inode*() is supposed to be "the inode that would be used if
the dentry was opened"

   What part of "dentry_inode()" implies "if the dentry was opened" to
you? Nothing. The name is fundamentally bad.

  And what *possible* situation could make that "_once()" version ever
be valid? None. It's bogus. It's crap. It's insane. There is no way
that it is *ever* a valid question to even ask. If the dentry is so
unstable that you can't safely look at the inode, you had damn well
better never ask "ok, what would the inode be if I opened this random
pointer"?

   So one of them is badly named, and the other one is fundamentally
not a valid operation at all, as far as I can tell.

 - fs_inode*() is supposed to be "this is the inode that the native
filesystem uses".

   So again, I think the naming is horrible, since it doesn't really
follow the normal dentry helper routine names. But I'm sure we have
other cases where we screwed that up, so whatever..

   The "_once()" naming is doubly bad, as explained elsewhere. What
possible situation merits using that helper? If it's just
revalidate(), then make it about that.

   But more importantly, this is the one where I don't see how it
could ever possibly be anything but "dentry->d_inode". I'd much rather
just leave that.

So of the four new helpers, I really don't see any of them as "good".
I think "dentry_inode()" could remain, but even there I think the name
should specify *what* it is ("d_opened_inode()"? I don't like that
name either, but at least it would try to explain what the point is,
rather than having to look up a comment above the function definition
to figure out what the point is)

The strongest argument I've seen for them existing at all was that
"markers for what has been looked at". But that's something that
belongs in a development tree, not as a series to confuse others with.

                        Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/