Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, May 02, 2025 at 10:12:15AM +0200, Benno Lossin wrote:
> > +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> > +    #[cfg(CONFIG_DEBUG_FS)]
> > +    fn create(name: &CStr, parent: Option<&Self>) -> Self {
> > +        let parent_ptr = match parent {
> > +            Some(parent) => parent.as_ptr(),
> > +            None => core::ptr::null_mut(),
> > +        };
> > +        // SAFETY:
> > +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> > +        //   invariants of `&CStr`.
> > +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> > +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> > +        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> > +        //   so we can call `Self::from_ptr`.
> > +        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
> 
> What about when an error got returned? Should that be exposed to the
> user?

No, not at all.  See my comments on version 1 of this patchset.  No
error should ever go back to the caller, it should never know if a
debugfs call succeeded or not so that it can just keep moving forward
and not act any differently.

Many of the C debugfs apis are already changed to be this way, let's not
go backwards and add this logic to the rust code only to rip it out in
the future.

> > +    }
> > +
> > +    #[cfg(not(CONFIG_DEBUG_FS))]
> > +    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
> > +        Self()
> > +    }
> > +
> 
> > +impl Drop for Dir {
> > +    fn drop(&mut self) {
> > +        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
> > +        // `as_ptr` guarantees that the pointer is of this form.
> > +        #[cfg(CONFIG_DEBUG_FS)]
> > +        unsafe {
> 
> I feel a bit uneasy with seeing `cfg` on `unsafe` code, since now the
> correctness also depends on the configuration. Someone might add/modify
> it making it incorrect under certain configurations.

The option is either enabled or not, this should be fine.

> This case is pretty straight forward, but I'm not so sure if we already
> have such a case.
> 
> How about having two modules providing the two implementations and then
> just conditionally import one or the other?

That would require a lot more duplicated code that you then have to
always keep in sync.  And from past experience, that's hard to do over
time.  So let's do it this way if at all possible.

thanks,

greg k-h