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

[Laurent Pinchart]

On Thu, Mar 28, 2024 at 02:08:43PM +0300, Andrei Konovalov wrote:
> 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.

I replied to this separately in the thread.

> >>> +
> >>> +	/* 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.
> 
> >>> --- /dev/null
> >>> +++ b/src/libcamera/shared_mem_object.cpp
> >>> @@ -0,0 +1,190 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2023, Raspberry Pi Ltd
> >>
> >> The documentation is Dennis and Andrei's work, I don't think this should
> >> be owned by Raspberry Pi.
> > 
> > Should I put you there as private persons or are there institutions behind you
> > that should own the copyright?
> 
> If the author of the patch is "Andrei Konovalov <andrey.konovalov.ynk at gmail.com>"
> then please put me as a private person.
> 
> If the author of the patch is "Andrey Konovalov <andrey.konovalov at linaro.org>"
> then please use "Copyright (C) <year>, Linaro Ltd"

I don't know what agreement exists between Denis, Andrey, Linaro and
Redhat, so I can't answer the question, but from a libcamera point of
view, copyright can equally belong to corporations or individuals.

> >>> +{
> >>> +	int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
> >>> +	if (fd < 0)
> >>> +		return;
> >>> +
> >>> +	fd_ = SharedFD(std::move(fd));
> >>> +	if (!fd_.isValid())
> >>> +		return;
> >>> +
> >>> +	if (ftruncate(fd_.get(), size_) < 0)
> >>> +		return;
> >>
> >> Should you clear fd_ here ? It seems pointless to keep it open, and the
> >> class would expose consistent information.
> 
> Yes, this makes sense.
> 
> >>> +
> >>
> >> 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.
> 
> >>> +	mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> >>> +		    fd_.get(), 0);
> >>> +	if (mem_ == MAP_FAILED)
> >>> +		mem_ = nullptr;
> >>
> >> Same here, should you clear fd_ ?
> 
> I think so.
> 
> > Do you have answers for these questions?

-- 
Regards,

Laurent Pinchart