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

feat: introduce macos mount API support #2347

Merged
merged 12 commits into from
Apr 2, 2024

Conversation

oowl
Copy link
Contributor

@oowl oowl commented Mar 28, 2024

What does this PR do

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@oowl oowl marked this pull request as draft March 28, 2024 15:39
@oowl oowl marked this pull request as ready for review March 31, 2024 07:13
@oowl
Copy link
Contributor Author

oowl commented Mar 31, 2024

I do not know how to write the MacOS test because MacOS can not support tmpfs and nullfs, Is there any workaround for the MacOS mount test? cc @asomers @SteveLauC

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.

Hi, thanks for your interest in contributing to Nix! I have left some comments.

And:

  1. A changelog entry is needed, please take a look at doc on how to add one.
  2. There are still some #[cfg(apple_targets)] attributes in bsd.rs, I think we should remove them.

build.rs Outdated Show resolved Hide resolved
src/mount/apple.rs Outdated Show resolved Hide resolved
src/mount/apple.rs Outdated Show resolved Hide resolved
src/mount/mod.rs Outdated Show resolved Hide resolved
src/mount/mod.rs Outdated Show resolved Hide resolved
src/mount/apple.rs Outdated Show resolved Hide resolved
src/mount/apple.rs Show resolved Hide resolved
src/mount/apple.rs Outdated Show resolved Hide resolved
src/mount/apple.rs Outdated Show resolved Hide resolved
s.as_ptr(),
t.as_ptr(),
flags.bits(),
d as *mut libc::c_void,
Copy link
Member

Choose a reason for hiding this comment

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

For pointer casting, we prefer .cast() and .cast_mut() over the as keyword:

Suggested change
d as *mut libc::c_void,
d.cast().cast_mut(),

BTW, we are casting an immutable reference (&P3) to a muable pinter (*mut c_void), is this safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libc:mount() needs to recv libc:c_void type, It's hard to cast type to libc:c_void by the cast function, So I have changed it to d.cast_mut() as *mut libc::c_void,

Copy link
Member

@SteveLauC SteveLauC Apr 2, 2024

Choose a reason for hiding this comment

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

It's hard to cast type to libc:c_void by the cast function, So I have changed it to d.cast_mut() as *mut libc::c_void,

Yeah, .cast().cast_mut() should not work as the pointee type is unknown, .cast_mut().cast() should work.

BTW, we are casting an immutable reference (&P3) to a muable pinter (*mut c_void), is this safe?

Will mount(2) write to that pointer, if so, then a UB could happen:<

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/mount.2.html man page

 int
 mount(const char *type, const char *dir, int flags, void *data);

 int
 unmount(const char *dir, int flags);
   Data is a pointer to a structure that contains the type specific argu-ments arguments
   ments to mount.  The format for these argument structures is described in
   the manual page for each filesystem.

I think it's not be written, but nobody knows how Apple doing I think.

@SteveLauC
Copy link
Member

SteveLauC commented Mar 31, 2024

I do not know how to write the MacOS test because MacOS can not support tmpfs and nullfs, Is there any workaround for the MacOS mount test?

For tests, I think we can just do something simple to ensure that the binding works, e.g., using the following stupid code:

$ cat src/main.rs
use nix::{errno::Errno, mount::{mount, MntFlags}};

fn main() {
    let res = mount::<str, str, str>("", "", MntFlags::empty(), None);
    assert_eq!(res, Err(Errno::ENOENT));
}

$ cargo r -q
mounting "" to ""

$ echo $?
0

I think we should create a directory test_mount under test and move the current test_mount.rs file to linux.rs, then add a dedicated apple.rs file under test_mount.

@SteveLauC SteveLauC mentioned this pull request Apr 1, 2024
@oowl
Copy link
Contributor Author

oowl commented Apr 2, 2024

@SteveLauC I have made some changes depending on your comment, Let's continue.

src/mount/mod.rs Outdated Show resolved Hide resolved
@oowl oowl requested a review from SteveLauC April 2, 2024 06:54
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.

Thx!

@SteveLauC SteveLauC added this pull request to the merge queue Apr 2, 2024
Merged via the queue into nix-rust:master with commit bc374af Apr 2, 2024
33 checks passed
@Xuanwo
Copy link

Xuanwo commented Apr 9, 2024

Hello, @SteveLauC, is there a plan to get this PR released?

@SteveLauC
Copy link
Member

Hello, @SteveLauC, is there a plan to get this PR released?

Personally, I don't have a plan to do the next release in the near future. Do you guys need to patch? If so, can you use a git dependency?

@Xuanwo
Copy link

Xuanwo commented Apr 9, 2024

Personally, I don't have a plan to do the next release in the near future.

Is there a specific reason? I believe it's just a patch update that should have minimal impact.

Do you guys need to patch?

Yes.

If so, can you use a git dependency?

No, we can't release with git dependency.

@SteveLauC
Copy link
Member

Is there a specific reason?

Not a strong reason, I simply want to get more work done in a release.

I believe it's just a patch update that should have minimal impact.

Yeah, I agree, Nix always releases on demand, so I guess I will do a release as requested.

@Xuanwo
Copy link

Xuanwo commented Apr 9, 2024

Yeah, I agree, Nix always releases on demand, so I guess I will do a release as requested.

That will be very helpful!

@SteveLauC SteveLauC mentioned this pull request May 7, 2024
3 tasks
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.

3 participants