Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

["Erik Schilling" <erik.schilling@linaro.org>]

On Fri Sep 29, 2023 at 12:39 PM CEST, Viresh Kumar wrote:
> On 28-09-23, 14:27, Erik Schilling wrote:
> > On Thu Sep 28, 2023 at 1:27 PM CEST, Viresh Kumar wrote:
> > > > -    /// Get the Line info object associated with an event.
> > > > -    pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> > > > -        Info::new_internal(info, true)
> > > > +    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> > > > +        self as *const _ as *mut _
> > >
> > > What's wrong with keeping `_info` as `info` in the structure and using it
> > > directly instead of this, since this is private anyway ?
>
> Ahh, I missed that it is not *mut anymore. Shouldn't we mark it with & as well,
> since it is a reference to the gpiod structure ? Something like ? (I must admit
> that I have forgotten a lot of Rust during my long absence from work :)).
>
>     _info: &'a gpiod::gpiod_line_info,

Technically, yes. But that would require a 'a lifetime parameter on
the `Info` struct. Then, instead of using `&Info` you would need to
use `Info<'a>` everywhere.

Which then gets ugly pretty fast since you need to carry it through all
impl blocks, the `Deref` implementation on `InfoOwned` and force it onto
the consumer of the lib.

I think staying with `&Info` keeps the API a lot simpler (and this code
simpler).

>
> > We would still need to cast it the same way. One _could_ write:
> > 
> >     fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> >         &self.info as *const _ as *mut _
> >     }
>
> Can we use deref to just do this magically for us in this file somehow ?

Hm... Not exactly sure what you mean here. Do you mean a `Deref`
implementation? That one would leak this implementation detail into
public API.

>
> > But the cast dance is still required since we need a *mut, but start
> > with a readonly reference.
> > 
> > This is required since libgpiod's C lib keeps the struct internals
> > opaque and does not make guarantees about immutable datastructures for
> > any API calls.
> > 
> > Technically, the 1:1 mapping of this to Rust would be to restrict the
> > entire API to `&mut self`. One could do that - it would probably allow
> > us to advertise the structs as `Sync` - but it would require consumers
> > to declare all libgpiod-related variables as `mut`.