Re: [GIT PULL (resend)] readlink cleanup

[Al Viro <viro@ZenIV.linux.org.uk>]

On Fri, Dec 16, 2016 at 11:48:59PM +0100, Miklos Szeredi wrote:

> This is a rework of the readlink cleanup patchset from the last cycle.  Now
> readlink(2) does the following:
> 
>  - if i_op->readlink() is non-NULL (only proc and afs mountpoints for now)
>    then it calls that
> 
>  - otherwise call i_op->get_link()
> 
>  - signature of ->readlink() now matches that of ->get_link()
> 
> In particular this last bullet point buys us:
> 
>  - less complexity, because we already handle the delayed free of the
>    buffer and copying to userspace due to ->get_link() being the normal way
>    to read the symlink

Less complexity where, exactly?  In the caller the life does not become
any simpler - instead of "call ->readlink() and bugger off" you have
"call ->readlink() and go through the same motions as in ->get_link()-based
case".  In the instances it becomes _more_ complex.

What's more, this new signature for ->readlink() makes no sense - instead of
"symlink traversal does not involve resolving a pathname, so we have to
fake one for readlink(2)" you get something resembling ->get_link(), which
would _not_ function as ->get_link() ought to.  But it can be called by the
same codepath that calls ->get_link(), saving us the burden of returning
without doing what ->get_link-based case would - we still get to check if
->readlink() is there, but we rejoin the common path immediately.  And AFAICS
that's the _only_ benefit of that signature change - making it possible to
reuse a few lines that adapt ->get_link() to readlinkat(2) needs.

IOW, I'm still not convinced.  Beginning of the series is fine - having
NULL ->readlink() interpreted for symlinks as "no override, use
generic_readlink()" makes a lot of sense.  This part, IMO, does not.