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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

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.

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

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.

-- 
viresh