Re: [RFC PATCH 1/2] rust: LED abstraction

[Alice Ryhl <aliceryhl@google.com>]

On Wed, Oct 9, 2024 at 12:58 PM Fiona Behrens <me@kloenk.dev> wrote:
> +impl<'a, T> Led<T>
> +where
> +    T: Operations + 'a,
> +{
> +    /// Register a new LED with a predefine name.
> +    pub fn register_with_name(
> +        name: &'a CStr,
> +        device: Option<&'a Device>,
> +        config: &'a LedConfig,
> +        data: T,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        try_pin_init!( Self {
> +            led <- Opaque::try_ffi_init(move |place: *mut bindings::led_classdev| {
> +            // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
> +            unsafe { place.write_bytes(0, 1) };
> +
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +            unsafe { Self::build_with_name(place, name) };
> +
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +            unsafe { Self::build_config(place, config) };
> +
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +            unsafe { Self::build_vtable(place) };
> +
> +        let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +        crate::error::to_result(unsafe {
> +            bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
> +        })
> +            }),
> +            data: data,
> +        })
> +    }
> +
> +    /// Add nameto the led_classdev.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` has to be valid.
> +    unsafe fn build_with_name(ptr: *mut bindings::led_classdev, name: &'a CStr) {
> +        // SAFETY: `ptr` is pointing to a live allocation, so the deref is safe.
> +        let name_ptr = unsafe { ptr::addr_of_mut!((*ptr).name) };
> +        // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
> +        unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
> +    }
> +
> +    /// Add config to led_classdev.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` has to be valid.
> +    unsafe fn build_config(ptr: *mut bindings::led_classdev, config: &'a LedConfig) {
> +        // SAFETY: `ptr` is pointing to a live allocation, so the deref is safe.
> +        let color_ptr = unsafe { ptr::addr_of_mut!((*ptr).color) };
> +        // SAFETY: `color_ptr` points to a valid allocation and we have exclusive access.
> +        unsafe { ptr::write(color_ptr, config.color.into()) };
> +    }
> +}

This usage of lifetimes looks incorrect to me. It looks like you are
trying to say that the references must be valid for longer than the
Led<T>, but what you are writing here does not enforce that. The Led
struct must be annotated with the 'a lifetime if you want that, but
I'm inclined to say you should not go for the lifetime solution in the
first place.

Alice