On Wed, 18 Oct 2023 at 12:38, Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
> On 18.10.23 14:25, Wedson Almeida Filho wrote:
> > +/// A registration of a file system.
> > +#[pin_data(PinnedDrop)]
> > +pub struct Registration {
> > + #[pin]
> > + fs: Opaque<bindings::file_system_type>,
> > + #[pin]
> > + _pin: PhantomPinned,
>
> Note that since commit 0b4e3b6f6b79 ("rust: types: make `Opaque` be
> `!Unpin`") you do not need an extra pinned `PhantomPinned` in your struct
> (if you already have a pinned `Opaque`), since `Opaque` already is
> `!Unpin`.
Will remove in v2.
> > +impl Registration {
> > + /// Creates the initialiser of a new file system registration.
> > + pub fn new<T: FileSystem + ?Sized>(module: &'static ThisModule) -> impl PinInit<Self, Error> {
>
> I am a bit curious why you specify `?Sized` here, is it common
> for types that implement `FileSystem` to not be `Sized`?
>
> Or do you want to use `dyn FileSystem`?
No reason beyond `Sized` being a restriction I don't need.
For something I was doing early on in binder, I ended up having to
change a bunch of generic type decls to allow !Sized, so here I'm
doing it preemptively as I don't lose anything.
> > + try_pin_init!(Self {
> > + _pin: PhantomPinned,
> > + fs <- Opaque::try_ffi_init(|fs_ptr: *mut bindings::file_system_type| {
> > + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write.
> > + unsafe { fs_ptr.write(bindings::file_system_type::default()) };
> > +
> > + // SAFETY: `try_ffi_init` guarantees that `fs_ptr` is valid for write, and it has
> > + // just been initialised above, so it's also valid for read.
> > + let fs = unsafe { &mut *fs_ptr };
> > + fs.owner = module.0;
> > + fs.name = T::NAME.as_char_ptr();
> > + fs.init_fs_context = Some(Self::init_fs_context_callback);
> > + fs.kill_sb = Some(Self::kill_sb_callback);
> > + fs.fs_flags = 0;
> > +
> > + // SAFETY: Pointers stored in `fs` are static so will live for as long as the
> > + // registration is active (it is undone in `drop`).
> > + to_result(unsafe { bindings::register_filesystem(fs_ptr) })
> > + }),
> > + })
> > + }
> > +
> > + unsafe extern "C" fn init_fs_context_callback(
> > + _fc_ptr: *mut bindings::fs_context,
> > + ) -> core::ffi::c_int {
> > + from_result(|| Err(ENOTSUPP))
> > + }
> > +
> > + unsafe extern "C" fn kill_sb_callback(_sb_ptr: *mut bindings::super_block) {}
> > +}
> > +
> > +#[pinned_drop]
> > +impl PinnedDrop for Registration {
> > + fn drop(self: Pin<&mut Self>) {
> > + // SAFETY: If an instance of `Self` has been successfully created, a call to
> > + // `register_filesystem` has necessarily succeeded. So it's ok to call
> > + // `unregister_filesystem` on the previously registered fs.
>
> I would simply add an invariant on `Registration` that `self.fs` is
> registered, then you do not need such a lengthy explanation here.
Since this is the only place I need this explanation, I prefer to
leave it here because it's exactly where I need it.
Thanks,
-Wedson
