feat: add Windows FUSE support using WinFSP#321
Conversation
Implements Windows filesystem mounting using winfsp-rs bindings. Changes: - Add winfsp, windows, and widestring dependencies for Windows target - Create src/mount/windows.rs with FileSystemContext implementation - Map EncryptedFs operations to WinFSP callbacks - Update conditional compilation in mount.rs for Windows - Enable Windows platform in main.rs Closes xoriors#3
There was a problem hiding this comment.
Pull request overview
This PR implements Windows filesystem mounting support using WinFSP (Windows File System Proxy), enabling Windows users to mount encrypted filesystems similar to the existing Linux FUSE support.
Changes:
- Added WinFSP integration with FileSystemContext trait implementation for Windows
- Updated platform-specific conditional compilation to include Windows alongside Linux
- Added Windows-specific dependencies (winfsp, windows crate, widestring)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mount/windows.rs | New ~762-line implementation of WinFSP filesystem adapter with path resolution, file operations, and directory listing |
| src/mount.rs | Updated conditional compilation directives to include Windows module alongside Linux |
| src/main.rs | Enabled Windows platform support by updating conditional compilation guards |
| Cargo.toml | Added Windows-specific dependencies for WinFSP integration |
| Cargo.lock | Dependency resolution including winfsp, windows, and widestring crates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut host = FileSystemHost::new(winfsp_fs) | ||
| .map_err(|_| FsError::Other("Failed to create FileSystemHost"))?; | ||
|
|
||
| let mount_point_str = self | ||
| .mountpoint | ||
| .to_str() | ||
| .ok_or_else(|| FsError::Other("Invalid mount point path"))?; | ||
|
|
||
| host.mount(mount_point_str) | ||
| .map_err(|_| FsError::Other("Failed to mount filesystem"))?; | ||
|
|
||
| host.start() | ||
| .map_err(|_| FsError::Other("Failed to start filesystem service"))?; |
There was a problem hiding this comment.
The error messages "Failed to create FileSystemHost", "Failed to mount filesystem", and "Failed to start filesystem service" discard the underlying error information from WinFSP. This makes debugging difficult for users. Consider preserving the original error details or at least logging them before converting to FsError.
| let mut host = FileSystemHost::new(winfsp_fs) | |
| .map_err(|_| FsError::Other("Failed to create FileSystemHost"))?; | |
| let mount_point_str = self | |
| .mountpoint | |
| .to_str() | |
| .ok_or_else(|| FsError::Other("Invalid mount point path"))?; | |
| host.mount(mount_point_str) | |
| .map_err(|_| FsError::Other("Failed to mount filesystem"))?; | |
| host.start() | |
| .map_err(|_| FsError::Other("Failed to start filesystem service"))?; | |
| let mut host = FileSystemHost::new(winfsp_fs).map_err(|e| { | |
| error!("Failed to create FileSystemHost: {:?}", e); | |
| FsError::Other("Failed to create FileSystemHost") | |
| })?; | |
| let mount_point_str = self | |
| .mountpoint | |
| .to_str() | |
| .ok_or_else(|| FsError::Other("Invalid mount point path"))?; | |
| host.mount(mount_point_str).map_err(|e| { | |
| error!("Failed to mount filesystem at {}: {:?}", mount_point_str, e); | |
| FsError::Other("Failed to mount filesystem") | |
| })?; | |
| host.start().map_err(|e| { | |
| error!("Failed to start filesystem service: {:?}", e); | |
| FsError::Other("Failed to start filesystem service") | |
| })?; |
| use crate::mount; | ||
| use crate::mount::{MountHandleInner, MountPoint}; | ||
|
|
||
| const TTL: Duration = Duration::from_secs(1); |
There was a problem hiding this comment.
The constant TTL is defined but never used in the implementation. Consider removing this unused constant.
| const TTL: Duration = Duration::from_secs(1); |
| let attr = if is_directory { | ||
| CreateFileAttr { | ||
| kind: FileType::Directory, | ||
| perm: 0o755, | ||
| uid: 0, | ||
| gid: 0, | ||
| rdev: 0, | ||
| flags: 0, | ||
| } | ||
| } else { | ||
| CreateFileAttr { | ||
| kind: FileType::RegularFile, | ||
| perm: 0o644, | ||
| uid: 0, | ||
| gid: 0, | ||
| rdev: 0, | ||
| flags: 0, | ||
| } |
There was a problem hiding this comment.
The uid and gid fields are hardcoded to 0 in CreateFileAttr. While Windows doesn't use Unix-style user/group IDs, setting these to 0 might not be appropriate or meaningful. Consider documenting why these values are chosen or if they have any effect on Windows.
| if let Err(e) = self | ||
| .runtime | ||
| .block_on(async { self.fs.set_attr(context.ino, set_attr).await }) | ||
| { | ||
| error!("set_basic_info error: {}", e); | ||
| } | ||
|
|
||
| self.refresh_file_info(context.ino, file_info); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The set_basic_info method logs the error from set_attr but still returns Ok(()), silently ignoring the failure. This inconsistent error handling differs from other methods like set_file_size which properly propagate errors. Consider returning an error when set_attr fails, or document why failures are acceptable here.
| fn flush(&self, context: &Self::FileContext, file_info: &mut FileInfo) -> winfsp::Result<()> { | ||
| trace!("flush: ino={}", context.ino); | ||
|
|
||
| if let Some(fh) = context.fh { | ||
| if let Err(e) = self.runtime.block_on(async { self.fs.flush(fh).await }) { | ||
| error!("Flush error: {}", e); | ||
| } | ||
| } | ||
|
|
||
| self.refresh_file_info(context.ino, file_info); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The flush method logs errors but always returns Ok(()), silently ignoring flush failures. Since flush operations are critical for data integrity, consider propagating these errors to the caller so they can be properly handled.
| fn get_fs(&self) -> Arc<EncryptedFs> { | ||
| self.fs.clone() | ||
| } | ||
|
|
There was a problem hiding this comment.
The method get_fs is defined but never used in this implementation. Consider removing this unused method.
| fn get_fs(&self) -> Arc<EncryptedFs> { | |
| self.fs.clone() | |
| } |
| STATUS_FILE_IS_A_DIRECTORY, STATUS_NOT_A_DIRECTORY, STATUS_OBJECT_NAME_COLLISION, | ||
| STATUS_OBJECT_NAME_NOT_FOUND, | ||
| }; |
There was a problem hiding this comment.
The STATUS_FILE_IS_A_DIRECTORY constant is imported but never used in the implementation. Consider removing this unused import.
| STATUS_FILE_IS_A_DIRECTORY, STATUS_NOT_A_DIRECTORY, STATUS_OBJECT_NAME_COLLISION, | |
| STATUS_OBJECT_NAME_NOT_FOUND, | |
| }; | |
| STATUS_NOT_A_DIRECTORY, STATUS_OBJECT_NAME_COLLISION, STATUS_OBJECT_NAME_NOT_FOUND, | |
| }; |
| volume_info.total_size = 1024 * 1024 * 1024 * 100; | ||
| volume_info.free_size = 1024 * 1024 * 1024 * 50; |
There was a problem hiding this comment.
The hardcoded volume sizes (100GB total, 50GB free) should be either documented or made configurable. These magic numbers make the implementation inflexible and might confuse users about actual storage capacity.
| [target.'cfg(target_os = "windows")'.dependencies] | ||
| winfsp = "0.12" | ||
| windows = { version = "0.58", features = ["Win32_Foundation", "Win32_Security", "Win32_Storage_FileSystem", "Win32_System_IO"] } | ||
| widestring = "1.1" |
There was a problem hiding this comment.
The widestring version specifier "1.1" in Cargo.toml will resolve to "1.2.1" in Cargo.lock. While this is not incorrect, consider using a more precise version specifier like "1.2" or "1.2.1" for better reproducibility.
| widestring = "1.1" | |
| widestring = "1.2.1" |
| Ok((fh, created_attr)) => { | ||
| *file_info.as_mut() = Self::attr_to_file_info(&created_attr); | ||
|
|
||
| Ok(EncryptedFsFileContext { | ||
| ino: created_attr.ino, | ||
| fh: if is_directory { None } else { Some(fh) }, |
There was a problem hiding this comment.
When creating a directory, the file handle returned from self.fs.create() is discarded. If the underlying filesystem returns a valid file handle for directories that needs to be closed, this could result in a resource leak. Consider explicitly releasing the handle for directories or verify that the underlying EncryptedFs properly handles directory handles.
- Fix flush() to propagate errors instead of silently succeeding - Fix set_basic_info() to propagate errors instead of silently succeeding - Fix read_directory() to actually add entries to dir_buffer using fill() - Fix umount() to return Unsupported error instead of Ok(()) - Fix create() to release directory file handles to prevent resource leak - Add comments for hardcoded volume sizes (100GB/50GB placeholders) - Add comments explaining uid/gid=0 choice for Windows - Update widestring dependency from 1.1 to 1.2.1
Log underlying WinFSP errors before converting to FsError, as suggested in code review. This helps users debug mount failures.
radumarias
left a comment
There was a problem hiding this comment.
PR Review: Windows FUSE Support using WinFSP
Thank you for this solid implementation! The code structure follows the Linux patterns well and includes good logging. However, I found some issues that need to be addressed before merge.
Critical Issues (Must Fix)
1. Missing File/Directory Deletion Implementation
The implementation is missing the set_delete method and proper deletion handling in cleanup(). In WinFSP, file deletion is a two-stage process:
set_deletemarks the file for deletioncleanupwithFspCleanupDeleteflag (0x01) performs actual deletion
Currently, cleanup() (line 589-595) only flushes and ignores the flags parameter.
Impact: DeleteFile() and RemoveDirectory() will appear to succeed but won't actually remove files.
Fix: Add set_delete implementation and modify cleanup to handle the deletion flag:
const FSP_CLEANUP_DELETE: u32 = 0x01;
fn cleanup(&self, context: &mut Self::FileContext, file_name: Option<&U16CStr>, flags: u32) {
trace!("cleanup: ino={}, flags={}", context.ino, flags);
if let Some(fh) = context.fh {
let _ = self.runtime.block_on(async { self.fs.flush(fh).await });
}
// Handle deletion when FspCleanupDelete flag is set
if (flags & FSP_CLEANUP_DELETE) != 0 {
if let Some(file_name) = file_name {
if let Ok((parent_ino, name)) = self.path_to_parent_and_name(file_name) {
if let Ok(secret_name) = SecretString::from_str(&name) {
let result = if context.is_directory {
self.runtime.block_on(async {
self.fs.remove_dir(parent_ino, &secret_name).await
})
} else {
self.runtime.block_on(async {
self.fs.remove_file(parent_ino, &secret_name).await
})
};
if let Err(e) = result {
error!("Failed to delete during cleanup: {}", e);
}
}
}
}
}
}Important Issues (Should Fix)
2. Silent Failures in Resource Cleanup
Multiple let _ = ... patterns silently ignore critical errors:
close()line 281: ignoresrelease()errorscreate()line 568: ignores directory handle release errorscleanup()line 593: ignoresflush()errors
For a cryptographic filesystem, silent failures could mask data loss or security issues.
Fix: Add error logging:
if let Err(e) = self.runtime.block_on(async { self.fs.release(fh).await }) {
error!("Failed to release handle {}: {}", fh, e);
}3. Incorrect Error Status for Read Failures (line 308)
All read errors return STATUS_END_OF_FILE. I/O errors and crypto failures should return appropriate status codes like STATUS_DISK_CORRUPT_ERROR or STATUS_ACCESS_DENIED.
4. Error Context Discarded
The pattern Err(_) => ... appears multiple times (lines 88, 121, 230, 248, 374), discarding error information. Consider logging before returning:
Err(e) => {
debug!("Error looking up path: {}", e);
return Err(STATUS_OBJECT_NAME_NOT_FOUND.into());
}5. Future::poll Always Returns Pending (line 764)
Unlike Linux where the mount handle properly polls, Windows returns Poll::Pending forever. Code awaiting the mount handle will block indefinitely. Either implement proper polling, return Poll::Ready(Ok(())), or document this behavior.
6. Double-Mount Panic Risk (line 714)
self.password_provider.take().unwrap() will panic if mount() is called twice. Return an error instead:
self.password_provider
.take()
.ok_or_else(|| FsError::Other("Mount already called"))?Suggestions
7. Missing Documentation
Consider adding doc comments for:
- Constants
WINDOWS_TICKandSEC_TO_UNIX_EPOCH(magic numbers for time conversion) EncryptedFsFileContextstruct fields (especially whyfhisOption)- The
path_to_inodefunction algorithm
8. Unused Parameters
Several _ prefixed parameters suggest incomplete implementations:
_patterninread_directory()- directory filtering not implemented_replace_if_existsinrename()- overwrite semantics ignored
What's Good
- Consistent logging with trace/debug/error levels
- Structure mirrors
linux.rswell - Proper
read_onlymode enforcement - Well-implemented time conversion functions
- Comments explain rationale (volume size, uid/gid)
Summary: Please address the critical deletion issue (#1) and the silent failure patterns (#2) before merge. The other issues are important but could be addressed in follow-up PRs if needed.
- Update to latest WinFSP API with VolumeParams and WideNameInfo - Improve error handling in path_to_inode and path_to_parent_and_name - Add FSP_CLEANUP_DELETE constant for proper deletion support - Simplify volume label handling using set_volume_label() - Update imports to use winfsp_sys for FILE_ACCESS_RIGHTS
Security & Privacy Review1. Argument Injection in
|
This PR implements Windows filesystem mounting support using WinFSP (Windows File System Proxy).
Summary
Changes
Testing
Closes #3