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

mount/linux: add a safe wrapper for open_tree(2) #1958

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Jan 6, 2023

This adds an ergonomic wrapper for the Linux-specific open_tree(2)
syscall, using safe-io file descriptors.
The syscall has been introduced in kernel version 5.2 and allows
FD-based manipulation of mount hierarchies.

@lucab lucab force-pushed the ups/linux-mount branch 3 times, most recently from bc17641 to d723bce Compare January 6, 2023 14:43
@lucab
Copy link
Contributor Author

lucab commented Jan 6, 2023

Mmh, it looks like the libc crate is missing some constants on Android and uclibc. I'll try to find some time to look into that.

@lucab lucab force-pushed the ups/linux-mount branch 2 times, most recently from 46e2944 to f55782b Compare January 12, 2023 19:49
@lucab
Copy link
Contributor Author

lucab commented Jan 22, 2023

Ok, I landed several fixes in libc and now the CI here is green. This is ready for review.

@lucab
Copy link
Contributor Author

lucab commented Jan 30, 2023

@rtzoeller @asomers may I ask for a review on this one?

src/mount/linux.rs Outdated Show resolved Hide resolved
test/test_mount.rs Show resolved Hide resolved
@SteveLauC
Copy link
Member

Sorry for such a slow response! And thanks for you work!

I have left some comments on this PR :)

lucab added 2 commits October 6, 2023 16:26
This adds an ergonomic wrapper for the Linux-specific `open_tree(2)`
syscall, using safe-io file descriptors.
The syscall has been introduced in kernel version 5.2 and allows
FD-based manipulation of mount hierarchies.
@lucab
Copy link
Contributor Author

lucab commented Oct 6, 2023

@SteveLauC thanks for the review! (And no need to apologise 💙)
I've rebased against latest master and addressed the comments.

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, again! Left some comments

Comment on lines +45 to +47
- Added `mount::open_tree()` helper on Linux.
([#1958](https://github.com/nix-rust/nix/pull/1958))

Copy link
Member

Choose a reason for hiding this comment

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

We have changed our changelog mode in #2149 (the old one is way too conflict-prone), please see CONTRIBUTING.md on how to add a changelog

flags: OpenTreeFlags,
) -> Result<OwnedFd> {
let res = pathname.with_nix_path(|cstr| unsafe {
let raw_dirfd = at_rawfd(dirfd.map(|v| v.as_fd().as_raw_fd()));
Copy link
Member

Choose a reason for hiding this comment

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

at_rawfd() has been gated with:

#[cfg(any(
    all(feature = "fs", not(target_os = "redox")),
    all(feature = "process", any(target_os = "android", target_os = "linux"))
))]
pub(crate) fn at_rawfd(fd: Option<RawFd>) -> raw::c_int {
    fd.unwrap_or(libc::AT_FDCWD)
}

Given that open_tree() is under feature mount and for Linux and Android, we should probably gate at_rawfd with:

#[cfg(any(
    all(feature = "fs", not(target_os = "redox")),
    all(
        any(feature = "process", feature = "mount"),
        any(target_os = "android", target_os = "linux")
    )
))]
pub(crate) fn at_rawfd(fd: Option<RawFd>) -> raw::c_int {
    fd.unwrap_or(libc::AT_FDCWD)
}

/// If target is a symbolic link, do not dereference it.
AT_SYMLINK_NOFOLLOW as c_uint;
/// Clone the mount object.
OPEN_TREE_CLONE as c_uint;
Copy link
Member

Choose a reason for hiding this comment

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

Is this cast necessary, it seems that this constant is of type c_uint on both Linux and Android:

$ rg "OPEN_TREE_CLONE"
src/unix/linux_like/android/mod.rs
2174:pub const OPEN_TREE_CLONE: ::c_uint = 0x01;

src/unix/linux_like/linux/mod.rs
3328:pub const OPEN_TREE_CLONE: ::c_uint = 0x01;

libc-test/semver/linux.txt
1728:OPEN_TREE_CLONE

libc-test/semver/android.txt
1754:OPEN_TREE_CLONE

Copy link
Member

Choose a reason for hiding this comment

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

To support both Linux and Android, we use the following alias, for more detail, see cfg-gates

#[cfg(linux_android)]

/// Clone the mount object.
OPEN_TREE_CLONE as c_uint;
/// Set the close-on-exec flag for the returned file descriptor.
OPEN_TREE_CLOEXEC as c_uint;
Copy link
Member

Choose a reason for hiding this comment

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

Same to this one:

$ rg "OPEN_TREE_CLOEXEC"
libc-test/semver/linux.txt
1727:OPEN_TREE_CLOEXEC

libc-test/semver/android.txt
1753:OPEN_TREE_CLOEXEC

src/unix/linux_like/linux/mod.rs
3329:pub const OPEN_TREE_CLOEXEC: ::c_uint = O_CLOEXEC as ::c_uint;

src/unix/linux_like/android/mod.rs
2175:pub const OPEN_TREE_CLOEXEC: ::c_uint = O_CLOEXEC as ::c_uint;

@@ -203,6 +204,31 @@ exit 23";
assert_eq!(buf, SCRIPT_CONTENTS);
}

pub(crate) fn test_open_tree() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would like to change this to pub fn test_open_tree() so that it could be consistent with the other functions in this module :)

@SteveLauC
Copy link
Member

Gentle on the author @lucab, would you like to finish this PR:)

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.

2 participants