On Sat, Dec 17, 2016 at 12:08 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > 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. Have you looked? Because in actual fact they don't. Theoretically it's either: - kmalloc + fill + readlink_copy + kfree --> kmalloc + fill + set_delayed_call - declare char[] on stack + fill + readlink_copy --> kmalloc + fill + set_delayed_call Presumably it's the second one you are talking about becoming more complex. There's exactly one instance of that in the tree and it actually becomes cleaner after the change. Current code does: - guess max link size to be 50 (very scientifically I'm sure, but no explanation given) - call filler - hope it didn't get truncated Which becomes: - call filler which allocates correctly sized buffer. > 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. With the signature change we get a consistent interface for reading the contents of symlinks. With that it will never make sense to play the stupid get_ds/set_ds() games that we've had. And no need to duplicate helper functions, like page_readlink() that is exactly the same as page_getlink() only for the different interface. And no need to export readlink_copy() which is something the filesystems never actually want to care about. Having different interfaces for the same thing is going to be more complex. I just don't get it what you are opposed to here. Thanks, Miklos -- 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
