Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd452afdab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
cberner
left a comment
There was a problem hiding this comment.
Cool, I've wanted to remove the fusermount dependency for a while.
I had some comments, and also please add a test for this feature to the Linux & FreeBSD suites
src/mnt/fuse_direct.rs
Outdated
| MountOption::CUSTOM(val) if val == "diratime" => flags &= !MsFlags::MS_NODIRATIME, | ||
| MountOption::CUSTOM(val) if val == "nodiratime" => flags |= MsFlags::MS_NODIRATIME, | ||
| MountOption::CUSTOM(val) if val == "lazytime" => flags |= MsFlags::MS_LAZYTIME, | ||
| MountOption::CUSTOM(val) if val == "nolazytime" => flags &= !MsFlags::MS_LAZYTIME, | ||
| MountOption::CUSTOM(val) if val == "relatime" => flags |= MsFlags::MS_RELATIME, | ||
| MountOption::CUSTOM(val) if val == "norelatime" => flags &= !MsFlags::MS_RELATIME, | ||
| MountOption::CUSTOM(val) if val == "strictatime" => { | ||
| flags |= MsFlags::MS_STRICTATIME | ||
| } | ||
| MountOption::CUSTOM(val) if val == "nostrictatime" => { | ||
| flags &= !MsFlags::MS_STRICTATIME | ||
| } |
There was a problem hiding this comment.
I don't think we should handle these CUSTOM options here. Instead please submit a separate PR that adds them to the MountOption enum
There was a problem hiding this comment.
Sounds good. I only covered these options to because the reference fusermount does.
| let mut flags = MsFlags::MS_NOSUID | MsFlags::MS_NODEV; | ||
|
|
||
| let mut opts = Vec::new(); | ||
| for opt in options { |
There was a problem hiding this comment.
The for loop looks very similar to the code in the "pure" implementation. Let's reuse the option_group() and option_to_flag() functions
…st impl and removed handling of CUSTOM options
|
It's going to take some time for me to get FreeBSD working. It works differently enough that I'm going to have to review the |
There was a problem hiding this comment.
Pull request overview
Adds a new direct-mount mount backend that performs FUSE mounts via direct kernel mount syscalls (without requiring fusermount), including an auto-unmount daemon mode.
Changes:
- Introduces
direct-mountmount implementation and build-time selection viafuser_mount_impl. - Refactors mount option grouping/flag conversion helpers into
mount_options.rsfor reuse. - Extends test harness to run mount tests with the new
direct-mountfeature.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mnt/mount_options.rs | Adds shared option grouping + option→flag conversion for direct mount backend |
| src/mnt/mod.rs | Wires in the new direct-mount backend behind cfg |
| src/mnt/fuse_pure.rs | Switches to shared option helpers after refactor |
| src/mnt/fuse_direct.rs | New direct syscall-based mount/unmount + auto-unmount daemon |
| fuser-tests/src/features.rs | Adds DirectMount test feature flag |
| fuser-tests/src/commands/mount.rs | Runs existing mount tests with DirectMount enabled |
| fuser-tests/src/commands/bsd_mount.rs | Runs BSD mount tests with and without DirectMount |
| build.rs | Adds cfg selection logic for direct-mount |
| Cargo.toml | Adds direct-mount feature and required nix subfeatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/mnt/mount_options.rs
Outdated
| #[cfg_attr( | ||
| all(fuser_mount_impl = "direct-mount", fuser_mount_impl = "macos-no-mount"), | ||
| expect(dead_code) | ||
| )] | ||
| #[derive(PartialEq)] | ||
| pub(crate) enum MountOptionGroup { | ||
| KernelOption, | ||
| KernelFlag, | ||
| Fusermount, | ||
| } |
There was a problem hiding this comment.
The cfg_attr condition uses all(fuser_mount_impl = "direct-mount", fuser_mount_impl = "macos-no-mount"), which can never be true (a single cfg key can’t equal two different values at once). If the intent is to suppress dead_code when either impl is selected, use any(...), or move the expect(dead_code) to the specific platforms/configs where the enum is genuinely unused.
There was a problem hiding this comment.
There is no longer a cfg_attr on this type
src/mnt/fuse_direct.rs
Outdated
| let mut opts = Vec::new(); | ||
| for opt in options { | ||
| match option_group(opt) { | ||
| MountOptionGroup::KernelFlag => flags |= option_to_flag(opt)?, | ||
| MountOptionGroup::KernelOption => write!(opts, "{},", option_to_string(opt))?, | ||
| MountOptionGroup::Fusermount => match opt { | ||
| MountOption::FSName(val) => fsname = Some(val), | ||
| MountOption::Subtype(val) => subtype = Some(val), | ||
| MountOption::AutoUnmount => auto_unmount = true, | ||
| _ => {} | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
opts is built here but never used afterwards (the kernel options are rebuilt inside do_mount). This adds extra work and duplicates formatting/error paths. Consider removing opts from new() and only extracting flags/fsname/subtype/auto_unmount here, leaving kernel-option serialization to do_mount.
There was a problem hiding this comment.
opts was left over from the original linux-only implementation. The logic has been moved to the linux implementation of MountImpl::do_mount.
| let root_mode = mountpoint | ||
| .metadata() | ||
| .map(|meta| meta.mode() & nix::sys::stat::SFlag::S_IFMT.bits())?; | ||
|
|
||
| let old_len = opts.len(); | ||
| write!( | ||
| opts, | ||
| "fd={},rootmode={:o},user_id={},group_id={}", | ||
| dev_fd, | ||
| root_mode, | ||
| Uid::current().as_raw(), | ||
| nix::unistd::Gid::current().as_raw(), | ||
| )?; |
There was a problem hiding this comment.
rootmode is derived using mode() & S_IFMT, which keeps only the file type bits and drops permission bits. This will produce values like 040000 (directory type) without actual permissions, which can cause incorrect access checks/behavior compared to fusermount. Use the full mode (or at least preserve permission bits, e.g., type bits + 0o7777) to match expected kernel FUSE mount semantics.
There was a problem hiding this comment.
mode & S_IFMT is what fusermount does:
res = do_mount(real_mnt, type, stbuf.st_mode & S_IFMT,
fd, do_mount_opts, dev, &source, &mnt_opts);| pub(crate) fn umount_impl(&mut self) -> io::Result<()> { | ||
| self.do_unmount(true) | ||
| } |
There was a problem hiding this comment.
On non-Linux targets, lazy=true maps to MNT_FORCE, and umount_impl() always passes true, meaning unmount becomes forced by default. That’s a much stronger behavior than Linux MNT_DETACH semantics and can be disruptive. Consider making umount_impl() do a non-forced unmount first (empty flags), and only retry with MNT_FORCE when explicitly requested or when a non-forced attempt fails with EBUSY.
There was a problem hiding this comment.
Fixed so MNT_FORCE is used when lazy=false.
| fn do_unmount(&mut self, lazy: bool) -> io::Result<()> { | ||
| let flags = if lazy { | ||
| nix::mount::MntFlags::MNT_FORCE | ||
| } else { | ||
| nix::mount::MntFlags::empty() | ||
| }; | ||
| nix::mount::unmount(&self.mountpoint, flags)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
On non-Linux targets, lazy=true maps to MNT_FORCE, and umount_impl() always passes true, meaning unmount becomes forced by default. That’s a much stronger behavior than Linux MNT_DETACH semantics and can be disruptive. Consider making umount_impl() do a non-forced unmount first (empty flags), and only retry with MNT_FORCE when explicitly requested or when a non-forced attempt fails with EBUSY.
There was a problem hiding this comment.
This was the opposite of what it should be. Fixed so lazy=true results in empty flags, and lazy=false results in MNT_FORCE.
src/mnt/fuse_direct.rs
Outdated
| fn do_auto_unmount(&mut self, mut pipe: UnixStream) -> io::Result<()> { | ||
| close_inherited_fds(pipe.as_raw_fd()); | ||
| let _ = setsid(); | ||
| let _ = sigprocmask(SigmaskHow::SIG_BLOCK, Some(&SigSet::empty()), None); |
There was a problem hiding this comment.
SIG_BLOCK with an empty set is effectively a no-op (it blocks nothing). If the intent is to clear the signal mask in the daemon after fork(), this should likely be SigmaskHow::SIG_SETMASK with an empty set; otherwise, the call can be removed to reduce confusion.
| let _ = sigprocmask(SigmaskHow::SIG_BLOCK, Some(&SigSet::empty()), None); | |
| let _ = sigprocmask(SigmaskHow::SIG_SETMASK, Some(&SigSet::empty()), None); |
There was a problem hiding this comment.
Yeah, this was supposed to be SigSet::all(). Fixed
| fn should_auto_unmount(&self) -> io::Result<bool> { | ||
| let etc_mtab = Path::new("/etc/mtab"); | ||
| let proc_mounts = Path::new("/proc/mounts"); | ||
|
|
||
| let mtab_path = if etc_mtab.try_exists()? { | ||
| etc_mtab | ||
| } else if proc_mounts.try_exists()? { | ||
| proc_mounts | ||
| } else { | ||
| return Err(io::ErrorKind::NotFound.into()); | ||
| }; |
There was a problem hiding this comment.
direct-mount is enabled for BSD targets in build.rs, but should_auto_unmount() relies on Linux-style /proc/mounts / /etc/mtab. On BSD (and likely others), this will return NotFound and prevent auto-unmount from happening. Consider making the auto-unmount check OS-specific (e.g., parse the platform’s mount table) or fall back to attempting unmount/umount2 directly and treating EINVAL/ENOENT as “already unmounted”.
There was a problem hiding this comment.
Added a BSD specific version of should_auto_unmount() using getfsstat(). Unfortunately, getfsstat() doesn't seem to have safe binding in nix, so I had to use libc::getfsstat FFI binding.
| run_test( | ||
| &[Feature::DirectMount], | ||
| Unmount::Auto, | ||
| libfuse.fusermount(), | ||
| 1, | ||
| false, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
This test path is meant to validate “no fusermount dependency” for direct-mount, but it passes libfuse.fusermount() (which may allow fusermount to be present/used depending on the harness). To specifically cover the new requirement, run the auto-unmount direct-mount test with Fusermount::False so the test proves it works without fusermount available.
| fn close_inherited_fds(pipe: RawFd) { | ||
| let max_fds = getrlimit(Resource::RLIMIT_NOFILE).map_or(RawFd::MAX, |(soft, hard)| { | ||
| Ord::min(soft, hard).try_into().unwrap_or(RawFd::MAX) | ||
| }); | ||
|
|
||
| let _ = redirect_stdio(); | ||
|
|
||
| for fd in 3..=max_fds { | ||
| if fd != pipe { | ||
| let _ = close(fd); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterating and closing every fd up to RLIMIT_NOFILE can be very expensive when limits are large (common in container environments). On Linux, consider using close_range (or the nix equivalent) to close fd ranges efficiently, or at least cap the loop to a reasonable maximum when RLIMIT_NOFILE is unexpectedly huge.
There was a problem hiding this comment.
Surprisingly, close_range does essentially the same thing internally rather than something more efficient. It's also not in nix and is a relatively recent addition to glibc. I think it's best to just use the loop for maximum portability.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd307d2ebc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let root_mode = mountpoint | ||
| .metadata() | ||
| .map(|meta| meta.mode() & nix::sys::stat::SFlag::S_IFMT.bits())?; |
There was a problem hiding this comment.
Preserve mountpoint permission bits in rootmode
In the direct-mount Linux path, root_mode is masked with S_IFMT, which strips all permission bits and leaves only the file type (e.g., 040000). The kernel uses rootmode to set the permissions of the FUSE root inode, so this makes the root directory appear to have no access bits even when the mountpoint is 0755/0777, leading to unexpected EACCES on lookup/traversal for non-root users. The pure-rust path uses the full mode(); this new masking likely breaks access for direct-mount users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional. fusermount does exactly this. Mounting with pure-rust and direct-mount do result in the same permissions on the mount from my experimentations.
| pub(crate) fn umount_impl(&mut self) -> io::Result<()> { | ||
| self.do_unmount(true) | ||
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| fn do_unmount(&mut self, lazy: bool) -> io::Result<()> { | ||
| let flags = if lazy { | ||
| nix::mount::MntFlags::MNT_DETACH | ||
| } else { | ||
| nix::mount::MntFlags::empty() | ||
| }; | ||
| nix::mount::umount2(&self.mountpoint, flags)?; |
There was a problem hiding this comment.
Avoid unmounting unrelated filesystems on drop
The direct-mount umount_impl always detaches the mountpoint without verifying the mount still corresponds to this DevFuse instance. If a caller manually unmounts and then another filesystem is mounted on the same path before the Mount is dropped, this will unmount the new filesystem instead. The pure-rust path guards this with is_mounted; direct-mount lacks any check, so this race can detach unrelated mounts under legitimate use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. unmount_impl now does essentially the same thing as the pure-rust (fusermount) version.
… the mount is still mounted
I have a use case where I'm mounting in a Linux mount+user namespace (essentially a light weight container) and I don't want to depend on
fusermountbeing available.This PR adds a new feature
direct-mount, which provides a newfuser_mount_implthat directly makes the necessarymountsyscalls thatfusermountwould normally do. IfAutoUnmountis requested, similar tofusermount,direct-mountwill spawn a daemon process that waits for the mounting process to terminate before making the necessaryumount2syscall.There are two main differences from
fusermount:direct-mountdoes not attempt to modify themtabfile, which is an old behavior unnecessary on more recent systems.