Conversation
|
Could we separate the platform specific code? For example, src/platform/linux.rs and src/platform/macos.rs behind a src/platform/mod.rs that re-exports the right impl via #[cfg]. Move into it:
|
congwang-mk
left a comment
congwang-mk
left a comment
There was a problem hiding this comment.
Thanks for your update. Some additional reviews
| let options = vec![ | ||
| let mut options = vec![ | ||
| MountOption::FSName("branchfs".to_string()), | ||
| MountOption::DefaultPermissions, |
There was a problem hiding this comment.
Is this also removed for Linux?
There was a problem hiding this comment.
I removed it here because we refactored all mount options out into the new platform/ module to separate the OS-specific logic. I've restored MountOption::DefaultPermissions explicitly for Linux inside
src/platform/linux.rs
get_mount_options()
function, while successfully keeping it disabled for macOS. Let me know if you have any concerns.
src/fs_helpers.rs
Outdated
| crtime: UNIX_EPOCH, | ||
| kind: FileType::RegularFile, | ||
| perm: 0o600, | ||
| perm: 0o777, |
There was a problem hiding this comment.
Is this perm change expected?
There was a problem hiding this comment.
Ah, no, that was an accident! I temporarily raised the permissions to 0o777 during some deep debugging of EPERM failures specifically on macOS. Now that the true root cause is solved, this should definitely be 0o600. I have reverted it back. Good catch!
| // Report a non-zero size so the kernel issues read() calls. | ||
| // The actual content length is determined by the read handler. | ||
| let size = if ino == CTL_INO { 256 } else { 0 }; | ||
| let size = 0; |
There was a problem hiding this comment.
Yes, it is intentional. When testing on mac, I discovered that if the control file reports a size > 0 (e.g. 256), writing to it causes the macOS kernel to issue a Read-Modify-Write cycle. The kernel would internally read the dummy contents, blend it with our command, and then write a corrupted string (e.g. \0\0\0commit\0\0...) which broke our command parsing. Setting the size to exactly 0 forces the kernel to send the raw write payload exactly as intended.
tests/test_integration.rs
Outdated
| let mut buf = [0u8; 128]; | ||
| let ret = libc::ioctl(fd, FS_IOC_BRANCH_CREATE as libc::c_ulong, buf.as_mut_ptr()); | ||
| if ret < 0 { | ||
| #[cfg(any(target_os = "macos", target_os = "ios", target_os = "watchos", target_os = "tvos", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd", target_os = "netbsd"))] |
There was a problem hiding this comment.
Good catch! That was a copy-pasted polyfill for parsing errno across all Apple and BSD platforms. Since branchfs only officially targets linux and macos, I've stripped out the rest of those OS targets as dead code to make it much cleaner.
tests/test_integration.rs
Outdated
| if ret < 0 { | ||
| #[cfg(any(target_os = "macos", target_os = "ios", target_os = "watchos", target_os = "tvos", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd", target_os = "netbsd"))] | ||
| return Err(unsafe { *libc::__error() }); | ||
| #[cfg(not(any(target_os = "macos", target_os = "ios", target_os = "watchos", target_os = "tvos", target_os = "freebsd", target_os = "dragonfly", target_os = "openbsd", target_os = "netbsd")))] |
src/platform/mod.rs
Outdated
| #[cfg(target_os = "linux")] | ||
| pub use linux::*; | ||
|
|
||
| #[cfg(not(target_os = "linux"))] |
There was a problem hiding this comment.
Great point. Relying on not(linux) was too broad and would break if anyone ever tried to build this on Windows or FreeBSD. I've flipped both of those to explicitly use #[cfg(target_os = "macos")] to strictly isolate the Apple-specific module. Thanks for catching that.
src/platform/mod.rs
Outdated
|
|
||
| #[cfg(not(target_os = "linux"))] | ||
| mod macos; | ||
| #[cfg(not(target_os = "linux"))] |
2 participants
Description
Port branchfs to macOS with verified integration tests and stable control interface
Testing
All existing tests pass