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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
