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

[Milan Zamazal]

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?

>> +
>> +	/* Make SharedMem non-copyable for now. */
>
> Record the reason in the commit message, and drop this comment.

The same here.

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

>> +{
>> +	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.
>
>> +
>
> Should we set the GROW and SHRINK seals (in a separate patch) ?
>
>> +	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_ ?

Do you have answers for these questions?

Thanks,
Milan