On Mon, Jun 4, 2018 at 5:52 PM, Al Viro <v...@zeniv.linux.org.uk> wrote: > On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote: > >> fsopen = create fsfd >> fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd >> fspick = path -> fsfd >> move_mount = attach mountfd or move existing >> fsinfo = info from path >> open_tree = new mountfd from path or clone >> mount_setattr = set attr on mountfd >> >> Notice that fsmount() encompasses mount_setattr() + move_mount() >> functionality. Split those out and leave fsmount() to actually do >> the "fsfd ->mountfd" translation? > > Might make sense.
> FWIW, to make it clear: fsmount(2) in this series actually does *NOT* > attach it to the tree. Ah, that leaves the mount_setattr() functionality to split out. I'd be more happy to rid this new API of all the old MS_* crap and have have a new set of attributes, that just apply to mounts. It will also need two args: a bitmap of new attributes and a mask to tell us which attributes to change. > Commit message definitely needs updating - as it > is, it's > > +SYSCALL_DEFINE5(fsmount, int, fs_fd, unsigned int, flags, unsigned int, > ms_flags, > + void *, reserved4, void *, reserved5) > > PS: IMO these reserved... arguments are in bad taste; if anyone has good > reasons > for that practice in ABI design, I'd like to hear those. Agreed. A flags argument is often wise to add even if currently unused (and should be checked for undefined flags), but adding a random number of pointers doesn't seem to make a lot of sense. > >> fsinfo() name suggests it's in the same class as >> fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I >> think that's slightly confusing. >> >> Rename move_mount() -> mount_move()? > > mount_move_bikeshed_bikeshed_bikeshed(), surely? Consistent naming for related functions... not unheard of in API design. The above set definitely does not qualify. >> Also does it make sense to make the cloning behavior of open_tree() >> optional? Without cloning it's just a plain open(O_PATH). That way >> it could be renamed mount_clone(). > > Umm... I'm not sure about that one. If nothing else, OPEN_TREE_DETACH > might be a good idea, in which case cloning is not the primary effect; > hell knows. So conceptually we have the following distinct mount tree operations: treefd = clone(path); treefd = detach(path); attach(treefd, path); move(path1, path2); The detach/move/attach trio are more related in functionality, while clone and detach have the same signature. I'm not sure either. Thanks, Miklos
