rust: add support for file system parameters #827
Conversation
rust/kernel/fs.rs
Outdated
| // context is valid, `from_pointer` wasn't called yet, so `ptr` is valid. Additionally, | ||
| // the callback contract guarantees that callbacks are serialised, so it is ok to | ||
| // mutably reference it. | ||
| let mut data = unsafe { borrow_mut::<<T::Context as Context<T>>::Data>(ptr) }; |
There was a problem hiding this comment.
To fulfill the safety guarantee, we also need to make sure &data won't be Copy to another thread, right? Because data is Sync, allowing copy &data breaks the safety guarantee:
no other concurrent users of the pointer run at least until the returned
ScopeGuardis dropped.
There was a problem hiding this comment.
I think it's better if we have a PointerWrapper::borrow_mut
There was a problem hiding this comment.
To fulfill the safety guarantee, we also need to make sure
&datawon't beCopyto another thread, right? BecausedataisSync, allowing copy&databreaks the safety guarantee:no other concurrent users of the pointer run at least until the returned
ScopeGuardis dropped.
It's ok to have &data in other threads as long as it is derived from this data (e.g., we call a function that takes a &data as argument and sends it out to other threads but guarantees that they are done before returning). What needs to be avoided is &data or &mut data acquired through other means.
I think it's better if we have a
PointerWrapper::borrow_mut
I wanted to avoid having each implementation replicate the same functionality, but thinking about it, we can just provide a default implementation and have everyone use it. Will do.
There was a problem hiding this comment.
(BTW, I expanded the comment to include the fact that we can have &data derived from data.)
This allows file system contexts to be further initialised with parameters from userspace before a fs is mounted or reconfigured. Signed-off-by: Wedson Almeida Filho <[email protected]>
| @@ -44,6 +76,55 @@ impl<T: Type + ?Sized> Tables<T> { | |||
| } | |||
| } | |||
|
|
|||
| unsafe extern "C" fn parse_param_callback( | |||
There was a problem hiding this comment.
Doesn't all 'unsafe' comments require a '//SAFETY' comment ( #351 ) ?
Although the following unsafe code it is indeed commented.
There was a problem hiding this comment.
Note that this is an unsafe block, but an unsafe fn. For the former, you indeed need to describe why the operations inside the block are sound with a // SAFETY: comment. For the latter, we use /// # Safety documentation sections to describe the safety preconditions, if public.
It may be a good idea to start requiring documentation for private unsafe functions later on too, though.
There was a problem hiding this comment.
As Miguel explained, we normally require /// # Safety blocks for these, which is slightly different: we're telling callers what they need to do to use it safely. I do write these sections even for private functions (see for example unnregister_keys in this very same file), but this case is a callback from C: there are no Rust callers, so no one is going to write a // SAFETY block based on this function.
None of the C callbacks currently have /// # Safety sections because of this.
This allows file system contexts to be further initialised with
parameters from userspace before a fs is mounted or reconfigured.
Signed-off-by: Wedson Almeida Filho [email protected]