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: implement fuse-t feature #144

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Conversation

killagu
Copy link
Contributor

@killagu killagu commented Jul 17, 2023

Use fuse-t to replace MacFUSE as the implementation of FUSE. fuse-t is based on NFS and does not require the use of kernel extensions. Installation can be done with lower privileges and it does not impact system stability.

Copy link
Contributor

@bergwolf bergwolf left a comment

Choose a reason for hiding this comment

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

Now that we have a third fuse session implementation, IMHO we should define a trait for all of them. But we can do it later on.

@@ -51,6 +51,7 @@ async-io = ["async-trait", "tokio-uring", "tokio/fs", "tokio/net", "tokio/sync",
fusedev = ["vmm-sys-util", "caps", "core-foundation-sys"]
virtiofs = ["virtio-queue", "caps", "vmm-sys-util"]
vhost-user-fs = ["virtiofs", "vhost", "caps"]
fuse-t=[]
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a feature only available on macos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but features not support [target.'cfg(target_os = "macos")'.features]?

src/transport/fusedev/mod.rs Outdated Show resolved Hide resolved
@@ -53,7 +53,7 @@ mod macfuse_tests {
}

#[test]
#[ignore] // it depends on privileged mode to pass through /dev/fuse
#[cfg(feature = "fuse-t")] // it depends on privileged mode to pass through /dev/fuse
Copy link
Contributor

Choose a reason for hiding this comment

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

we use --ignore to test with privilege mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case only run macos, has no --ignore run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macfuse can not get privilege, becase it must click in System Setting.

src/transport/fusedev/mod.rs Outdated Show resolved Hide resolved
src/transport/fusedev/mod.rs Outdated Show resolved Hide resolved
src/transport/fusedev/fuse_t_session.rs Outdated Show resolved Hide resolved
const FUSE_HEADER_SIZE: usize = 0x1000;
const FS_SND_SIZE: usize = 4 * 1024 * 1024;

const FUSE_NFSSRV_PATH: &str = "/usr/local/bin/go-nfsv4";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the binary installed by default? I don't see it getting installed in the test setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Install by this script

wget https://github.com/macos-fuse-t/fuse-t/releases/download/1.0.24/fuse-t-macos-installer-1.0.24.pkg
sudo installer -pkg fuse-t-macos-installer-1.0.24.pkg -target /

src/transport/fusedev/fuse_t_session.rs Show resolved Hide resolved
@bergwolf
Copy link
Contributor

bergwolf commented Aug 4, 2023

There are a few warnings:

warning: unused import: `vmm_sys_util::tempdir::TempDir`
  --> tests/macfuse_smoke.rs:17:9
   |
17 |     use vmm_sys_util::tempdir::TempDir;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `crate::example::macfuse`
  --> tests/macfuse_smoke.rs:19:9
   |
19 |     use crate::example::macfuse;
   |         ^^^^^^^^^^^^^^^^^^^^^^^

warning: function `validate_hello_file` is never used
  --> tests/macfuse_smoke.rs:21:8
   |
21 |     fn validate_hello_file(dest: &str) -> bool {
   |        ^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: function `exec` is never used
  --> tests/macfuse_smoke.rs:38:8
   |
38 |     fn exec(cmd: &str) -> Result<String> {
   |        ^^^^

warning: `fuse-backend-rs` (test "macfuse_smoke") generated 4 warnings (run `cargo fix --test "macfuse_smoke"` to apply 2 suggestions)

Not sure why CI didn't catch it.

Otherwise lgtm!

Use fuse-t to replace MacFUSE as the implementation
of FUSE. fuse-t is based on NFS and does not require
the use of kernel extensions. Installation can be done
with lower privileges and it does not impact system stability.

Signed-off-by: killagu <[email protected]>
@killagu
Copy link
Contributor Author

killagu commented Aug 14, 2023

tests/macfuse_smoke.rs this file only run fuse-t enabled, so has warning in fusedev test.

Add feature = fuse-t works.

Copy link
Contributor

@bergwolf bergwolf left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@bergwolf bergwolf merged commit 1e5b2ac into cloud-hypervisor:master Sep 5, 2023
4 checks passed
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