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

[Fiona Behrens <me@kloenk.dev>]

On 18 Nov 2024, at 11:22, Alice Ryhl wrote:

> 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.

The `led_classdev_register_ext` function copies the name, therefore the idea was that the name only has to exists until the pin init function is called, which should be the case with how I used the lifetimes here

Fiona

>
> Alice