[{"id":30531,"web_url":"https://patchwork.libcamera.org/comment/30531/","msgid":"<0e2f0914-42e4-49be-8078-c8c5eb962bc1@redhat.com>","date":"2024-07-31T18:58:45","subject":"Re: [PATCH v2 1/4] libcamera: base: Add MemFd helper class","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 7/31/24 3:59 PM, Laurent Pinchart wrote:\n> libcamera creates memfds in two locations already, duplicating some\n> code. Move the code to a new MemFd helper class.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Milan Zamazal \n> Reviewed-by: Kieran Bingham \n\nThanks, patch looks good to me:\n\nReviewed-by: Hans de Goede \n\nRegards,\n\nHans\n\n\n\n> ---\n> include/libcamera/base/memfd.h | 34 ++++++++\n> include/libcamera/base/meson.build | 1 +\n> src/libcamera/base/memfd.cpp | 116 ++++++++++++++++++++++++++++\n> src/libcamera/base/meson.build | 1 +\n> src/libcamera/dma_buf_allocator.cpp | 46 +----------\n> src/libcamera/shared_mem_object.cpp | 21 ++---\n> 6 files changed, 161 insertions(+), 58 deletions(-)\n> create mode 100644 include/libcamera/base/memfd.h\n> create mode 100644 src/libcamera/base/memfd.cpp\n> \n> diff --git a/include/libcamera/base/memfd.h b/include/libcamera/base/memfd.h\n> new file mode 100644\n> index 000000000000..b0edd2de5d83\n> --- /dev/null\n> +++ b/include/libcamera/base/memfd.h\n> @@ -0,0 +1,34 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * Anonymous file creation\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class MemFd\n> +{\n> +public:\n> +\tenum class Seal {\n> +\t\tNone = 0,\n> +\t\tShrink = (1 << 0),\n> +\t\tGrow = (1 << 1),\n> +\t};\n> +\n> +\tusing Seals = Flags;\n> +\n> +\tstatic UniqueFD create(const char *name, std::size_t size,\n> +\t\t\t Seals seals = Seal::None);\n> +};\n> +\n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MemFd::Seal)\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> index bace25d56b13..2a0cee317204 100644\n> --- a/include/libcamera/base/meson.build\n> +++ b/include/libcamera/base/meson.build\n> @@ -21,6 +21,7 @@ libcamera_base_private_headers = files([\n> 'event_notifier.h',\n> 'file.h',\n> 'log.h',\n> + 'memfd.h',\n> 'message.h',\n> 'mutex.h',\n> 'private.h',\n> diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp\n> new file mode 100644\n> index 000000000000..72474307af09\n> --- /dev/null\n> +++ b/src/libcamera/base/memfd.cpp\n> @@ -0,0 +1,116 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * Anonymous file creation\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file base/memfd.h\n> + * \\brief Anonymous file creation\n> + */\n> +\n> +/* uClibc doesn't provide the file sealing API. */\n> +#ifndef __DOXYGEN__\n> +#if not HAVE_FILE_SEALS\n> +#define F_ADD_SEALS\t\t1033\n> +#define F_SEAL_SHRINK\t\t0x0002\n> +#define F_SEAL_GROW\t\t0x0004\n> +#endif\n> +#endif\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(File)\n> +\n> +/**\n> + * \\class MemFd\n> + * \\brief Helper class to create anonymous files\n> + *\n> + * Anonymous files behave like regular files, and can be modified, truncated,\n> + * memory-mapped and so on. Unlike regular files, they however live in RAM and\n> + * don't have permanent backing storage.\n> + */\n> +\n> +/**\n> + * \\enum MemFd::Seal\n> + * \\brief Seals for the MemFd::create() function\n> + * \\var MemFd::Seal::None\n> + * \\brief No seals (used as default value)\n> + * \\var MemFd::Seal::Shrink\n> + * \\brief Prevent the memfd from shrinking\n> + * \\var MemFd::Seal::Grow\n> + * \\brief Prevent the memfd from growing\n> + */\n> +\n> +/**\n> + * \\typedef MemFd::Seals\n> + * \\brief A bitwise combination of MemFd::Seal values\n> + */\n> +\n> +/**\n> + * \\brief Create an anonymous file\n> + * \\param[in] name The file name (displayed in symbolic links in /proc/self/fd/)\n> + * \\param[in] size The file size\n> + * \\param[in] seals The file seals\n> + *\n> + * This function is a helper that wraps anonymous file (memfd) creation and\n> + * sets the file size and optional seals.\n> + *\n> + * \\return The descriptor of the anonymous file if creation succeeded, or an\n> + * invalid UniqueFD otherwise\n> + */\n> +UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals)\n> +{\n> +#if HAVE_MEMFD_CREATE\n> +\tint ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);\n> +#else\n> +\tint ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);\n> +#endif\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(File, Error)\n> +\t\t\t<< \"Failed to allocate memfd storage for \" << name\n> +\t\t\t<< \": \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tUniqueFD memfd(ret);\n> +\n> +\tret = ftruncate(memfd.get(), size);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n> +\t\tLOG(File, Error)\n> +\t\t\t<< \"Failed to set memfd size for \" << name\n> +\t\t\t<< \": \" << strerror(ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tif (seals) {\n> +\t\tint fileSeals = (seals & Seal::Shrink ? F_SEAL_SHRINK : 0)\n> +\t\t\t | (seals & Seal::Grow ? F_SEAL_GROW : 0);\n> +\n> +\t\tret = fcntl(memfd.get(), F_ADD_SEALS, fileSeals);\n> +\t\tif (ret < 0) {\n> +\t\t\tret = errno;\n> +\t\t\tLOG(File, Error)\n> +\t\t\t\t<< \"Failed to seal the memfd for \" << name\n> +\t\t\t\t<< \": \" << strerror(ret);\n> +\t\t\treturn {};\n> +\t\t}\n> +\t}\n> +\n> +\treturn memfd;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> index 7a7fd7e4ca87..4a228d335ba4 100644\n> --- a/src/libcamera/base/meson.build\n> +++ b/src/libcamera/base/meson.build\n> @@ -10,6 +10,7 @@ libcamera_base_sources = files([\n> 'file.cpp',\n> 'flags.cpp',\n> 'log.cpp',\n> + 'memfd.cpp',\n> 'message.cpp',\n> 'mutex.cpp',\n> 'object.cpp',\n> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> index c06eca7d04ec..be6efb89fbb7 100644\n> --- a/src/libcamera/dma_buf_allocator.cpp\n> +++ b/src/libcamera/dma_buf_allocator.cpp\n> @@ -13,7 +13,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> \n> @@ -22,6 +21,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> /**\n> * \\file dma_buf_allocator.cpp\n> @@ -126,54 +126,16 @@ DmaBufAllocator::~DmaBufAllocator() = default;\n> * \\brief Check if the DmaBufAllocator instance is valid\n> * \\return True if the DmaBufAllocator is valid, false otherwise\n> */\n> -\n> -/* uClibc doesn't provide the file sealing API. */\n> -#ifndef __DOXYGEN__\n> -#if not HAVE_FILE_SEALS\n> -#define F_ADD_SEALS\t\t1033\n> -#define F_SEAL_SHRINK\t\t0x0002\n> -#endif\n> -#endif\n> -\n> UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)\n> {\n> \t/* Size must be a multiple of the page size. Round it up. */\n> \tstd::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;\n> \tsize = (size + pageMask) & ~pageMask;\n> \n> -#if HAVE_MEMFD_CREATE\n> -\tint ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);\n> -#else\n> -\tint ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);\n> -#endif\n> -\tif (ret < 0) {\n> -\t\tret = errno;\n> -\t\tLOG(DmaBufAllocator, Error)\n> -\t\t\t<< \"Failed to allocate memfd storage for \" << name\n> -\t\t\t<< \": \" << strerror(ret);\n> -\t\treturn {};\n> -\t}\n> -\n> -\tUniqueFD memfd(ret);\n> -\n> -\tret = ftruncate(memfd.get(), size);\n> -\tif (ret < 0) {\n> -\t\tret = errno;\n> -\t\tLOG(DmaBufAllocator, Error)\n> -\t\t\t<< \"Failed to set memfd size for \" << name\n> -\t\t\t<< \": \" << strerror(ret);\n> -\t\treturn {};\n> -\t}\n> -\n> \t/* udmabuf dma-buffers *must* have the F_SEAL_SHRINK seal. */\n> -\tret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);\n> -\tif (ret < 0) {\n> -\t\tret = errno;\n> -\t\tLOG(DmaBufAllocator, Error)\n> -\t\t\t<< \"Failed to seal the memfd for \" << name\n> -\t\t\t<< \": \" << strerror(ret);\n> +\tUniqueFD memfd = MemFd::create(name, size, MemFd::Seal::Shrink);\n> +\tif (!memfd.isValid())\n> \t\treturn {};\n> -\t}\n> \n> \tstruct udmabuf_create create;\n> \n> @@ -182,7 +144,7 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)\n> \tcreate.offset = 0;\n> \tcreate.size = size;\n> \n> -\tret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);\n> +\tint ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);\n> \tif (ret < 0) {\n> \t\tret = errno;\n> \t\tLOG(DmaBufAllocator, Error)\n> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp\n> index 809fbdaf95de..022645e71a35 100644\n> --- a/src/libcamera/shared_mem_object.cpp\n> +++ b/src/libcamera/shared_mem_object.cpp\n> @@ -13,9 +13,9 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> -#include \n> +\n> +#include \n> \n> /**\n> * \\file shared_mem_object.cpp\n> @@ -58,22 +58,11 @@ SharedMem::SharedMem() = default;\n> */\n> SharedMem::SharedMem(const std::string &name, std::size_t size)\n> {\n> -#if HAVE_MEMFD_CREATE\n> -\tint fd = memfd_create(name.c_str(), MFD_CLOEXEC);\n> -#else\n> -\tint fd = syscall(SYS_memfd_create, name.c_str(), MFD_CLOEXEC);\n> -#endif\n> -\tif (fd < 0)\n> +\tUniqueFD memfd = MemFd::create(name.c_str(), size);\n> +\tif (!memfd.isValid())\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\tfd_ = SharedFD();\n> -\t\treturn;\n> -\t}\n> +\tfd_ = SharedFD(std::move(memfd));\n> \n> \tvoid *mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,\n> \t\t\t fd_.get(), 0);","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 3D998BDC71\n\tfor ;\n\tWed, 31 Jul 2024 18:58:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D093163377;\n\tWed, 31 Jul 2024 20:58:53 +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 C136563373\n\tfor ;\n\tWed, 31 Jul 2024 20:58:51 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-3-V53hubh1M0SsTaZBG9W_WA-1; Wed, 31 Jul 2024 14:58:49 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a7d63fbf4afso396768666b.2\n\tfor ;\n\tWed, 31 Jul 2024 11:58:48 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a7acad9104csm794034166b.157.2024.07.31.11.58.46\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 31 Jul 2024 11:58:46 -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=\"B5vSv0Kc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1722452330;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=ci+Z4a0YKP17caeue+7YhZGGb5J9YaZJ5IBcjtorqxg=;\n\tb=B5vSv0KcooOMlMam/7mnjgqLGVrDol1HPcGIQVJ8mIwPM5nNsF0RwBX9QTJ4Iedj5P9ZRe\n\tJ00jpPOmq9Za6EeXUhICYHyru6SeojChfKifpg/YDDjJllhOF9Kq+0l3fo2rv6WqzIFSqt\n\tSJ6tSwWPlLAKPT2JSgmHrD/HR/0scBI=","X-MC-Unique":"V53hubh1M0SsTaZBG9W_WA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1722452327; x=1723057127;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references: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=ci+Z4a0YKP17caeue+7YhZGGb5J9YaZJ5IBcjtorqxg=;\n\tb=Rosn+BhpIxF2B713T0S9pxuKKvG7g/QrMvBHOmViKvXpi/Ou47sYEPlylg2Z20W/0z\n\tmgAbA9MJAyRm0iwG+yZj8S6xtXC6zgHdNeBmkfn6VwAVKlKBWeACvYzArTtOlQ1qp7rC\n\tk7022Qa2kZ8zmAueNuIvuSnDFWcnhTL7J2VZEANnFqviLGIf0N19G2FD76Bfg1fKcdbv\n\t2agNcJp1olIG/zQkj8ayXPzY9N60g5HxV9twJXvzuVTjwDzO3pJBRzJMIi3VLpTsVoNq\n\tfjFZ+jf6cP8ifDLFy44oSpsxVGqO0N3jfB1Ahzz/hjbzdFDWy/ZA6PHa9pu5DX+qeE9u\n\tcbLA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCV5Jx+qDR8j+j0QWujhDpwwy4glXYzpmAzs9OCefB0k5Y8nzSyeL0K+BnuXtxXye4o5txt0mZ6TuHm7TuLpBQk8ZkvZ9z+YwjT47oR7RN1YLVjwrw==","X-Gm-Message-State":"AOJu0YwFlThAVyE8VxrfnVDmg6vSZscMROrCN5WARcklIDatBbdQG8Gv\n\tNWmjq6YuTcUPc/+bRnuh5CfI6Jk98rgxXTInSO39pTyzWT7AQNuIwCgXAz1NAuY6wOe7NJJwn/Z\n\t2nI/zjZDhwCkRyyXg3C8SE5gtWBH1tum1dkbEzyWtze1vQN/b6apzLZU33jv+HHANfy5IdhK/xu\n\tdmMx56dA==","X-Received":["by 2002:a17:907:7e9d:b0:a7a:c812:36c5 with SMTP id\n\ta640c23a62f3a-a7daf5bf476mr2870866b.46.1722452327281; \n\tWed, 31 Jul 2024 11:58:47 -0700 (PDT)","by 2002:a17:907:7e9d:b0:a7a:c812:36c5 with SMTP id\n\ta640c23a62f3a-a7daf5bf476mr2869066b.46.1722452326679; \n\tWed, 31 Jul 2024 11:58:46 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEV1tjGWNTt5gcLIrfG63MfPGfDj3JKpjjUon+kGWaAg4Jp1bh81XZKgIdhsC9LWU/bWQ8ecQ==","Message-ID":"<0e2f0914-42e4-49be-8078-c8c5eb962bc1@redhat.com>","Date":"Wed, 31 Jul 2024 20:58:45 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 1/4] libcamera: base: Add MemFd helper class","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240731135936.2105-1-laurent.pinchart@ideasonboard.com>\n\t<20240731135936.2105-2-laurent.pinchart@ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240731135936.2105-2-laurent.pinchart@ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","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\" "}}]