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

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> 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).

Thanks, added the explanations to the commit message.