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

19762 diff mbox series

From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>

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.

Doxygen documentation by Dennis Bonke and Andrei Konovalov.

Reviewed-by: Pavel Machek <pavel@ucw.cz>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../libcamera/internal/shared_mem_object.h    | 101 ++++++----
 src/libcamera/meson.build                     |   1 +
 src/libcamera/shared_mem_object.cpp           | 190 ++++++++++++++++++
 3 files changed, 253 insertions(+), 39 deletions(-)
 create mode 100644 src/libcamera/shared_mem_object.cpp

Hi Milan and Andrey,

Thank you for the patch.

On Tue, Mar 19, 2024 at 01:35:52PM +0100, Milan Zamazal wrote:
> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
> 
> 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.

> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
> 
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../libcamera/internal/shared_mem_object.h    | 101 ++++++----
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/shared_mem_object.cpp           | 190 ++++++++++++++++++
>  3 files changed, 253 insertions(+), 39 deletions(-)
>  create mode 100644 src/libcamera/shared_mem_object.cpp
> 
> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> index 98636b44..43b07c9d 100644
> --- a/include/libcamera/internal/shared_mem_object.h
> +++ b/include/libcamera/internal/shared_mem_object.h
> @@ -6,12 +6,9 @@
>   */
>  #pragma once
>  
> -#include <fcntl.h>
>  #include <stddef.h>
>  #include <string>
>  #include <sys/mman.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
>  #include <utility>
>  
>  #include <libcamera/base/class.h>
> @@ -19,58 +16,92 @@
>  
>  namespace libcamera {
>  
> +class SharedMem
> +{
> +public:
> +	SharedMem()
> +		: mem_(nullptr)
> +	{
> +	}
> +
> +	SharedMem(const std::string &name, std::size_t size);
> +
> +	SharedMem(SharedMem &&rhs)
> +	{
> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->mem_ = rhs.mem_;
> +		rhs.mem_ = nullptr;
> +	}
> +
> +	virtual ~SharedMem()
> +	{
> +		if (mem_)
> +			munmap(mem_, size_);
> +	}

I think neither of these need to be inline. Same for operator=().

> +
> +	/* Make SharedMem non-copyable for now. */

Record the reason in the commit message, and drop this comment.

> +	LIBCAMERA_DISABLE_COPY(SharedMem)

This goes in the private: section.

> +
> +	SharedMem &operator=(SharedMem &&rhs)
> +	{
> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->mem_ = rhs.mem_;
> +		rhs.mem_ = nullptr;
> +		return *this;
> +	}
> +
> +	const SharedFD &fd() const
> +	{
> +		return fd_;
> +	}
> +
> +	void *mem() const

Make this return a Span<uint8_t>. Naked pointers lead to out-of-bound
access.

> +	{
> +		return mem_;
> +	}
> +
> +private:
> +	std::string name_;

name_ is set in the constructor and then never used. I would drop it.

> +	SharedFD fd_;
> +	size_t size_;

Missing blank line.

> +protected:
> +	void *mem_;

We put protected before private, but it seems mem_ can be private as
it's exposed by the mem() function, and classes that inherit SharedMem
shouldn't have a need to set mem_. You can then store mem_ and size_ as
a Span instead of separate members.

> +};
> +
>  template<class T>

Should we limit this to standard-layout types
(https://en.cppreference.com/w/cpp/types/is_standard_layout) ? Ideally
we should forbid usage of types that contain pointers, as they can't be
shared between processes, but there's no corresponding C++ type trait as
far as I can tell.

> -class SharedMemObject
> +class SharedMemObject : public SharedMem
>  {
>  public:
>  	static constexpr std::size_t SIZE = sizeof(T);

s/SIZE/kSize/

It is probably best done in a separate patch (before or after this one,
probably before is better).

>  
>  	SharedMemObject()
> -		: obj_(nullptr)
> +		: SharedMem(), obj_(nullptr)
>  	{
>  	}
>  
>  	template<class... Args>
>  	SharedMemObject(const std::string &name, Args &&...args)
> -		: name_(name), obj_(nullptr)
> +		: SharedMem(name, SIZE), obj_(nullptr)
>  	{
> -		void *mem;
> -		int ret;
> -
> -		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
> -		if (ret < 0)
> -			return;
> -
> -		fd_ = SharedFD(std::move(ret));
> -		if (!fd_.isValid())
> -			return;
> -
> -		ret = ftruncate(fd_.get(), SIZE);
> -		if (ret < 0)
> +		if (mem_ == nullptr)
>  			return;
>  
> -		mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
> -			   fd_.get(), 0);
> -		if (mem == MAP_FAILED)
> -			return;
> -
> -		obj_ = new (mem) T(std::forward<Args>(args)...);
> +		obj_ = new (mem_) T(std::forward<Args>(args)...);
>  	}
>  
>  	SharedMemObject(SharedMemObject<T> &&rhs)
> +		: SharedMem(std::move(rhs))
>  	{
> -		this->name_ = std::move(rhs.name_);
> -		this->fd_ = std::move(rhs.fd_);
>  		this->obj_ = rhs.obj_;
>  		rhs.obj_ = nullptr;
>  	}
>  
>  	~SharedMemObject()
>  	{
> -		if (obj_) {
> +		if (obj_)
>  			obj_->~T();
> -			munmap(obj_, SIZE);
> -		}
>  	}
>  
>  	/* Make SharedMemObject non-copyable for now. */

This also goes to the private: section.

> @@ -78,8 +109,7 @@ public:
>  
>  	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>  	{
> -		this->name_ = std::move(rhs.name_);
> -		this->fd_ = std::move(rhs.fd_);
> +		SharedMem::operator=(std::move(rhs));
>  		this->obj_ = rhs.obj_;
>  		rhs.obj_ = nullptr;
>  		return *this;
> @@ -105,19 +135,12 @@ public:
>  		return *obj_;
>  	}
>  
> -	const SharedFD &fd() const
> -	{
> -		return fd_;
> -	}
> -
>  	explicit operator bool() const

This seems to be a candidate for the base class, it doesn't depend on
the type T.

>  	{
>  		return !!obj_;
>  	}
>  
>  private:
> -	std::string name_;
> -	SharedFD fd_;
>  	T *obj_;

Do we need to store obj_, or could we always cast the 

>  };
>  
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index dd8107fa..ce31180b 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -39,6 +39,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'shared_mem_object.cpp',
>      'source_paths.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
> new file mode 100644
> index 00000000..44fe74c2
> --- /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.

> + *
> + * shared_mem_object.cpp - Helper class for shared memory allocations
> + */
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +/**
> + * \file shared_mem_object.cpp
> + * \brief Helper class for shared memory allocations

s/class/classes/, or maybe better, just "Helpers for shared memory
allocations".

> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class SharedMem
> + * \brief Helper class for allocating shared memory

 * \brief Helper class to allocate and manage memory shareable between processes

> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * SharedMem allocates the shared memory of the given size and maps it.
> + * To check that the shared memory was allocated and mapped successfully, one
> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()
> + * is not nullptr.
> + *
> + * To access the shared memory from another process the SharedFD should be passed
> + * to that process, and then the shared memory should be mapped into that process
> + * address space by calling mmap().
> + *
> + * A single memfd is created for every SharedMem. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped
> + * together and use the shared memory allocated by a single SharedMem object if
> + * possible. This will help to minimize the number of created memfd's.

memfd comes a bit out of the blue here. Let me try to improve the
documentation:

 * SharedMem manages memory suitable for sharing between processes. When an
 * instance is constructed, it allocates a memory buffer of the requested size
 * backed by an anonymous file, using the memfd API.
 *
 * The allocated memory is exposed by the mem() function. If memory allocation
 * fails, the function returns an empty Span.
 *
 * The file descriptor for the backing file is exposed as a SharedFD by the fd()
 * function. It can be shared with other processes across IPC boundaries, which
 * can then map the memory with mmap().
 *
 * A single memfd is created for every SharedMem. If there is a need to allocate
 * a large number of objects in shared memory, these objects should be grouped
 * together and use the shared memory allocated by a single SharedMem object if
 * possible. This will help to minimize the number of created memfd's.

> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
> + * \brief Constructor for the SharedMem

 * \brief Construct a SharedMem with memory of the given \a size

> + * \param[in] name Name of the SharedMem
> + * \param[in] size Size of the shared memory to allocate and map

 *
 * The \a name is used for debugging purpose only. Multiple SharedMem instances
 * can have the same name.

> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMem::~SharedMem()
> + * \brief SharedMem destructor
> + *
> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
> + * count.

/**
 * \fn SharedMem::~SharedMem()
 * \brief Destroy the SharedMem instance
 *
 * Destroying an instance invalidates the memory mapping exposed with mem().
 * Other mappings of the backing file, created in this or other processes with
 * mmap(), remain valid.
 *
 * Similarly, other references to the backing file descriptor created by copying
 * the SharedFD returned by fd() remain valid. The underlying memory will be
 * freed only when all file descriptors that reference the anonymous file get
 * closed.
 */

> + */
> +
> +/**
> + * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem

s/constructor/assignment operator/

> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn const SharedFD &SharedMem::fd() const
> + * \brief Gets the file descriptor for the underlying shared memory

s/Gets/Retrieve/

> + * \return The file descriptor

 * \return The file descriptor, or an invalid SharedFD if allocation failed

> + */
> +
> +/**
> + * \fn void *SharedMem::mem() const
> + * \brief Gets the pointer to the underlying shared memory
> + * \return The pointer to the shared memory

 * \fn Span<uint8_t> SharedMem::mem() const
 * \brief Retrieve the underlying shared memory
 * \return The memory buffer, or an empty span if allocation failed

> + */
> +
> +SharedMem::SharedMem(const std::string &name, std::size_t size)
> +	: name_(name), size_(size), mem_(nullptr)

This should come right after the corresponding documentation block, and
you can then delete the \fn line of that block.

> +{
> +	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_ ?

> +}
> +
> +/**
> + * \var SharedMem::mem_
> + * \brief Pointer to the shared memory allocated
> + */
> +
> +/**
> + * \class SharedMemObject
> + * \brief Helper class for allocating objects in shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * Given the type of the object to be created in shared memory and the arguments
> + * to pass to this object's constructor, SharedMemObject allocates the shared memory
> + * of the size of the object and constructs the object in this memory. To ensure
> + * that the SharedMemObject was created successfully, one needs to verify that the
> + * overloaded bool() operator returns true. The object created in the shared memory

The part about the bool() operator should be moved to SharedMem.

> + * can be accessed using the SharedMemObject::operator*() indirection operator. Its
> + * members can be accessed with the SharedMemObject::operator->() member of pointer
> + * operator.
> + *
> + * To access the object from another process the SharedFD should be passed to that
> + * process, and the shared memory should be mapped by calling mmap().
> + *
> + * A single memfd is created for every SharedMemObject. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped into a
> + * single large object to keep the number of created memfd's reasonably small.

 * \class SharedMemObject
 * \brief Helper class to allocate an object in shareable memory
 * \tparam The object type
 *
 * The SharedMemObject class is a specialization of the SharedMem class that
 * wraps an object of type \a T and constructs it in shareable memory. It uses
 * the same underlying memory allocation and sharing mechanism as the SharedMem
 * class.
 *
 * The wrapped object is constructed at the same time as the SharedMemObject
 * instance, by forwarding the arguments passed to the SharedMemObject
 * constructor. The underlying memory allocation is sized to the object \a T
 * size. The object can be accessed using the dereference operators operator*()
 * and operator->().
 *
 * While no restriction on the type \a T is enforced, not all types are suitable
 * for sharing between multiple processes. Most notably, any object type that
 * contains pointer or reference members will likely cause issues. Even if those
 * members refer to other members of the same object, the shared memory will be
 * mapped at different addresses in different processes, and the pointers will
 * not be valid.
 *
 * A new anonymous file is created for every SharedMemObject instance. If there
 * is a need to share a large number of small objects, these objects should be
 * grouped into a single larger object to limit the number of file descriptors.
 *
 * To share the object with other processes, see the SharedMem documentation.

> + */
> +
> +/**
> + * \var SharedMemObject::SIZE
> + * \brief The size of the object that is going to be stored here

 * \brief The size of the object stored in shared memory

> + */
> +
> +/**
> + * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)

I think you can drop < T > here (or it's missing everywhere below).

> + * \brief Constructor for the SharedMemObject

 * \brief Construct a SharedMemObject

> + * \param[in] name Name of the SharedMemObject
> + * \param[in] args Args to pass to the constructor of the object in shared memory

s/Args/Arguments/
s/in shared memory/T/

 * The \a name is used for debugging purpose only. Multiple SharedMem instances
 * can have the same name.

> + */
> +
> +/**
> + * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
> + * \brief Move constructor for SharedMemObject
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMemObject::~SharedMemObject()
> + * \brief SharedMemObject destructor
> + *
> + * Destroys the object created in the shared memory and then unmaps the shared memory.
> + * Decrements the shared memory descriptor use count.
> + */

/**
 * \fn SharedMemObject::~SharedMemObject()
 * \brief Destroy the SharedMemObject instance
 *
 * Destroying a SharedMemObject calls the wrapped T object's destructor. While
 * the underlying memory may not be freed immediately if other mappings have
 * been created manually (see SharedMem::~SharedMem() for more information), the
 * stored object may be modified. Depending on the ~T() destructor, accessing
 * the object after destruction of the SharedMemObject causes undefined
 * behaviour. It is the responsibility of the user of this class to synchronize
 * with other users who have access to the shared object.
 */

> +
> +/**
> + * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
> + * \brief Operator= for SharedMemObject

 * \brief Move assignment operator for SharedMemObject

> + * \param[in] rhs The SharedMemObject object to take the data from

 *
 * Moving a SharedMemObject does not affect in stored object.

> + */
> +
> +/**
> + * \fn SharedMemObject::operator->()
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the object

 * \brief Dereference the stored object
 * \return Pointer to the stored object

> + */
> +
> +/**
> + * \fn const T *SharedMemObject::operator->() const
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the const object

Same here. You use \copydoc to avoid duplicating the documentation,
there are a few examples in the code base. You may need to use copydoc
in the non-const version instead of here though, no sure.

> + */
> +
> +/**
> + * \fn SharedMemObject::operator*()
> + * \brief Operator* for SharedMemObject
> + * \return The reference to the object

 * \brief Dereference the stored object
 * \return Reference to the stored object

Same comment regarding copydoc.

> + */
> +
> +/**
> + * \fn const T &SharedMemObject::operator*() const
> + * \brief Operator* for SharedMemObject
> + * \return Const reference to the object
> + */
> +
> +/**
> + * \fn SharedMemObject::operator bool()
> + * \brief Operator bool() for SharedMemObject
> + * \return True if the object was created OK in the shared memory, false otherwise

Assuming you'll move this to SharedMem,

 * \brief Check if the shared memory allocation succeeded
 * \return True if allocation of the shared memorysucceeded, false otherwise

> + */
> +
> +} // namespace libcamera

/* namespace libcamera */

Andrei, Dennis,

could you please help me with the points below?

Laurent Pinchart <laurent.pinchart@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

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@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@gmail.com>"
then please put me as a private person.

If the author of the patch is "Andrey Konovalov <andrey.konovalov@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
>

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

> On Tue, Mar 19, 2024 at 01:35:52PM +0100, Milan Zamazal wrote:
>> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>>
>> +class SharedMemObject : public SharedMem
>>  {

[...]

>>  private:
>> -	std::string name_;
>> -	SharedFD fd_;
>>  	T *obj_;
>
> Do we need to store obj_, or could we always cast the 

IIRC something similar has already been suggested, with the conclusion it'd
better not to touch this now.  If there is a good idea how to improve this, it
can be done later.

Hi Andrei,

thank you for clarification.

Andrei Konovalov <andrey.konovalov.ynk@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@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.

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

[...]

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

Quoting Milan Zamazal (2024-04-02 20:12:51)
> Hi Andrei,
> 
> thank you for clarification.
> 
> Andrei Konovalov <andrey.konovalov.ynk@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@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.
> 
> >>>> +
> >>>> +  /* 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....

> [...]
> 
> >>>> +  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.
>

Hi Laurent,

the changes in this patch were not completely trivial so I'd suggest briefly
reviewing the updated version as a whole again, once it is posted.

Thanks,
Milan

Laurent Pinchart <laurent.pinchart@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@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@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.