Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0062de67bfe89653ffdc5e3c564fb24bc1adf3f4.camel@collabora.com>
Subject: Re: [PATCH 0/6] Initial Rust V4L2 support
From:   Daniel Almeida <daniel.almeida@collabora.com>
To:     wedsonaf@gmail.com, ojeda@kernel.org, mchehab@kernel.org,
        hverkuil@xs4all.nl
Cc:     rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, kernel@collabora.com
Date:   Sat, 08 Apr 2023 16:06:31 -0300
In-Reply-To: <20230406215615.122099-1-daniel.almeida@collabora.com>
References: <20230406215615.122099-1-daniel.almeida@collabora.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 
MIME-Version: 1.0
Precedence: bulk

By the way, one of the things I dislike about this series is that
there's a needless distinction between

struct Foo(bindgen::foo)

vs

struct FooRef(*mut bindgen::foo)

This gets in the way of having an owned Foo embedded into a larger
struct. It also gets in the way of instantiating an owned Foo on the
stack.

My first thought was to use enums:

enum Foo {
  Owned(bindgen::foo),
  NotOwned(*mut bindgen::foo),
}

But that would mean that users would invariably pay the price for the
owned variant always, as enums use as much space as its largest
variant.

My current understanding is that we can move all the implementations to
traits, with a suitable bound on AsRef<bindings::foo> and
AsMut<bindings::foo>.

Here is a code example for the wrapper of bindings::v4l2_format (see
format.rs), which was extended to account for both owned and non-owned
bindgen types:


```
use core::cell::UnsafeCell;

/// The shared implementation between Format and FormatRef.
pub trait FormatImpl: AsRef<bindings::v4l2_format> +
AsMut<bindings::v4l2_format> {
    /// Returns the `type_` field.
    fn type_(&self) -> u32 {
        self.as_ref().type_
    }

    /// Get the field `field` for the `pix` union member.
    fn pix_field(&self) -> Result<enums::Field> {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        enums::Field::try_from(pix.field)
    }

    /// Get the field `width` for the `pix` union member.
    fn pix_width(&self) -> u32 {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        pix.width
    }

    /// Get the field `height` for the `pix` union member.
    fn pix_height(&self) -> u32 {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        pix.height
    }

    /// Get the field `pixelformat` for the `pix` union member.
    fn pix_pixelformat(&self) -> u32 {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        pix.pixelformat
    }

    /// Get the field `bytesperline` for the `pix` union member.
    fn pix_bytesperline(&self) -> u32 {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        pix.bytesperline
    }

    /// Get the field `sizeimage` for the `pix` union member.
    fn pix_sizeimage(&self) -> u32 {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        pix.sizeimage
    }

    /// Get the field `colorspace` for the `pix` union member.
    fn pix_colorspace(&self) -> Result<enums::Colorspace> {
        let fmt =3D self.as_ref();
        let pix =3D &unsafe { fmt.fmt.pix };
        enums::Colorspace::try_from(pix.colorspace)
    }

    /// Set the field `field` for the `pix` union member.
    fn set_pix_field(&mut self, field: enums::Field) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.field =3D field as u32;
    }

    /// Set the field `width` for the `pix` union member.
    fn set_pix_width(&mut self, width: u32) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.width =3D width;
    }

    /// Set the field `height` for the `pix` union member.
    fn set_pix_height(&mut self, height: u32) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.height =3D height;
    }

    /// Set the field `pixelformat` for the `pix` union member.
    fn set_pix_pixel_format(&mut self, pixel_format: u32) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.pixelformat =3D pixel_format;
    }

    /// Set the field `bytesperline` for the `pix` union member.
    fn set_pix_bytesperline(&mut self, bytesperline: u32) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.bytesperline =3D bytesperline;
    }

    /// Set the field `sizeimage` for the `pix` union member.
    fn set_pix_sizeimage(&mut self, sizeimage: u32) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.sizeimage =3D sizeimage;
    }

    /// Set the field `sizeimage` for the `pix` union member.
    fn set_pix_colorspace(&mut self, colorspace: enums::Colorspace) {
        let fmt =3D self.as_mut();
        let pix =3D &mut unsafe { fmt.fmt.pix };
        pix.colorspace =3D colorspace as u32;
    }
}

/// A wrapper over a pointer to `struct v4l2_format`.
#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
pub struct FormatRef(*mut bindings::v4l2_format);

impl FormatRef {
    /// # Safety
    /// The caller must ensure that `ptr` is valid and remains valid
for the lifetime of the
    /// returned [`FormatRef`] instance.
    pub unsafe fn from_ptr(ptr: *mut bindings::v4l2_format) -> Self {
        Self(ptr)
    }
}

impl AsRef<bindings::v4l2_format> for FormatRef {
    fn as_ref(&self) -> &bindings::v4l2_format {
        // SAFETY: ptr is safe during the lifetime of [`FormatRef`] as
per
        // the safety requirement in `from_ptr()`
        unsafe { self.0.as_ref().unwrap() }
    }
}

impl AsMut<bindings::v4l2_format> for FormatRef {
    fn as_mut(&mut self) -> &mut bindings::v4l2_format {
        // SAFETY: ptr is safe during the lifetime of [`FormatRef`] as
per
        // the safety requirement in `from_ptr()`
        unsafe { self.0.as_mut().unwrap() }
    }
}

impl FormatImpl for FormatRef {}

/// An owned version of `FormatRef`.
#[derive(Default)]
pub struct Format(UnsafeCell<bindings::v4l2_format>);

impl AsRef<bindings::v4l2_format> for Format {
    fn as_ref(&self) -> &bindings::v4l2_format {
        // SAFETY:
        // It is safe to dereference the pointer since it is valid
whenever this type is instantiated.
        // It is safe to cast this into a shared reference: because
this is the
        // only method that returns &bindings::v4l2_format and because
we take
        // &self, the compiler takes care of enforcing the Rust
reference rules for
        // us. Thus, enforcing the safety guarantees of
UnsafeCell::get() by
        // proxy.
        unsafe { &*self.0.get() }
    }
}

impl AsMut<bindings::v4l2_format> for Format {
    fn as_mut(&mut self) -> &mut bindings::v4l2_format {
        // SAFETY:
        // It is safe to dereference the pointer since it is valid
whenever this type is instantiated.
        // It is safe to cast this into an exclusive reference: because
this is the
        // only method that returns &mut bindings::v4l2_format and
because we take
        // &mut self, the compiler takes care of enforcing the Rust
reference rules for
        // us. Thus, enforcing the safety guarantees of
UnsafeCell::get() by
        // proxy.
        unsafe { &mut *self.0.get() }
    }
}

impl FormatImpl for Format {}

```

This makes it possible to:

- Share the implementations between Format and FormatRef,
- Have both Format (while paying the cost of storing the
bindings::v4l2_format member) and FormatRef (while paying the cost of
storing a pointer) separately.
- Be generic over Format and FormatRef when needed, e.g.:

```
fn some_fn(format: impl FormatImpl) {...}
```=20

Thoughts?

-- Daniel