Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new mount api syscall for linux #752

Merged
merged 3 commits into from
Jul 31, 2023

Conversation

yujincheng08
Copy link
Contributor

Fix #744

@yujincheng08 yujincheng08 force-pushed the mount branch 13 times, most recently from 1da5e3f to 034a14c Compare July 19, 2023 15:11
@yujincheng08
Copy link
Contributor Author

@sunfishcode we need to distinguish two move_mount's, any idea?

@sunfishcode
Copy link
Member

@yujincheng08 I'm not familiar with the move_mount API; could you describe the problem in more detail?

@yujincheng08
Copy link
Contributor Author

yujincheng08 commented Jul 19, 2023

There's a new linux syscall named move_mount, but now rustix has used the name move_mount for mount MS_MOVE, so we need to distinguish them

@sunfishcode sunfishcode mentioned this pull request Jul 19, 2023
28 tasks
@sunfishcode
Copy link
Member

Oh, I see. Next time we have a semver bump, we should rename that move_mount function, and have the actual move_mount syscall be named move_mount. I've now made a note of that in #753. But it will likely be quite a while before the next semver bump, so for now, how about naming the new function move_mount_syscall and add some comments in the existing move_mount mentioning that it's not the move_mount syscall?

@yujincheng08
Copy link
Contributor Author

yujincheng08 commented Jul 19, 2023

how about naming the new function move_mount_syscall and add some comments in the existing move_mount mentioning that it's not the move_mount syscall?

sounds good to me. i am afk for serval hours, and will update it then. or maybe you can help me correct it?

@yujincheng08 yujincheng08 mentioned this pull request Jul 20, 2023
6 tasks
@yujincheng08
Copy link
Contributor Author

@sunfishcode changed as requested. plz review again.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Would it be possible to write some simple tests that exercise these new functions? Or do these APIs require elevated privileges?

src/fs/mount.rs Show resolved Hide resolved
@sunfishcode
Copy link
Member

Thanks! Thinking about this more, I have a few additional thoughts.

It appears that documentation for these syscalls was posted as patches but never finished. What would you think about adding the following as documentation links?

fspick: https://lwn.net/ml/linux-kernel/159827189767.306468.1803062787718957199.stgit@warthog.procyon.org.uk/
fsconfig: https://lwn.net/ml/linux-kernel/159827191245.306468.4903071494263813779.stgit@warthog.procyon.org.uk/
open_tree: https://lwn.net/ml/linux-kernel/159827188271.306468.16962617119460123110.stgit@warthog.procyon.org.uk/
move_mount: https://lwn.net/ml/linux-kernel/159827189025.306468.4916341547843731338.stgit@warthog.procyon.org.uk/
fsopen: https://lwn.net/ml/linux-kernel/159827190508.306468.12755090833140558156.stgit@warthog.procyon.org.uk/
fsmount: https://lwn.net/ml/linux-kernel/159827190508.306468.12755090833140558156.stgit@warthog.procyon.org.uk/

Also, with the addition of these new functions, alongside the existing mount/remount/bind_mount/move_mount etc., it's starting to feel like there are enough of these functions and they're distinct enough that they should live in a new top-level rustix::mount instead of including them all in rustix::fs. We'd still need the existing rustix::fs names for semver compat, but they could become aliases for the real functions in rustix::mount that we remove when we bump. That would also be an opportunity to rename the existing move_mount. Do you think that would make sense? If you'd like, I'd be willing to make the actual change.

@yujincheng08
Copy link
Contributor Author

What would you think about adding the following as documentation links?

I also noticed these docs but I intently not adding them because they are not merged, and they are not in well format (not very human readable).

Do you think that would make sense?

Looks good to me! I like this idea :).

@yujincheng08
Copy link
Contributor Author

For documentation, maybe we can copy some contents of the unfinished patches to rust doc as a temporarily workaround? Once their docs finished, we can change them to link then.

@sunfishcode
Copy link
Member

I'd be ok with copying the unfinished documentation content into the tree; we should just be sure to cite where we copied it from, include their license, and mention why we need our own copy.

@yujincheng08
Copy link
Contributor Author

Thanks.

You previously said you would like to do the actual change. Is there anything else I need to do for this PR for merging?

@sunfishcode
Copy link
Member

Ah, no, this is fine. I'll merge this and make the change soon.

@sunfishcode sunfishcode merged commit d8cb8bb into bytecodealliance:main Jul 31, 2023
53 checks passed
@yujincheng08 yujincheng08 deleted the mount branch July 31, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new mount api for linux
2 participants