Date: Fri, 16 Dec 2016 23:08:23 +0000
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [GIT PULL (resend)] readlink cleanup
Message-ID: <20161216230823.GU1555@ZenIV.linux.org.uk>
References: <20161216224859.GD27207@veci.piliscsaba.szeredi.hu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161216224859.GD27207@veci.piliscsaba.szeredi.hu>
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1760
Lines: 36

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.