Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes:
> On Thu, Apr 02, 2015 at 08:56:20PM -0500, Eric W. Biederman wrote:
>
> One general note - I'd probably put a pointer to that sucker into struct
> mount. For one thing, root-preserving clone_mnt() is a fairly common
> case. For another, searching for that thing in mnt_put_root() looks
> wrong. Matter of taste, but...
So I just played with the possibilities and adding a field in struct
mount makes the code more complicated not less. So I have developed a
distaste for the idea of having a struct mountroot pointer in struct mount.
It especially complicates clone_mnt where I always have to have the code
look at the dentry to find the associated a dentry. Resuing a current
struct mountroot and/or preallocating one is just a complicated mess.
The current implementation has a much more localized (and thus
understandable and maintainable) implementation.
> Another thing is that IMO it's better to preallocate that thing in
> vfs_kern_mount() and free if it turns out to be unused. Simpler cleanup
> path that way...
It is a touch cleaner in vfs_kern_mount (not as many things need to be
freed) and much uglier and in clone_mnt.
>> - mnt->mnt.mnt_root = root;
>> + err = mnt_set_root(mnt, root);
>> + if (err) {
>> + dput(mnt->mnt.mnt_root);
>
> Unless I'm misreading your code, mnt_set_root() does *not* set it
> on failure, so what's going on here?
Typo. That should simply have been dput(root);
Eric
--
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
