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

Add missing pub structs exposed in lib.rs #30

Merged
merged 5 commits into from
Aug 9, 2024

Conversation

touilleMan
Copy link
Member

No description provided.

@touilleMan touilleMan force-pushed the add-missing-pub-structs branch from cdc3ccb to b14f9ad Compare August 7, 2024 01:57
@touilleMan touilleMan force-pushed the add-missing-pub-structs branch 2 times, most recently from b6b143e to c7f0382 Compare August 7, 2024 02:59
@touilleMan touilleMan force-pushed the add-missing-pub-structs branch from c7f0382 to e73325a Compare August 7, 2024 03:21
Compilation on non-Windows platform will obviously fail (as WinFSP is only available
on Windows).
However keeping the Rust part platform agnostic is still useful given it allows
linter & IDE to work correctly when groking into the code from a non-Windows machine.
@touilleMan touilleMan force-pushed the add-missing-pub-structs branch 4 times, most recently from 4bc7cd7 to 5f79c5f Compare August 7, 2024 10:33
- Rename `FileSystemContext` to `FileSystemInterface` (better shows it is the high level equivalent of `FSP_FILE_SYSTEM_INTERFACE`)
- Expose associated const boolean in the trait to explicitly indicate which function pointer in `FSP_FILE_SYSTEM_INTERFACE` should be set
- Remove default implementations of trait methods that was somewhat usable as-is (i.e. could be passed to WinFSP as function pointer,
  but created unexpected behavior given NULL function pointer != function pointer returning `Err(STATUS_NOT_IMPLEMENTED)`)
- Fix handling of empty buffer converted to to slice (due to `std::slice::from_raw_parts_mut` requesting non-NULL buffer)
@touilleMan touilleMan force-pushed the add-missing-pub-structs branch from 5f79c5f to 3ed8240 Compare August 7, 2024 10:37

/// Get volume information.
fn get_volume_info(&self) -> Result<VolumeInfo, NTSTATUS>;
fn get_volume_info(&self) -> Result<VolumeInfo, NTSTATUS> {
unreachable!("To be used, trait method must be overwritten !");

Choose a reason for hiding this comment

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

I don't get why you provide blanket implementations, only for it to be unreachable.

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit of context

The thing to consider here is we expose a trait as a high level construct in order to build a structure of pointer (that is the thing WinFSP actully wants).

However there is no 1-to-1 relationship between trait implementation and struct of pointers: a struct of pointer can have NULL pointers which is not possible to express when implementing a trait (I've tried some tricks with function pointer comparison, but it doesn't support methods with foo: impl Type parameter !)

Hence why we ask the end user to both implement the methods he needs AND set corresponding xxx_DEFINED boolean (this way all methods with the boolean not set will be set as NULL in the struct of pointers).

The actual explanation

Providing no default implementation means the end user implementing this trait would have to implement all the methods.

However most of the time, not all methods need to be implemented (see for instance the methods related to reparse points).

I this case what should be the implementation of such method ?

The obvious answer is "just implement with a unreachable and you're good to go !". However this has two drawbacks:

  • It is much more verbose
  • It feels very weird to define a method, but with a unreachable, so this method is not really "defined" and hence the xxx_DEFINED boolean should not be set !
  • It is very tempting to implement those methods by returning a NTSTATUS STATUS_NOT_IMPLEMENTED... which cause very hard to track bugs ! (This is the current behavior of winfsp_wrs, I just had a rough week trying to pinpoint the issue 😆 )

So the alternative is set those default implementations in the trait, so this way the end user only have to defined the methods (and the corresponding xxx_DEFINED) he uses.

@touilleMan touilleMan merged commit 4a289b4 into main Aug 9, 2024
2 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