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

[Andrei Konovalov]

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.

>>> +
>>> +	/* 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"

>>> +{
>>> +	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.

Thanks,
Andrei

> Do you have answers for these questions?
> 
> Thanks,
> Milan
>