[{"id":29080,"web_url":"https://patchwork.libcamera.org/comment/29080/","msgid":"<20240327125701.GD8623@pendragon.ideasonboard.com>","date":"2024-03-27T12:57:01","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan and Andrey,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:35:52PM +0100, Milan Zamazal wrote:\n> From: Andrei Konovalov \n> \n> Split the parts which doesn't otherwise depend on the type T or\n> arguments Args out of the SharedMemObject class into a new\n> SharedMem class.\n\nThe commit message should explain why.\n\n> Doxygen documentation by Dennis Bonke and Andrei Konovalov.\n> \n> Reviewed-by: Pavel Machek \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Signed-off-by: Andrei Konovalov \n> Signed-off-by: Hans de Goede \n> Reviewed-by: Milan Zamazal \n> ---\n> .../libcamera/internal/shared_mem_object.h | 101 ++++++----\n> src/libcamera/meson.build | 1 +\n> src/libcamera/shared_mem_object.cpp | 190 ++++++++++++++++++\n> 3 files changed, 253 insertions(+), 39 deletions(-)\n> create mode 100644 src/libcamera/shared_mem_object.cpp\n> \n> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> index 98636b44..43b07c9d 100644\n> --- a/include/libcamera/internal/shared_mem_object.h\n> +++ b/include/libcamera/internal/shared_mem_object.h\n> @@ -6,12 +6,9 @@\n> */\n> #pragma once\n> \n> -#include \n> #include \n> #include \n> #include \n> -#include \n> -#include \n> #include \n> \n> #include \n> @@ -19,58 +16,92 @@\n> \n> namespace libcamera {\n> \n> +class SharedMem\n> +{\n> +public:\n> +\tSharedMem()\n> +\t\t: mem_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\tSharedMem(const std::string &name, std::size_t size);\n> +\n> +\tSharedMem(SharedMem &&rhs)\n> +\t{\n> +\t\tthis->name_ = std::move(rhs.name_);\n> +\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tthis->mem_ = rhs.mem_;\n> +\t\trhs.mem_ = nullptr;\n> +\t}\n> +\n> +\tvirtual ~SharedMem()\n> +\t{\n> +\t\tif (mem_)\n> +\t\t\tmunmap(mem_, size_);\n> +\t}\n\nI think neither of these need to be inline. Same for operator=().\n\n> +\n> +\t/* Make SharedMem non-copyable for now. */\n\nRecord the reason in the commit message, and drop this comment.\n\n> +\tLIBCAMERA_DISABLE_COPY(SharedMem)\n\nThis goes in the private: section.\n\n> +\n> +\tSharedMem &operator=(SharedMem &&rhs)\n> +\t{\n> +\t\tthis->name_ = std::move(rhs.name_);\n> +\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tthis->mem_ = rhs.mem_;\n> +\t\trhs.mem_ = nullptr;\n> +\t\treturn *this;\n> +\t}\n> +\n> +\tconst SharedFD &fd() const\n> +\t{\n> +\t\treturn fd_;\n> +\t}\n> +\n> +\tvoid *mem() const\n\nMake this return a Span. Naked pointers lead to out-of-bound\naccess.\n\n> +\t{\n> +\t\treturn mem_;\n> +\t}\n> +\n> +private:\n> +\tstd::string name_;\n\nname_ is set in the constructor and then never used. I would drop it.\n\n> +\tSharedFD fd_;\n> +\tsize_t size_;\n\nMissing blank line.\n\n> +protected:\n> +\tvoid *mem_;\n\nWe put protected before private, but it seems mem_ can be private as\nit's exposed by the mem() function, and classes that inherit SharedMem\nshouldn't have a need to set mem_. You can then store mem_ and size_ as\na Span instead of separate members.\n\n> +};\n> +\n> template\n\nShould we limit this to standard-layout types\n(https://en.cppreference.com/w/cpp/types/is_standard_layout) ? Ideally\nwe should forbid usage of types that contain pointers, as they can't be\nshared between processes, but there's no corresponding C++ type trait as\nfar as I can tell.\n\n> -class SharedMemObject\n> +class SharedMemObject : public SharedMem\n> {\n> public:\n> \tstatic constexpr std::size_t SIZE = sizeof(T);\n\ns/SIZE/kSize/\n\nIt is probably best done in a separate patch (before or after this one,\nprobably before is better).\n\n> \n> \tSharedMemObject()\n> -\t\t: obj_(nullptr)\n> +\t\t: SharedMem(), obj_(nullptr)\n> \t{\n> \t}\n> \n> \ttemplate\n> \tSharedMemObject(const std::string &name, Args &&...args)\n> -\t\t: name_(name), obj_(nullptr)\n> +\t\t: SharedMem(name, SIZE), obj_(nullptr)\n> \t{\n> -\t\tvoid *mem;\n> -\t\tint ret;\n> -\n> -\t\tret = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> -\t\tif (ret < 0)\n> -\t\t\treturn;\n> -\n> -\t\tfd_ = SharedFD(std::move(ret));\n> -\t\tif (!fd_.isValid())\n> -\t\t\treturn;\n> -\n> -\t\tret = ftruncate(fd_.get(), SIZE);\n> -\t\tif (ret < 0)\n> +\t\tif (mem_ == nullptr)\n> \t\t\treturn;\n> \n> -\t\tmem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,\n> -\t\t\t fd_.get(), 0);\n> -\t\tif (mem == MAP_FAILED)\n> -\t\t\treturn;\n> -\n> -\t\tobj_ = new (mem) T(std::forward(args)...);\n> +\t\tobj_ = new (mem_) T(std::forward(args)...);\n> \t}\n> \n> \tSharedMemObject(SharedMemObject &&rhs)\n> +\t\t: SharedMem(std::move(rhs))\n> \t{\n> -\t\tthis->name_ = std::move(rhs.name_);\n> -\t\tthis->fd_ = std::move(rhs.fd_);\n> \t\tthis->obj_ = rhs.obj_;\n> \t\trhs.obj_ = nullptr;\n> \t}\n> \n> \t~SharedMemObject()\n> \t{\n> -\t\tif (obj_) {\n> +\t\tif (obj_)\n> \t\t\tobj_->~T();\n> -\t\t\tmunmap(obj_, SIZE);\n> -\t\t}\n> \t}\n> \n> \t/* Make SharedMemObject non-copyable for now. */\n\nThis also goes to the private: section.\n\n> @@ -78,8 +109,7 @@ public:\n> \n> \tSharedMemObject &operator=(SharedMemObject &&rhs)\n> \t{\n> -\t\tthis->name_ = std::move(rhs.name_);\n> -\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tSharedMem::operator=(std::move(rhs));\n> \t\tthis->obj_ = rhs.obj_;\n> \t\trhs.obj_ = nullptr;\n> \t\treturn *this;\n> @@ -105,19 +135,12 @@ public:\n> \t\treturn *obj_;\n> \t}\n> \n> -\tconst SharedFD &fd() const\n> -\t{\n> -\t\treturn fd_;\n> -\t}\n> -\n> \texplicit operator bool() const\n\nThis seems to be a candidate for the base class, it doesn't depend on\nthe type T.\n\n> \t{\n> \t\treturn !!obj_;\n> \t}\n> \n> private:\n> -\tstd::string name_;\n> -\tSharedFD fd_;\n> \tT *obj_;\n\nDo we need to store obj_, or could we always cast the \n\n> };\n> \n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index dd8107fa..ce31180b 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -39,6 +39,7 @@ libcamera_sources = files([\n> 'process.cpp',\n> 'pub_key.cpp',\n> 'request.cpp',\n> + 'shared_mem_object.cpp',\n> 'source_paths.cpp',\n> 'stream.cpp',\n> 'sysfs.cpp',\n> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp\n> new file mode 100644\n> index 00000000..44fe74c2\n> --- /dev/null\n> +++ b/src/libcamera/shared_mem_object.cpp\n> @@ -0,0 +1,190 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n\nThe documentation is Dennis and Andrei's work, I don't think this should\nbe owned by Raspberry Pi.\n\n> + *\n> + * shared_mem_object.cpp - Helper class for shared memory allocations\n> + */\n> +\n> +#include \"libcamera/internal/shared_mem_object.h\"\n> +\n> +#include \n> +#include \n> +\n> +/**\n> + * \\file shared_mem_object.cpp\n> + * \\brief Helper class for shared memory allocations\n\ns/class/classes/, or maybe better, just \"Helpers for shared memory\nallocations\".\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class SharedMem\n> + * \\brief Helper class for allocating shared memory\n\n * \\brief Helper class to allocate and manage memory shareable between processes\n\n> + *\n> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.\n> + *\n> + * SharedMem allocates the shared memory of the given size and maps it.\n> + * To check that the shared memory was allocated and mapped successfully, one\n> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()\n> + * is not nullptr.\n> + *\n> + * To access the shared memory from another process the SharedFD should be passed\n> + * to that process, and then the shared memory should be mapped into that process\n> + * address space by calling mmap().\n> + *\n> + * A single memfd is created for every SharedMem. If there is a need to allocate\n> + * a large number of objects in shared memory, these objects should be grouped\n> + * together and use the shared memory allocated by a single SharedMem object if\n> + * possible. This will help to minimize the number of created memfd's.\n\nmemfd comes a bit out of the blue here. Let me try to improve the\ndocumentation:\n\n * SharedMem manages memory suitable for sharing between processes. When an\n * instance is constructed, it allocates a memory buffer of the requested size\n * backed by an anonymous file, using the memfd API.\n *\n * The allocated memory is exposed by the mem() function. If memory allocation\n * fails, the function returns an empty Span.\n *\n * The file descriptor for the backing file is exposed as a SharedFD by the fd()\n * function. It can be shared with other processes across IPC boundaries, which\n * can then map the memory with mmap().\n *\n * A single memfd is created for every SharedMem. If there is a need to allocate\n * a large number of objects in shared memory, these objects should be grouped\n * together and use the shared memory allocated by a single SharedMem object if\n * possible. This will help to minimize the number of created memfd's.\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMem::SharedMem(const std::string &name, std::size_t size)\n> + * \\brief Constructor for the SharedMem\n\n * \\brief Construct a SharedMem with memory of the given \\a size\n\n> + * \\param[in] name Name of the SharedMem\n> + * \\param[in] size Size of the shared memory to allocate and map\n\n *\n * The \\a name is used for debugging purpose only. Multiple SharedMem instances\n * can have the same name.\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMem::SharedMem(SharedMem &&rhs)\n> + * \\brief Move constructor for SharedMem\n> + * \\param[in] rhs The object to move\n> + */\n> +\n> +/**\n> + * \\fn SharedMem::~SharedMem()\n> + * \\brief SharedMem destructor\n> + *\n> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use\n> + * count.\n\n/**\n * \\fn SharedMem::~SharedMem()\n * \\brief Destroy the SharedMem instance\n *\n * Destroying an instance invalidates the memory mapping exposed with mem().\n * Other mappings of the backing file, created in this or other processes with\n * mmap(), remain valid.\n *\n * Similarly, other references to the backing file descriptor created by copying\n * the SharedFD returned by fd() remain valid. The underlying memory will be\n * freed only when all file descriptors that reference the anonymous file get\n * closed.\n */\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMem &SharedMem::operator=(SharedMem &&rhs)\n> + * \\brief Move constructor for SharedMem\n\ns/constructor/assignment operator/\n\n> + * \\param[in] rhs The object to move\n> + */\n> +\n> +/**\n> + * \\fn const SharedFD &SharedMem::fd() const\n> + * \\brief Gets the file descriptor for the underlying shared memory\n\ns/Gets/Retrieve/\n\n> + * \\return The file descriptor\n\n * \\return The file descriptor, or an invalid SharedFD if allocation failed\n\n> + */\n> +\n> +/**\n> + * \\fn void *SharedMem::mem() const\n> + * \\brief Gets the pointer to the underlying shared memory\n> + * \\return The pointer to the shared memory\n\n * \\fn Span SharedMem::mem() const\n * \\brief Retrieve the underlying shared memory\n * \\return The memory buffer, or an empty span if allocation failed\n\n> + */\n> +\n> +SharedMem::SharedMem(const std::string &name, std::size_t size)\n> +\t: name_(name), size_(size), mem_(nullptr)\n\nThis should come right after the corresponding documentation block, and\nyou can then delete the \\fn line of that block.\n\n> +{\n> +\tint fd = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> +\tif (fd < 0)\n> +\t\treturn;\n> +\n> +\tfd_ = SharedFD(std::move(fd));\n> +\tif (!fd_.isValid())\n> +\t\treturn;\n> +\n> +\tif (ftruncate(fd_.get(), size_) < 0)\n> +\t\treturn;\n\nShould you clear fd_ here ? It seems pointless to keep it open, and the\nclass would expose consistent information.\n\n> +\n\nShould we set the GROW and SHRINK seals (in a separate patch) ?\n\n> +\tmem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,\n> +\t\t fd_.get(), 0);\n> +\tif (mem_ == MAP_FAILED)\n> +\t\tmem_ = nullptr;\n\nSame here, should you clear fd_ ?\n\n> +}\n> +\n> +/**\n> + * \\var SharedMem::mem_\n> + * \\brief Pointer to the shared memory allocated\n> + */\n> +\n> +/**\n> + * \\class SharedMemObject\n> + * \\brief Helper class for allocating objects in shared memory\n> + *\n> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.\n> + *\n> + * Given the type of the object to be created in shared memory and the arguments\n> + * to pass to this object's constructor, SharedMemObject allocates the shared memory\n> + * of the size of the object and constructs the object in this memory. To ensure\n> + * that the SharedMemObject was created successfully, one needs to verify that the\n> + * overloaded bool() operator returns true. The object created in the shared memory\n\nThe part about the bool() operator should be moved to SharedMem.\n\n> + * can be accessed using the SharedMemObject::operator*() indirection operator. Its\n> + * members can be accessed with the SharedMemObject::operator->() member of pointer\n> + * operator.\n> + *\n> + * To access the object from another process the SharedFD should be passed to that\n> + * process, and the shared memory should be mapped by calling mmap().\n> + *\n> + * A single memfd is created for every SharedMemObject. If there is a need to allocate\n> + * a large number of objects in shared memory, these objects should be grouped into a\n> + * single large object to keep the number of created memfd's reasonably small.\n\n * \\class SharedMemObject\n * \\brief Helper class to allocate an object in shareable memory\n * \\tparam The object type\n *\n * The SharedMemObject class is a specialization of the SharedMem class that\n * wraps an object of type \\a T and constructs it in shareable memory. It uses\n * the same underlying memory allocation and sharing mechanism as the SharedMem\n * class.\n *\n * The wrapped object is constructed at the same time as the SharedMemObject\n * instance, by forwarding the arguments passed to the SharedMemObject\n * constructor. The underlying memory allocation is sized to the object \\a T\n * size. The object can be accessed using the dereference operators operator*()\n * and operator->().\n *\n * While no restriction on the type \\a T is enforced, not all types are suitable\n * for sharing between multiple processes. Most notably, any object type that\n * contains pointer or reference members will likely cause issues. Even if those\n * members refer to other members of the same object, the shared memory will be\n * mapped at different addresses in different processes, and the pointers will\n * not be valid.\n *\n * A new anonymous file is created for every SharedMemObject instance. If there\n * is a need to share a large number of small objects, these objects should be\n * grouped into a single larger object to limit the number of file descriptors.\n *\n * To share the object with other processes, see the SharedMem documentation.\n\n> + */\n> +\n> +/**\n> + * \\var SharedMemObject::SIZE\n> + * \\brief The size of the object that is going to be stored here\n\n * \\brief The size of the object stored in shared memory\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)\n\nI think you can drop < T > here (or it's missing everywhere below).\n\n> + * \\brief Constructor for the SharedMemObject\n\n * \\brief Construct a SharedMemObject\n\n> + * \\param[in] name Name of the SharedMemObject\n> + * \\param[in] args Args to pass to the constructor of the object in shared memory\n\ns/Args/Arguments/\ns/in shared memory/T/\n\n * The \\a name is used for debugging purpose only. Multiple SharedMem instances\n * can have the same name.\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::SharedMemObject(SharedMemObject &&rhs)\n> + * \\brief Move constructor for SharedMemObject\n> + * \\param[in] rhs The object to move\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::~SharedMemObject()\n> + * \\brief SharedMemObject destructor\n> + *\n> + * Destroys the object created in the shared memory and then unmaps the shared memory.\n> + * Decrements the shared memory descriptor use count.\n> + */\n\n/**\n * \\fn SharedMemObject::~SharedMemObject()\n * \\brief Destroy the SharedMemObject instance\n *\n * Destroying a SharedMemObject calls the wrapped T object's destructor. While\n * the underlying memory may not be freed immediately if other mappings have\n * been created manually (see SharedMem::~SharedMem() for more information), the\n * stored object may be modified. Depending on the ~T() destructor, accessing\n * the object after destruction of the SharedMemObject causes undefined\n * behaviour. It is the responsibility of the user of this class to synchronize\n * with other users who have access to the shared object.\n */\n\n> +\n> +/**\n> + * \\fn SharedMemObject::operator=(SharedMemObject &&rhs)\n> + * \\brief Operator= for SharedMemObject\n\n * \\brief Move assignment operator for SharedMemObject\n\n> + * \\param[in] rhs The SharedMemObject object to take the data from\n\n *\n * Moving a SharedMemObject does not affect in stored object.\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator->()\n> + * \\brief Operator-> for SharedMemObject\n> + * \\return The pointer to the object\n\n * \\brief Dereference the stored object\n * \\return Pointer to the stored object\n\n> + */\n> +\n> +/**\n> + * \\fn const T *SharedMemObject::operator->() const\n> + * \\brief Operator-> for SharedMemObject\n> + * \\return The pointer to the const object\n\nSame here. You use \\copydoc to avoid duplicating the documentation,\nthere are a few examples in the code base. You may need to use copydoc\nin the non-const version instead of here though, no sure.\n\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator*()\n> + * \\brief Operator* for SharedMemObject\n> + * \\return The reference to the object\n\n * \\brief Dereference the stored object\n * \\return Reference to the stored object\n\nSame comment regarding copydoc.\n\n> + */\n> +\n> +/**\n> + * \\fn const T &SharedMemObject::operator*() const\n> + * \\brief Operator* for SharedMemObject\n> + * \\return Const reference to the object\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator bool()\n> + * \\brief Operator bool() for SharedMemObject\n> + * \\return True if the object was created OK in the shared memory, false otherwise\n\nAssuming you'll move this to SharedMem,\n\n * \\brief Check if the shared memory allocation succeeded\n * \\return True if allocation of the shared memorysucceeded, false otherwise\n\n> + */\n> +\n> +} // namespace libcamera\n\n/* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2784BC0DA4\n\tfor ;\n\tWed, 27 Mar 2024 12:57:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01A4A63331;\n\tWed, 27 Mar 2024 13:57:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D28461C35\n\tfor ;\n\tWed, 27 Mar 2024 13:57:11 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F44413AC;\n\tWed, 27 Mar 2024 13:56:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XHTmm8Jm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711544198;\n\tbh=LRT8mZoC8+/rE85n9L9tn810GdojALgi3yWjPk5lujY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XHTmm8Jm+D0QIe/4/5lhSaZWdn2bN8XRRLEKXDzbzt2A5HTSCGpe2PysuaVU4cI6H\n\tnXWmDx6MgLH/MDsm8O8t3hcre25HZCfMY9Maxtlr3gLCaKslmgDjtHz2nrcRaVfdnb\n\t+WXjAH4E9d87AZyRWfQ50pO7ck8tudXQpn/vgj4c=","Date":"Wed, 27 Mar 2024 14:57:01 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrei Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ,\n\tDennis Bonke ","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","Message-ID":"<20240327125701.GD8623@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-6-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29120,"web_url":"https://patchwork.libcamera.org/comment/29120/","msgid":"<87le62itef.fsf@redhat.com>","date":"2024-03-28T09:43:04","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Andrei, Dennis,\n\ncould you please help me with the points below?\n\nLaurent Pinchart writes:\n\nThank you, Laurent, for the review and all the suggestions regarding docstring\nimprovements (very nice and educative!) etc.\n\n>> Split the parts which doesn't otherwise depend on the type T or\n>> arguments Args out of the SharedMemObject class into a new\n>> SharedMem class.\n>\n> The commit message should explain why.\n\nDo you have a suggestion what to write there?\n\n>> +\n>> +\t/* Make SharedMem non-copyable for now. */\n>\n> Record the reason in the commit message, and drop this comment.\n\nThe same here.\n\n>> --- /dev/null\n>> +++ b/src/libcamera/shared_mem_object.cpp\n>> @@ -0,0 +1,190 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2023, Raspberry Pi Ltd\n>\n> The documentation is Dennis and Andrei's work, I don't think this should\n> be owned by Raspberry Pi.\n\nShould I put you there as private persons or are there institutions behind you\nthat should own the copyright?\n\n>> +{\n>> +\tint fd = memfd_create(name_.c_str(), MFD_CLOEXEC);\n>> +\tif (fd < 0)\n>> +\t\treturn;\n>> +\n>> +\tfd_ = SharedFD(std::move(fd));\n>> +\tif (!fd_.isValid())\n>> +\t\treturn;\n>> +\n>> +\tif (ftruncate(fd_.get(), size_) < 0)\n>> +\t\treturn;\n>\n> Should you clear fd_ here ? It seems pointless to keep it open, and the\n> class would expose consistent information.\n>\n>> +\n>\n> Should we set the GROW and SHRINK seals (in a separate patch) ?\n>\n>> +\tmem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,\n>> +\t\t fd_.get(), 0);\n>> +\tif (mem_ == MAP_FAILED)\n>> +\t\tmem_ = nullptr;\n>\n> Same here, should you clear fd_ ?\n\nDo you have answers for these questions?\n\nThanks,\nMilan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E7772BD160\n\tfor ;\n\tThu, 28 Mar 2024 09:43:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC1AA63339;\n\tThu, 28 Mar 2024 10:43:12 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C57E61C39\n\tfor ;\n\tThu, 28 Mar 2024 10:43:11 +0100 (CET)","from mail-lj1-f199.google.com (mail-lj1-f199.google.com\n\t[209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-611-Fd6j6uLDO0yUlsXEva63jQ-1; Thu, 28 Mar 2024 05:43:08 -0400","by mail-lj1-f199.google.com with SMTP id\n\t38308e7fff4ca-2d486c08c6eso5552781fa.0\n\tfor ;\n\tThu, 28 Mar 2024 02:43:07 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\th16-20020adff4d0000000b0033e72e104c5sm1277252wrp.34.2024.03.28.02.43.05\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 28 Mar 2024 02:43:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"HF4RJ53J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1711618989;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=dzPfzHPOfB6emHnun5c2q/xLTOZ4s0Z0uQkMBtCmYDA=;\n\tb=HF4RJ53JIzd8sMve+gVW8IffewHaJqPNbh2jkS5JZQHzcxpIXCFDMdZUP3i6j1BS/fpM/t\n\tIwE1vNt/3qpJsXxPeqKswXNDLBSyLhj2f2v9qPO0szx6kWYh+FgslKCKWcQu1KDu4RDS1r\n\tpgL/Gpdo7OkczqF5Cv711VvlYdYscgw=","X-MC-Unique":"Fd6j6uLDO0yUlsXEva63jQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711618987; x=1712223787;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=dzPfzHPOfB6emHnun5c2q/xLTOZ4s0Z0uQkMBtCmYDA=;\n\tb=LlS6ZOxOAVpZSDD/NNw05kPnQ4Bw/sr0Rhl3psVFLBXFFfeNGVvYFnZAJQCtzr0Jz5\n\tv/2q+PUhhIKV/CCF7THNlC+ay2e6LGGBno5MirxG5lSR93jz9GFbIDFkLUcciP2lnWyB\n\t90CDMOPQDZtKqkSJONxB2+UVmbAH89/Skh3fTjrh415PmqXjsUg60KQq1SeXngs01hHG\n\tm53V7S64U3M1uep1QXZhJFHxVvafewprw0psIo/gWGl4OK41Mdg2bSgSRApUagtgCFSX\n\tbUhX7tIWJUkG8nrZ9JglLqDfNhNpmgD5gRJyK1KPwrdrUj+yJRmgwGjmCMeEUW192b8+\n\tlE8w==","X-Gm-Message-State":"AOJu0YwG8shskOjO6S+KWox6XCtwp85PvGXaScTISyHhYrFgOgK0vFzg\n\tfKXb3iGJ6t8t2o9RYYSYuz242WY5Xz0COn1AiT2uPUsFAFucP3dal3Aq/P75gnf96VZt54V+Gzj\n\tRYwV9dIVw3TvSh8Ya9JYezrKIshFHG8LjcQNvHHgWPrAK6N8xPSalM4455LF5ZowjAzhBQjU=","X-Received":["by 2002:a2e:b059:0:b0:2d4:77ae:348c with SMTP id\n\td25-20020a2eb059000000b002d477ae348cmr1742501ljl.22.1711618986871; \n\tThu, 28 Mar 2024 02:43:06 -0700 (PDT)","by 2002:a2e:b059:0:b0:2d4:77ae:348c with SMTP id\n\td25-20020a2eb059000000b002d477ae348cmr1742479ljl.22.1711618986487; \n\tThu, 28 Mar 2024 02:43:06 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHlqYOP+M/L1pQuBantTwCHkoNoD6aHSYryIcThVqZ+woj3BCLAPoNmafrUk7Wr3kojWkWCAQ==","From":"Milan Zamazal ","To":"Andrei Konovalov , Dennis Bonke\n\t","Cc":"libcamera-devel@lists.libcamera.org, Laurent Pinchart\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"<20240327125701.GD8623@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 14:57:01 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>","Date":"Thu, 28 Mar 2024 10:43:04 +0100","Message-ID":"<87le62itef.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29122,"web_url":"https://patchwork.libcamera.org/comment/29122/","msgid":"<9b403d56-b095-430c-8225-465fe7aea5da@gmail.com>","date":"2024-03-28T11:08:43","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":179,"url":"https://patchwork.libcamera.org/api/people/179/","name":"Andrei Konovalov","email":"andrey.konovalov.ynk@gmail.com"},"content":"Hi Milan,\n\nOn 28.03.2024 12:43, Milan Zamazal wrote:\n> Andrei, Dennis,\n> \n> could you please help me with the points below?\n> \n> Laurent Pinchart writes:\n> \n> Thank you, Laurent, for the review and all the suggestions regarding docstring\n> improvements (very nice and educative!) etc.\n> \n>>> Split the parts which doesn't otherwise depend on the type T or\n>>> arguments Args out of the SharedMemObject class into a new\n>>> SharedMem class.\n>>\n>> The commit message should explain why.\n> \n> Do you have a suggestion what to write there?\n\nThis split was suggested by Laurent in his review of the v2 patch:\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2024-January/040344.html\n\nMy understanding was that we shouldn't create an extra copy of the same code\nper each type T.\n\n>>> +\n>>> +\t/* Make SharedMem non-copyable for now. */\n>>\n>> Record the reason in the commit message, and drop this comment.\n> \n> The same here.\n\nThis comment comes from the original code by RaspberryPi.\n\n>>> --- /dev/null\n>>> +++ b/src/libcamera/shared_mem_object.cpp\n>>> @@ -0,0 +1,190 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2023, Raspberry Pi Ltd\n>>\n>> The documentation is Dennis and Andrei's work, I don't think this should\n>> be owned by Raspberry Pi.\n> \n> Should I put you there as private persons or are there institutions behind you\n> that should own the copyright?\n\nIf the author of the patch is \"Andrei Konovalov \"\nthen please put me as a private person.\n\nIf the author of the patch is \"Andrey Konovalov \"\nthen please use \"Copyright (C) , Linaro Ltd\"\n\n>>> +{\n>>> +\tint fd = memfd_create(name_.c_str(), MFD_CLOEXEC);\n>>> +\tif (fd < 0)\n>>> +\t\treturn;\n>>> +\n>>> +\tfd_ = SharedFD(std::move(fd));\n>>> +\tif (!fd_.isValid())\n>>> +\t\treturn;\n>>> +\n>>> +\tif (ftruncate(fd_.get(), size_) < 0)\n>>> +\t\treturn;\n>>\n>> Should you clear fd_ here ? It seems pointless to keep it open, and the\n>> class would expose consistent information.\n\nYes, this makes sense.\n\n>>> +\n>>\n>> Should we set the GROW and SHRINK seals (in a separate patch) ?\n\nYes, this can be done.\nSetting F_SEAL_SHRINK and F_SEAL_GROW after the ftruncate() call above could catch\nsome potential errors related to improper access to the shared memory allocated by\nthe SharedMemObject.\n\n>>> +\tmem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,\n>>> +\t\t fd_.get(), 0);\n>>> +\tif (mem_ == MAP_FAILED)\n>>> +\t\tmem_ = nullptr;\n>>\n>> Same here, should you clear fd_ ?\n\nI think so.\n\nThanks,\nAndrei\n\n> Do you have answers for these questions?\n> \n> Thanks,\n> Milan\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B850CBD160\n\tfor ;\n\tThu, 28 Mar 2024 11:08:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFF906333B;\n\tThu, 28 Mar 2024 12:08:47 +0100 (CET)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C22CC632EA\n\tfor ;\n\tThu, 28 Mar 2024 12:08:46 +0100 (CET)","by mail-ej1-x62d.google.com with SMTP id\n\ta640c23a62f3a-a4715991c32so106275766b.1\n\tfor ;\n\tThu, 28 Mar 2024 04:08:46 -0700 (PDT)","from [192.168.118.26] ([87.116.161.254])\n\tby smtp.gmail.com with ESMTPSA id\n\tn12-20020a1709061d0c00b00a4da28f42f1sm626839ejh.177.2024.03.28.04.08.44\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tThu, 28 Mar 2024 04:08:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"ZF/ppX+X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1711624126; x=1712228926;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=OZqHGJrmN+fYTjiItjc0c0GRtE1i8CkPfFkil04X5YE=;\n\tb=ZF/ppX+XsVK2pmNuZi1D0cafzERiBmPN6XTQHHOun1WfiSGKoBFxD8xwtLf82/TROJ\n\tdIFGwFRhVnTiKJfe4pEqLRpzX1fo0vaCvUQrWHv1/63pjK2mVR63/98/iveWpBgP/dwF\n\tSClnzOLntiealQxuXA6KdY+Cbn4D2YRQBEjhrrSvifG0nOZMXuJYh0/asCmc+Wg2UIMK\n\tOxHj6b6kFgknVmwQGFBLay71sx3DbKBT88pflKxdDk+D0N1FFkUYpx3TQ4X+W/kobkpj\n\t6yLRjkFaOiTKnOzMA3zlQ+81V0U34qyBPs58nifW24d5e6BIIaAFPhMixFX+hqQJyuQj\n\tpJDw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1711624126; x=1712228926;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=OZqHGJrmN+fYTjiItjc0c0GRtE1i8CkPfFkil04X5YE=;\n\tb=DTv0RTXMJmAGNWP1amxP59VUIGCIgqjMpPwbjN1YWZfuSc2rexuOSoKslgR/Luiz0m\n\tAgsBhZ0B4UBDW4DTvsNJ9EABBcyctHz9VusbuO7s9+z0T0HUKbpHRRbeNI7Q8pe7ngD7\n\tUwLs5NBHvG9sP/fNwvcK/ZDF3/+6GotCQ2qgyNkf2LkWY/CGd17BYvYdggBi4+QSHfpL\n\t2Y4cDqxKvk2JfJJI//tjU3lr05QQWNJPpUzOzTWqv4IaQBOZPMLUy6cyF2Wf0W1wElNA\n\tNBtaLfE//sbPGW3pF/WPL4xV6yczNiUGTbdcC4TIHD3K109waO8E16z1XBlvQ5Pb756M\n\t+Zjg==","X-Gm-Message-State":"AOJu0Ywd6MpQbN37MC3Fda8rYDOSHGWCs58dv04mAUKs2OLok3XJoSfB\n\taIpMTVVHF29X+RLHsu5ywAPM+HJB2ihGX7X0TlgvEgtef2C+8SB9","X-Google-Smtp-Source":"AGHT+IEYT3AHlqS7WlvjGugFTM91rx4TU2yb4pw9aGuz5oowVOPLsS1FCiZS6naycOQEZZRUKEcvAA==","X-Received":"by 2002:a17:907:97d4:b0:a47:37af:3783 with SMTP id\n\tjs20-20020a17090797d400b00a4737af3783mr1811381ejc.15.1711624125408; \n\tThu, 28 Mar 2024 04:08:45 -0700 (PDT)","Message-ID":"<9b403d56-b095-430c-8225-465fe7aea5da@gmail.com>","Date":"Thu, 28 Mar 2024 14:08:43 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","To":"Milan Zamazal , Dennis Bonke ","Cc":"libcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>\n\t<87le62itef.fsf@redhat.com>","Content-Language":"en-US","From":"Andrei Konovalov ","In-Reply-To":"<87le62itef.fsf@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29131,"web_url":"https://patchwork.libcamera.org/comment/29131/","msgid":"<87h6gjio4c.fsf@redhat.com>","date":"2024-04-02T19:02:59","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> On Tue, Mar 19, 2024 at 01:35:52PM +0100, Milan Zamazal wrote:\n>> From: Andrei Konovalov \n>>\n>> +class SharedMemObject : public SharedMem\n>> {\n\n[...]\n\n>> private:\n>> -\tstd::string name_;\n>> -\tSharedFD fd_;\n>> \tT *obj_;\n>\n> Do we need to store obj_, or could we always cast the \n\nIIRC something similar has already been suggested, with the conclusion it'd\nbetter not to touch this now. If there is a good idea how to improve this, it\ncan be done later.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EFBF5C0DA4\n\tfor ;\n\tTue, 2 Apr 2024 19:03:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 237966334D;\n\tTue, 2 Apr 2024 21:03:07 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8473B61C33\n\tfor ;\n\tTue, 2 Apr 2024 21:03:05 +0200 (CEST)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-674-aDyfLJImOjG1p3rAf4D2uw-1; Tue, 02 Apr 2024 15:03:02 -0400","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5684bf1440dso2679115a12.3\n\tfor ;\n\tTue, 02 Apr 2024 12:03:02 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tby27-20020a0564021b1b00b0056bf96985adsm7200168edb.32.2024.04.02.12.03.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Apr 2024 12:03:00 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"FrbEdn67\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712084584;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Dyv7r+auspyqLvzv9RRVdkU+xcuaa2q2vz41eTF3eek=;\n\tb=FrbEdn67VIerUaDOjyyqwipZgjaECwUmCscwxJd9ZaID3ZL8Bc6ER5eLh4zDOSg0G8PcYC\n\tDCqgHEMAG2p6r98ibc/icwwNxbz74vNdDho2GsV5O8EVoEljhkrm71rbD2/8BojAP1GWyY\n\t0CwpofV5dk59N5r/EA3fmyn/oUXwIXE=","X-MC-Unique":"aDyfLJImOjG1p3rAf4D2uw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712084581; x=1712689381;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Dyv7r+auspyqLvzv9RRVdkU+xcuaa2q2vz41eTF3eek=;\n\tb=FPCyWIxPyv9G7mSRBd/dQdj42/MsQySern3WKInhEv5LfKaj/mpLVfBjlw1HSUhwFd\n\t6lxku7yuDq/RlX/7gTFvTXkxhZr3I4CmZdC/X5P/WvdlG4ATTrN/+UkQc7toCWc52h0G\n\tpfRLzh8PvYbJ/9TEtXWTl6mdgNbqDs2RyxcI7xhNmwCBipJNu9aKRrLbFqKm7DHJBjTX\n\tdLCDqEwk0a/xacroVvV+TpMdGF2074uwxTiFUN7EYu/x1BUxdulq4d+edAMeJy1CyGUb\n\t2bJo8OuT1b2YBMDQ7gRuD2A9iHpv5LcCyJL7ZIWFMnP7sPbUKDC2L9GarzT2BT9X6x9f\n\tSHJg==","X-Gm-Message-State":"AOJu0Yw6DQsM4IDWnF5L4C5lTuVOkTFLYFtHbknSU9ih89UEGwUByv63\n\td9WhGGMCTQaNnU1SyrUhRIRZIbzK8YHopFwRQNxNPdI02MyYjdD2qrpsXeSDoH2454+R4ME3Wed\n\t/q9pC7wikZhUtkT0oNT5/WaRdsn9lPywVEWtyx6tyleTjZkn6Ni7wuyUMMS+5TFl2KX4l7Ow=","X-Received":["by 2002:a50:d5c1:0:b0:56b:94e3:856c with SMTP id\n\tg1-20020a50d5c1000000b0056b94e3856cmr7960816edj.36.1712084581793; \n\tTue, 02 Apr 2024 12:03:01 -0700 (PDT)","by 2002:a50:d5c1:0:b0:56b:94e3:856c with SMTP id\n\tg1-20020a50d5c1000000b0056b94e3856cmr7960805edj.36.1712084581388; \n\tTue, 02 Apr 2024 12:03:01 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHF0SHcje5/tV8C465qwchTdDUPbXV1fDHQA68cLpt7Jh3yEU0m11blmtlIzEn45UKleDlzIw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Andrei Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , Dennis Bonke\n\t","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"<20240327125701.GD8623@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 14:57:01 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>","Date":"Tue, 02 Apr 2024 21:02:59 +0200","Message-ID":"<87h6gjio4c.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29132,"web_url":"https://patchwork.libcamera.org/comment/29132/","msgid":"<878r1vinnw.fsf@redhat.com>","date":"2024-04-02T19:12:51","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Andrei,\n\nthank you for clarification.\n\nAndrei Konovalov writes:\n\n> Hi Milan,\n>\n> On 28.03.2024 12:43, Milan Zamazal wrote:\n>> Andrei, Dennis,\n>> could you please help me with the points below?\n>> Laurent Pinchart writes:\n>> Thank you, Laurent, for the review and all the suggestions regarding docstring\n>> improvements (very nice and educative!) etc.\n>> \n>>>> Split the parts which doesn't otherwise depend on the type T or\n>>>> arguments Args out of the SharedMemObject class into a new\n>>>> SharedMem class.\n>>>\n>>> The commit message should explain why.\n>> Do you have a suggestion what to write there?\n>\n> This split was suggested by Laurent in his review of the v2 patch:\n> https://lists.libcamera.org/pipermail/libcamera-devel/2024-January/040344.html\n>\n> My understanding was that we shouldn't create an extra copy of the same code\n> per each type T.\n\nIn such a case, I don't have any idea what smart to add there, it looks saying\nbasically the thing.\n\n>>>> +\n>>>> +\t/* Make SharedMem non-copyable for now. */\n>>>\n>>> Record the reason in the commit message, and drop this comment.\n>> The same here.\n>\n> This comment comes from the original code by RaspberryPi.\n\nI couldn't find the reason, so I left this unchanged.\n\n[...]\n\n>>>> +\tif (ftruncate(fd_.get(), size_) < 0)\n>>>> +\t\treturn;\n>>>\n>>> Should we set the GROW and SHRINK seals (in a separate patch) ?\n>\n> Yes, this can be done.\n> Setting F_SEAL_SHRINK and F_SEAL_GROW after the ftruncate() call above could catch\n> some potential errors related to improper access to the shared memory allocated by\n> the SharedMemObject.\n\nAdded to TODO.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5B2FCBD16B\n\tfor ;\n\tTue, 2 Apr 2024 19:13:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7031363360;\n\tTue, 2 Apr 2024 21:13:00 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD78D63334\n\tfor ;\n\tTue, 2 Apr 2024 21:12:58 +0200 (CEST)","from mail-ej1-f72.google.com (mail-ej1-f72.google.com\n\t[209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-556-t2RrCdBIO4C8KBnXAOTPHA-1; Tue, 02 Apr 2024 15:12:54 -0400","by mail-ej1-f72.google.com with SMTP id\n\ta640c23a62f3a-a4e34a32a92so16303566b.0\n\tfor ;\n\tTue, 02 Apr 2024 12:12:54 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tc3-20020a170906340300b00a4739efd7cesm6894556ejb.60.2024.04.02.12.12.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Apr 2024 12:12:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"HJr8otCK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712085177;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=H0Oi77LMGYCqg8bPUx/1RPreEcCrc2LfDHj0zUQvzMA=;\n\tb=HJr8otCKhBUsXwhTA3as2amT5mYAcbOma+2xHrS3BsD2kwmRMGv6KGNuTHcANHFC4CUNty\n\tZXML2lgplz8wA/7D1NDXMFuia9EuhM2vdU3wdxEESy3ya2D5F/hDbxf5lWtSraJwTLfWuk\n\t7r09bN2B/HGagFgLdmB1kEi7ADMW1Cw=","X-MC-Unique":"t2RrCdBIO4C8KBnXAOTPHA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712085173; x=1712689973;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=H0Oi77LMGYCqg8bPUx/1RPreEcCrc2LfDHj0zUQvzMA=;\n\tb=jK9fbg5LfKLWTyAQaTmaMwApb42wpJEAtuKmuxlSyHOkWWJoj+7MnJCPTt2GaYeKOZ\n\tTY4r875gLZwatgMi+cFCYUQUsGbg3UBB9LZnGnJLyxj6n+30tXq4czd+HhHfyLCea7/p\n\t9nBKkVQ/a8sTuUpJmDDvo3EWSTdQj4PgI6MyOSrL5EW6U07Pe329WbkblJlhD6LbXXgK\n\tE5ge+SR2w8Pro7T5H+N+EOp2HV55FwrsviMXsq+Agd2iaype3sRkoSN2DmRxxVbbefcF\n\tjnEk84KkbnCUrZfbgeUYjsJOcoABTOhe+SbQQV1FfzfkwKVEC/q5E6dYDk6lKdJg+bux\n\tHUgA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUyeuNmLYaWlxId3X08yOvtS8IwWBr5Q9iC4GQsLuELMPuN2YpCCLYmIR1mS+Ju/o3z7z9pWoWLItMInRJm5QvZRje15fB+KkJw+zgqwRVHmeJ2Fw==","X-Gm-Message-State":"AOJu0Yws+S6wtcEl8aQYZEU1G4ykSBwRiGJ4tJVSwv6T6jcAgZ7TzuWg\n\tM+bO64AVsHbtlg5aJyE5eTJQcZn/YlwV5Ob2dIVBtdBON2JlKWBbJCjYEqW3nF7TiBWEXTNOGwW\n\tMmCTy0iyBRb+99E3dhogOYQaO4cVmC/2SbQ/qYyXJcW8k2wmf/k54UsqVFp8U0B3uArRCf8M=","X-Received":["by 2002:a17:906:3d42:b0:a45:40e4:8c8 with SMTP id\n\tq2-20020a1709063d4200b00a4540e408c8mr338872ejf.16.1712085173324; \n\tTue, 02 Apr 2024 12:12:53 -0700 (PDT)","by 2002:a17:906:3d42:b0:a45:40e4:8c8 with SMTP id\n\tq2-20020a1709063d4200b00a4540e408c8mr338850ejf.16.1712085172955; \n\tTue, 02 Apr 2024 12:12:52 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHqVpq+retSiLqnSDWhKLlBZo7MsRtnCcoIjcNT73S8mIUm5cJnT/iTN5N/m10G4zNkIY1Yog==","From":"Milan Zamazal ","To":"Andrei Konovalov ","Cc":"Dennis Bonke ,\n\tlibcamera-devel@lists.libcamera.org, Laurent Pinchart\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham ","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"<9b403d56-b095-430c-8225-465fe7aea5da@gmail.com> (Andrei\n\tKonovalov's message of \"Thu, 28 Mar 2024 14:08:43 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>\n\t<87le62itef.fsf@redhat.com>\n\t<9b403d56-b095-430c-8225-465fe7aea5da@gmail.com>","Date":"Tue, 02 Apr 2024 21:12:51 +0200","Message-ID":"<878r1vinnw.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29134,"web_url":"https://patchwork.libcamera.org/comment/29134/","msgid":"<171208652644.1844042.4617036082411274523@ping.linuxembedded.co.uk>","date":"2024-04-02T19:35:26","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-04-02 20:12:51)\n> Hi Andrei,\n> \n> thank you for clarification.\n> \n> Andrei Konovalov writes:\n> \n> > Hi Milan,\n> >\n> > On 28.03.2024 12:43, Milan Zamazal wrote:\n> >> Andrei, Dennis,\n> >> could you please help me with the points below?\n> >> Laurent Pinchart writes:\n> >> Thank you, Laurent, for the review and all the suggestions regarding docstring\n> >> improvements (very nice and educative!) etc.\n> >> \n> >>>> Split the parts which doesn't otherwise depend on the type T or\n> >>>> arguments Args out of the SharedMemObject class into a new\n> >>>> SharedMem class.\n> >>>\n> >>> The commit message should explain why.\n> >> Do you have a suggestion what to write there?\n> >\n> > This split was suggested by Laurent in his review of the v2 patch:\n> > https://lists.libcamera.org/pipermail/libcamera-devel/2024-January/040344.html\n> >\n> > My understanding was that we shouldn't create an extra copy of the same code\n> > per each type T.\n> \n> In such a case, I don't have any idea what smart to add there, it looks saying\n> basically the thing.\n> \n> >>>> +\n> >>>> + /* Make SharedMem non-copyable for now. */\n> >>>\n> >>> Record the reason in the commit message, and drop this comment.\n> >> The same here.\n> >\n> > This comment comes from the original code by RaspberryPi.\n> \n> I couldn't find the reason, so I left this unchanged.\n\nWhat would it mean to copy a SharedMem object? Would the copy simply be\na lightweight copy of the SharedMem class with an increased reference?\n(like copying a shared pointer) Or does it make a clean copy of the\nunderlying SharedMem?\n\nI suspect answering that question answers the why it's non-copyable. Or\nmaybe because that question isn't answered is why it's set as\nnon-copyable....\n\n> [...]\n> \n> >>>> + if (ftruncate(fd_.get(), size_) < 0)\n> >>>> + return;\n> >>>\n> >>> Should we set the GROW and SHRINK seals (in a separate patch) ?\n> >\n> > Yes, this can be done.\n> > Setting F_SEAL_SHRINK and F_SEAL_GROW after the ftruncate() call above could catch\n> > some potential errors related to improper access to the shared memory allocated by\n> > the SharedMemObject.\n> \n> Added to TODO.\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 48358BD16B\n\tfor ;\n\tTue, 2 Apr 2024 19:35:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F44963360;\n\tTue, 2 Apr 2024 21:35:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8C1963334\n\tfor ;\n\tTue, 2 Apr 2024 21:35:29 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C3C45564;\n\tTue, 2 Apr 2024 21:34:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OBqhf48E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712086492;\n\tbh=94iS4p0xzkUYyMxuImF6c81sn6pbqCKzne/u2Y+rli0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OBqhf48EymyeqdRJ/7ASYHk3aks0ZcoN5RxenlObcnl77TTykbHgFGc3aDIBoUWRe\n\t2+oL9BCT2MgjIoPZbmt/X436VNvgd4dYu8lCuGYEJqsGOpTrc3akEaZ2/QwlAyYgVq\n\tCoNibHzu240l6Vy9+vBwew7x8TMKgYeF/lY0a9X8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<878r1vinnw.fsf@redhat.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>\n\t<87le62itef.fsf@redhat.com>\n\t<9b403d56-b095-430c-8225-465fe7aea5da@gmail.com>\n\t<878r1vinnw.fsf@redhat.com>","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","From":"Kieran Bingham ","Cc":"Dennis Bonke , libcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ","To":"Andrei Konovalov ,\n\tMilan Zamazal ","Date":"Tue, 02 Apr 2024 20:35:26 +0100","Message-ID":"<171208652644.1844042.4617036082411274523@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29137,"web_url":"https://patchwork.libcamera.org/comment/29137/","msgid":"<87msqbh7cn.fsf@redhat.com>","date":"2024-04-02T19:50:32","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthe changes in this patch were not completely trivial so I'd suggest briefly\nreviewing the updated version as a whole again, once it is posted.\n\nThanks,\nMilan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0E99FC0DA4\n\tfor ;\n\tTue, 2 Apr 2024 19:50:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AED263363;\n\tTue, 2 Apr 2024 21:50:41 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3915D63334\n\tfor ;\n\tTue, 2 Apr 2024 21:50:40 +0200 (CEST)","from mail-ej1-f72.google.com (mail-ej1-f72.google.com\n\t[209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-41-5MnWioiCNtmFO3fZspGnkg-1; Tue, 02 Apr 2024 15:50:34 -0400","by mail-ej1-f72.google.com with SMTP id\n\ta640c23a62f3a-a46bae02169so15073466b.1\n\tfor ;\n\tTue, 02 Apr 2024 12:50:34 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tan19-20020a17090656d300b00a4e58c74c9fsm3855957ejc.6.2024.04.02.12.50.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Apr 2024 12:50:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"J2/AdjXk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712087439;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=qman+8PFQAc/V65pXD3HliVC7PJrvWvUsUJHFRz01vA=;\n\tb=J2/AdjXkAxp9ODAWcIGUAVO9iv2sxaOqq8ksnwKRnlo71hnDfTslqXazpArWTtj3ZKERYx\n\t7rS8mf9USi9YvhoOicizBeIO61IJtDoBEyL55K+TNEJK3R6YUkdEcV57Pnf3JNTv3ntLi2\n\txohroaDLOZ67UWqnxaB2RcdRIVtl3Xs=","X-MC-Unique":"5MnWioiCNtmFO3fZspGnkg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712087433; x=1712692233;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=qman+8PFQAc/V65pXD3HliVC7PJrvWvUsUJHFRz01vA=;\n\tb=mls5zIQZsy9NVu0xW5RU1wHeJwP57dCjhXcwHYXv+PDkp5WKDtKQal1DRZ9XcupDTZ\n\tL1D1g5CzHGhh6tOGosBM3LrgLeRota96HdNkhbXQ65Qp24c842VFD1qTZFLvlW4jL51x\n\tPwSjTltJ/8jHM/acs7jGfD/81DSDPXB1jcrGmIkEgODQ8K6oi9ciLSeCTX9Sin+cMkST\n\tWfe/DlKRclIeo50q1RsVYqrcohtF1M9OfdW3uyeQXMvhGd73tgzBUIwZ/qUgNjRj2KuR\n\tbFdNj3oT/hcUACLwv8LAEmjGAWybMRQFBu6ZU5c4gu0rsWg9rzRxDyMA59rTY2MvgilK\n\t9Wvw==","X-Gm-Message-State":"AOJu0YwjLuwJLvwHgZiYWBK+JQW0gMh3UZGdq9igpyErpKmNBOBCYYpv\n\tZSFh9Ry2Fc9UGCfGnuUPlOVfpLAiTs0eje55tbtG3PSOt054rnALVcvqgibnE86kq4E4h43Quu8\n\tnWzbseDEQeuR3+jweQyDw3axNRULFdh0FvqgW6B4RJDc8X15RvinSivw8A6eu6PVbfdPXS10=","X-Received":["by 2002:a17:906:345b:b0:a4e:5f6f:4dbb with SMTP id\n\td27-20020a170906345b00b00a4e5f6f4dbbmr449625ejb.13.1712087433791; \n\tTue, 02 Apr 2024 12:50:33 -0700 (PDT)","by 2002:a17:906:345b:b0:a4e:5f6f:4dbb with SMTP id\n\td27-20020a170906345b00b00a4e5f6f4dbbmr449601ejb.13.1712087433425; \n\tTue, 02 Apr 2024 12:50:33 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHCLHWAlGCY2UiV+pQVk2cDm2dbryZJtogrBIJH0fNCy0RbLyJL9AOPon6yKSrwsTDZCBXKsg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Andrei Konovalov\n\t, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede , Kieran\n\tBingham , Dennis Bonke\n\t","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"<20240327125701.GD8623@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 14:57:01 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>","Date":"Tue, 02 Apr 2024 21:50:32 +0200","Message-ID":"<87msqbh7cn.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29147,"web_url":"https://patchwork.libcamera.org/comment/29147/","msgid":"<87plv6epcc.fsf@redhat.com>","date":"2024-04-03T10:02:27","subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> On Tue, Apr 02, 2024 at 08:35:26PM +0100, Kieran Bingham wrote:\n>> Quoting Milan Zamazal (2024-04-02 20:12:51)\n>> > Hi Andrei,\n>\n>> > \n>> > thank you for clarification.\n>> > \n>> > Andrei Konovalov writes:\n>> > \n>> > > Hi Milan,\n>> > >\n>> > > On 28.03.2024 12:43, Milan Zamazal wrote:\n>> > >> Andrei, Dennis,\n>> > >> could you please help me with the points below?\n>> > >> Laurent Pinchart writes:\n>> > >> Thank you, Laurent, for the review and all the suggestions regarding docstring\n>> > >> improvements (very nice and educative!) etc.\n>> > >> \n>> > >>>> Split the parts which doesn't otherwise depend on the type T or\n>> > >>>> arguments Args out of the SharedMemObject class into a new\n>> > >>>> SharedMem class.\n>> > >>>\n>> > >>> The commit message should explain why.\n>> > >>\n>> > >> Do you have a suggestion what to write there?\n>> > >\n>> > > This split was suggested by Laurent in his review of the v2 patch:\n>> > > https://lists.libcamera.org/pipermail/libcamera-devel/2024-January/040344.html\n>> > >\n>> > > My understanding was that we shouldn't create an extra copy of the same code\n>> > > per each type T.\n>> > \n>> > In such a case, I don't have any idea what smart to add there, it looks saying\n>> > basically the thing.\n>\n> How about\n>\n> The SharedMemObject class template contains a fair amount of inline code\n> that does not depend on the template types T. To avoid duplicating it in\n> every template specialization, split that code to a separate base\n> SharedMem class.\n>\n>> > >>>> +\n>> > >>>> + /* Make SharedMem non-copyable for now. */\n>> > >>>\n>> > >>> Record the reason in the commit message, and drop this comment.\n>> > >>\n>> > >> The same here.\n>> > >\n>> > > This comment comes from the original code by RaspberryPi.\n>> > \n>> > I couldn't find the reason, so I left this unchanged.\n>> \n>> What would it mean to copy a SharedMem object? Would the copy simply be\n>> a lightweight copy of the SharedMem class with an increased reference?\n>> (like copying a shared pointer) Or does it make a clean copy of the\n>> underlying SharedMem?\n>> \n>> I suspect answering that question answers the why it's non-copyable. Or\n>> maybe because that question isn't answered is why it's set as\n>> non-copyable....\n>\n> :-)\n>\n> We could define copy semantics, but because we haven't, the class should\n> be non-copyable given that it owns the mapped memory resource. The\n> default copy constructor would lead to use-after-free (or rather\n> use-after-mmap).\n\nThanks, added the explanations to the commit message.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C7AD8C0DA4\n\tfor ;\n\tWed, 3 Apr 2024 10:02:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C04B963373;\n\tWed, 3 Apr 2024 12:02:34 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B29D61C31\n\tfor ;\n\tWed, 3 Apr 2024 12:02:33 +0200 (CEST)","from mail-lf1-f70.google.com (mail-lf1-f70.google.com\n\t[209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-63-JbvXFsluMAq6rqfa8b06eQ-1; Wed, 03 Apr 2024 06:02:30 -0400","by mail-lf1-f70.google.com with SMTP id\n\t2adb3069b0e04-515c91a7ffdso3966176e87.0\n\tfor ;\n\tWed, 03 Apr 2024 03:02:30 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tu3-20020a17090626c300b00a4e4f12a390sm5031313ejc.28.2024.04.03.03.02.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Apr 2024 03:02:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"OmuII3yM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712138552;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=dfgddwubSJ3qJ3nMeIvtTBW19N/yTXEv2KibDKBPs1A=;\n\tb=OmuII3yM5q7fs46V6hNMmcUWpYj7hD0sBjwDRvScqZi48qB3QdgQvQqYWmcOqjwtS49ESL\n\tpXTICzshq8SneB3RfW2HaCLindV8k0of1nOyQgTzKTnX4CgRgd16lXqRfCZJrdE5Ts0xj8\n\t2r9Ytl+kDmg3LBJOmp/Kjk7WTMZza5o=","X-MC-Unique":"JbvXFsluMAq6rqfa8b06eQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712138549; x=1712743349;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=dfgddwubSJ3qJ3nMeIvtTBW19N/yTXEv2KibDKBPs1A=;\n\tb=S6BuhD7oH/5exwWfKiGtopoJap51Uh3mjjSo5PwIhkxhb9X1+fJXpZnj9KR8t+LEYu\n\tOcj8AtgDCXtI+/tiKzC5SKsEo9i3/5spLHUL4OPnoNNjuKWbm19VCRrf0WxQa46LioEC\n\tUsg9YmcXpIqDTf3CFn5UxwTkKLt5hOqt2VmEwOaM3ATHCHpenFQ2+3yyfBZzCyaOUHck\n\tJzqXwnRLbaWQ51Ek/O5scRErxFBCx+y0tDlnXzOyYLiwBK4XjFRYTAGRSAYd52Vr50rl\n\tTBUUsB2r0owFJqN7Axi+3WPLvJ0LnRPk91pqgHCqmlYHnHgW3CNjGc+pAk405kSANc+6\n\tzG0Q==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVphGm4RaYOaGoGXGprJ+yw506LNhnw80Thqzi6eQyeWNfFfErccJojG3QLUwjFT4xfPhY76zyiMAOmuy7bmJ8+EzN/8iCG+Xw6vQJldjwC8rsLMA==","X-Gm-Message-State":"AOJu0Yyp5KJwGwrwh963FV4K4gTT+Y2SIQ4rj0AnZY0k1QmUfgnjjZip\n\tcY2uZsuTUvazt6DWcAfHFxfSlJJ1MbMh5ONYMdodRvtbIixMznynLOjmNH3Kv5zrMqS0aYJfBQp\n\t/rUAMmMNT+jLn/GvLIeYJ41csFcwlfSoA/QxjNq/lZ4mmojkQHh9t2tW2x9LNrZDCZQgVgqA=","X-Received":["by 2002:ac2:4e11:0:b0:516:9792:773c with SMTP id\n\te17-20020ac24e11000000b005169792773cmr4283303lfr.48.1712138549498; \n\tWed, 03 Apr 2024 03:02:29 -0700 (PDT)","by 2002:ac2:4e11:0:b0:516:9792:773c with SMTP id\n\te17-20020ac24e11000000b005169792773cmr4283273lfr.48.1712138549130; \n\tWed, 03 Apr 2024 03:02:29 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEOZH/Khmb6gv2WAQdIvpejJTSHX+HoLeiB7h9g2EZuvz8D2WYaPcjWy7AXoGSmFhWZKrxprA==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , Andrei Konovalov\n\t, Dennis Bonke ,\n\tlibcamera-devel@lists.libcamera.org, Bryan O'Donoghue\n\t, Maxime Ripard , Pavel\n\tMachek , Hans de Goede ","Subject":"Re: [PATCH v6 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"<20240402212054.GD16740@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 3 Apr 2024 00:20:54 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-6-mzamazal@redhat.com>\n\t<20240327125701.GD8623@pendragon.ideasonboard.com>\n\t<87le62itef.fsf@redhat.com>\n\t<9b403d56-b095-430c-8225-465fe7aea5da@gmail.com>\n\t<878r1vinnw.fsf@redhat.com>\n\t<171208652644.1844042.4617036082411274523@ping.linuxembedded.co.uk>\n\t<20240402212054.GD16740@pendragon.ideasonboard.com>","Date":"Wed, 03 Apr 2024 12:02:27 +0200","Message-ID":"<87plv6epcc.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]