Re: [RFC PATCH 2/3] rust: kernel: kobject: basic sysfs implementation

[Greg KH <gregkh@linuxfoundation.org>]

Some more review after I saw your examples:

On Sun, Dec 08, 2024 at 02:15:44PM +0100, Daniel Sedlak wrote:
> +/// Attribute represents a file in a directory.

Not always, but sometimes, be careful here :)

> +#[macros::vtable]
> +pub trait KObjectTextAttribute<K>
> +where
> +    Self: Sized,
> +{
> +    /// File permissions.
> +    const MODE: Mode = Mode::from_u16(0o777);
> +
> +    /// Name of the file in the sysfs.
> +    const NAME: &'static CStr;

No.  Well yes.   Kind of.  Why aren't you wrapping "struct attribute"
here instead?

> +
> +    /// Gets called when the read is called on the file.
> +    fn show(this: &mut K) -> Result<CString> {
> +        let _ = this;
> +        Err(EIO)
> +    }
> +
> +    /// Gets called when the write is called on the file.
> +    fn store(this: &mut K, input: &CStr) -> Result {
> +        let _ = this;
> +        let _ = input;
> +        Err(EIO)
> +    }

I see you called this "Text" attribute, but yet that's not what the
kernel calls them in the C side.  Again, let's not create multiple names
for the same thing as that's going to cause nothing but headaches for
decades as most of us flit from both C and Rust constantly.

Naming matters, be consistent.

Also, attributes in the kernel do NOT have show/store functions by
default.  The "wrapper" attribute type has them.  So again, be very
careful here.

sysfs/kobjects are tricky and slippery and take advantage of some crazy
things in C (i.e. object structure layouts and looney casts "just
because we know what we are doing".)  Representing this in rust is also
going to have those same types of issues, so watch out.  You simplified
it all here, but note that the C side is complex for real reasons (i.e.
we started out simple, and then got complex to solve real problems.)  Be
aware of our history here first please and don't make the same mistakes
we did in our youth, you have the benifit of learning from how foolish
we were back then :)

> +}
> +
> +#[doc(hidden)]
> +struct TextAttributeVTable<K, A: KObjectTextAttribute<K>>(PhantomData<(A, K)>);
> +#[doc(hidden)]
> +impl<K, A: KObjectTextAttribute<K>> TextAttributeVTable<K, A> {
> +    #[doc(hidden)]
> +    const fn new() -> Self {
> +        Self(PhantomData)
> +    }
> +    unsafe extern "C" fn show_callback(
> +        kobj: *mut bindings::kobject,
> +        _attr: *mut bindings::kobj_attribute,
> +        buf: *mut core::ffi::c_char,
> +    ) -> isize {
> +        match A::show(
> +            &mut unsafe { &mut *container_of!(kobj, KObject<K>, pointer).cast_mut() }.data,
> +        ) {
> +            Ok(cstring) => unsafe {
> +                bindings::sized_strscpy(buf.cast(), cstring.as_char_ptr(), PAGE_SIZE)
> +            },
> +            Err(error) => error.to_errno() as isize,
> +        }
> +    }

You should ALWAYS call sysfs_emit() when printing out data (for a
variety of reasons).  Here you are hard-coding the PAGE_SIZE reference
of the buffer that the kobject core gave you, are you SURE that is
always going to work?  Please just use the apis the C code gives you and
don't attempt to re-write them (again, learn from our mistakes of the
past...

thanks,

greg k-h