[PATCH v6 05/18] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class

[Laurent Pinchart]

On Tue, Apr 02, 2024 at 08:35:26PM +0100, Kieran Bingham wrote:
> Quoting Milan Zamazal (2024-04-02 20:12:51)
> > Hi Andrei,
> > 
> > thank you for clarification.
> > 
> > Andrei Konovalov <andrey.konovalov.ynk at gmail.com> writes:
> > 
> > > Hi Milan,
> > >
> > > On 28.03.2024 12:43, Milan Zamazal wrote:
> > >> Andrei, Dennis,
> > >> could you please help me with the points below?
> > >> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > >> Thank you, Laurent, for the review and all the suggestions regarding docstring
> > >> improvements (very nice and educative!) etc.
> > >> 
> > >>>> Split the parts which doesn't otherwise depend on the type T or
> > >>>> arguments Args out of the SharedMemObject class into a new
> > >>>> SharedMem class.
> > >>>
> > >>> The commit message should explain why.
> > >>
> > >> Do you have a suggestion what to write there?
> > >
> > > This split was suggested by Laurent in his review of the v2 patch:
> > > https://lists.libcamera.org/pipermail/libcamera-devel/2024-January/040344.html
> > >
> > > My understanding was that we shouldn't create an extra copy of the same code
> > > per each type T.
> > 
> > In such a case, I don't have any idea what smart to add there, it looks saying
> > basically the thing.

How about

The SharedMemObject class template contains a fair amount of inline code
that does not depend on the template types T. To avoid duplicating it in
every template specialization, split that code to a separate base
SharedMem class.

> > >>>> +
> > >>>> +  /* Make SharedMem non-copyable for now. */
> > >>>
> > >>> Record the reason in the commit message, and drop this comment.
> > >>
> > >> The same here.
> > >
> > > This comment comes from the original code by RaspberryPi.
> > 
> > I couldn't find the reason, so I left this unchanged.
> 
> What would it mean to copy a SharedMem object? Would the copy simply be
> a lightweight copy of the SharedMem class with an increased reference?
> (like copying a shared pointer) Or does it make a clean copy of the
> underlying SharedMem?
> 
> I suspect answering that question answers the why it's non-copyable. Or
> maybe because that question isn't answered is why it's set as
> non-copyable....

:-)

We could define copy semantics, but because we haven't, the class should
be non-copyable given that it owns the mapped memory resource. The
default copy constructor would lead to use-after-free (or rather
use-after-mmap).

> > [...]
> > 
> > >>>> +  if (ftruncate(fd_.get(), size_) < 0)
> > >>>> +          return;
> > >>>
> > >>> Should we set the GROW and SHRINK seals (in a separate patch) ?
> > >
> > > Yes, this can be done.
> > > Setting F_SEAL_SHRINK and F_SEAL_GROW after the ftruncate() call above could catch
> > > some potential errors related to improper access to the shared memory allocated by
> > > the SharedMemObject.
> > 
> > Added to TODO.

-- 
Regards,

Laurent Pinchart