Re: [PATCH V2 2/4] libgpiod: Add rust wrappers

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Dec 17, 2021 at 10:54 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 17-12-21, 10:43, Bartosz Golaszewski wrote:
> > On Fri, Dec 17, 2021 at 10:32 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > >
> > > On 17-12-21, 10:12, Bartosz Golaszewski wrote:
> > > > No, it's a different story altogether. In C the buffer allocates
> > > > memory for events and when you "get" an event, you only have a pointer
> > > > to the memory space in the buffer that you must not free. But you can
> > > > "copy" an event with gpiod_edge_event_copy() which returns you a deep
> > > > copy of the event that will survive the parent and that must be freed
> > > > with gpiod_edge_event_free(). This is done so that by default we try
> > > > to limit the number of allocations (as there can be a lot of events)
> > > > unless the user decides to manually copy the event.
> > > >
> > > > In C++ I used that mechanism together with the buffer's const
> > > > event_get() and event's copy assignment operator. "Getting" an event
> > > > returns a const reference to the event (still in buffer's memory) but
> > > > copying it triggers a deep copy. The memory management is of course
> > > > handled by the destructor.
> > > >
> > > > This is not used in Python as speed is no longer a concern and we'd be
> > > > creating new python objects anyway. But in Rust, I think it makes
> > > > sense to reuse this mechanism.
> > >
> > > Ahh, what about this then, it just caches all the values when the event is
> > > requested ?
> > >
> > > pub struct EdgeEvent {
> > >     event_type: LineEdgeEvent,
> > >     timestamp: Duration,
> > >     line_offset: u32,
> > >     global_seqno: u64,
> > >     line_seqno: u64,
> > > }
> > >
> >
> > Then does it make sense to make it a binding to libgpiod? :)
>
> Yes, because EdgeEvent::new() gets all these values from the internal C libgpiod
> library itself. We are going to make the same copy of data, either at the C
> library level, or here.
>
> > I don't know Rust that well to be able to come up with an idea off the
> > top of my head but I assume inheritance in one way or another exists
> > in rust? Can you have an EdgeEvent interface implemented by two
> > specialized structs, one that's stored in the buffer (stores a pointer
> > to the internal event)
>
> This is what my initial patch had, right? It isn't safe. There is no way for
> libgpiod-rust to guarantee that the users don't access the EdgeEvent after the
> buffer is freed and this needs to be made safer at libgpiod itself.
>

I'm not sure it had the same thing. In libgpiod it's simple a matter
of definition. The docs state that a pointer retrieved using
gpiod_edge_event_buffer_get_event() must not be freed while one
obtained with gpiod_edge_event_copy() does have to be disposed of. In
C++ it's safe because either you hold a const reference to the object
stored in the event buffer or you have copied it and now it will
survive the parent meaning the user can access the event even after
the buffer was deleted.

> > and one that stores the copy (and frees it when
> > it goes out of scope)?
>
> I don't understand what we will get out of such a scheme? Is there any benefit
> of getting these values via the pointer to the internal C event ?
>

You avoid allocating memory for those objects. The memory exists in
the C buffer. In order to keep a similar level of performance you'd
need to replicate the behavior of the buffer in rust as well.

Bart

> Any user that wants to use the edge-event data, needs to call the edge-event
> APIs at least once and this is what the new version does, just calls them from
> new() itself.