[PATCH 1/3] libcamera: base: Add MemFd helper class
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 31 10:37:30 CEST 2024
Quoting Laurent Pinchart (2024-07-31 00:27:06)
> libcamera creates memfds in two locations already, duplicating some
> code. Move the code to a new MemFd helper class.
>
Thanks, Moving this out to it's own class makes sense to me.
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/base/memfd.h | 34 +++++++++
> include/libcamera/base/meson.build | 1 +
> src/libcamera/base/memfd.cpp | 112 ++++++++++++++++++++++++++++
> src/libcamera/base/meson.build | 1 +
> src/libcamera/dma_buf_allocator.cpp | 46 +-----------
> src/libcamera/shared_mem_object.cpp | 21 ++----
> 6 files changed, 157 insertions(+), 58 deletions(-)
> create mode 100644 include/libcamera/base/memfd.h
> create mode 100644 src/libcamera/base/memfd.cpp
>
> diff --git a/include/libcamera/base/memfd.h b/include/libcamera/base/memfd.h
> new file mode 100644
> index 000000000000..b0edd2de5d83
> --- /dev/null
> +++ b/include/libcamera/base/memfd.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * Anonymous file creation
> + */
> +
> +#pragma once
> +
> +#include <stddef.h>
> +
> +#include <libcamera/base/flags.h>
> +#include <libcamera/base/unique_fd.h>
> +
> +namespace libcamera {
> +
> +class MemFd
> +{
> +public:
> + enum class Seal {
> + None = 0,
> + Shrink = (1 << 0),
> + Grow = (1 << 1),
> + };
> +
> + using Seals = Flags<Seal>;
> +
> + static UniqueFD create(const char *name, std::size_t size,
> + Seals seals = Seal::None);
> +};
> +
> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MemFd::Seal)
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> index bace25d56b13..2a0cee317204 100644
> --- a/include/libcamera/base/meson.build
> +++ b/include/libcamera/base/meson.build
> @@ -21,6 +21,7 @@ libcamera_base_private_headers = files([
> 'event_notifier.h',
> 'file.h',
> 'log.h',
> + 'memfd.h',
> 'message.h',
> 'mutex.h',
> 'private.h',
> diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp
> new file mode 100644
> index 000000000000..0d50e2d64638
> --- /dev/null
> +++ b/src/libcamera/base/memfd.cpp
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
Inconsistent with the header.
> + *
> + * Anonymous file creation
> + */
> +
> +#include <libcamera/base/memfd.h>
> +
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file base/memfd.h
> + * \brief Anonymous file creation
> + */
> +
> +/* uClibc doesn't provide the file sealing API. */
> +#ifndef __DOXYGEN__
> +#if not HAVE_FILE_SEALS
> +#define F_ADD_SEALS 1033
> +#define F_SEAL_SHRINK 0x0002
> +#define F_SEAL_GROW 0x0004
> +#endif
> +#endif
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(File)
> +
> +/**
> + * \class MemFd
> + * \brief Helper class to created anonymous files
> + */
> +
> +/**
> + * \enum MemFd::Seal
> + * \brief Seals for the MemFd::create() function
> + * \var MemFd::Seal::None
> + * \brief No seals (used as default value)
> + * \var MemFd::Seal::Shrink
> + * \brief Prevent the memfd from shrinking
> + * \var MemFd::Seal::Grow
> + * \brief Prevent the memfd from growing
> + */
> +
> +/**
> + * \typedef MemFd::Seals
> + * \brief A bitwise combination of MemFd::Seal values
> + */
> +
> +/**
> + * \brief Create an anonymous file
> + * \param[in] name The file name (displayed in symbolic links in /proc/self/fd/)
Curious: An anonymous file ... with a name ;-)
> + * \param[in] size The file size
> + * \param[in] seals The file seals
> + * \return The descriptor of the anonymous file is creation succeeded, or an
> + *
s/is/if/
> + * This function is a helper that wraps anonymous file (memfd) creation and
> + * sets the file size and optional seals.
> + *
> + * invalid UniqueFD otherwise
I think I saw Milan already commented here - but this doxy needs fixing
> + */
> +UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals)
> +{
> +#if HAVE_MEMFD_CREATE
> + int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#else
> + int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> +#endif
> + if (ret < 0) {
> + ret = errno;
> + LOG(File, Error)
> + << "Failed to allocate memfd storage for " << name
> + << ": " << strerror(ret);
> + return {};
> + }
> +
> + UniqueFD memfd(ret);
> +
> + ret = ftruncate(memfd.get(), size);
> + if (ret < 0) {
> + ret = errno;
> + LOG(File, Error)
> + << "Failed to set memfd size for " << name
> + << ": " << strerror(ret);
> + return {};
> + }
> +
> + if (seals) {
> + int fileSeals = (seals & Seal::Shrink ? F_SEAL_SHRINK : 0)
> + | (seals & Seal::Grow ? F_SEAL_GROW : 0);
> +
> + ret = fcntl(memfd.get(), F_ADD_SEALS, fileSeals);
> + if (ret < 0) {
> + ret = errno;
> + LOG(File, Error)
> + << "Failed to seal the memfd for " << name
> + << ": " << strerror(ret);
> + return {};
> + }
> + }
> +
> + return memfd;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> index 7a7fd7e4ca87..4a228d335ba4 100644
> --- a/src/libcamera/base/meson.build
> +++ b/src/libcamera/base/meson.build
> @@ -10,6 +10,7 @@ libcamera_base_sources = files([
> 'file.cpp',
> 'flags.cpp',
> 'log.cpp',
> + 'memfd.cpp',
> 'message.cpp',
> 'mutex.cpp',
> 'object.cpp',
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index c06eca7d04ec..be6efb89fbb7 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -13,7 +13,6 @@
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> #include <sys/stat.h>
> -#include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> @@ -22,6 +21,7 @@
> #include <linux/udmabuf.h>
>
> #include <libcamera/base/log.h>
> +#include <libcamera/base/memfd.h>
>
> /**
> * \file dma_buf_allocator.cpp
> @@ -126,54 +126,16 @@ DmaBufAllocator::~DmaBufAllocator() = default;
> * \brief Check if the DmaBufAllocator instance is valid
> * \return True if the DmaBufAllocator is valid, false otherwise
> */
> -
> -/* uClibc doesn't provide the file sealing API. */
> -#ifndef __DOXYGEN__
> -#if not HAVE_FILE_SEALS
> -#define F_ADD_SEALS 1033
> -#define F_SEAL_SHRINK 0x0002
> -#endif
> -#endif
> -
> UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
> {
> /* Size must be a multiple of the page size. Round it up. */
> std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> size = (size + pageMask) & ~pageMask;
>
> -#if HAVE_MEMFD_CREATE
> - int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> -#else
> - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC);
> -#endif
> - if (ret < 0) {
> - ret = errno;
> - LOG(DmaBufAllocator, Error)
> - << "Failed to allocate memfd storage for " << name
> - << ": " << strerror(ret);
> - return {};
> - }
> -
> - UniqueFD memfd(ret);
> -
> - ret = ftruncate(memfd.get(), size);
> - if (ret < 0) {
> - ret = errno;
> - LOG(DmaBufAllocator, Error)
> - << "Failed to set memfd size for " << name
> - << ": " << strerror(ret);
> - return {};
> - }
> -
> /* udmabuf dma-buffers *must* have the F_SEAL_SHRINK seal. */
> - ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
> - if (ret < 0) {
> - ret = errno;
> - LOG(DmaBufAllocator, Error)
> - << "Failed to seal the memfd for " << name
> - << ": " << strerror(ret);
> + UniqueFD memfd = MemFd::create(name, size, MemFd::Seal::Shrink);
> + if (!memfd.isValid())
Well that all tidies up nicely.
With the small fixes
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> return {};
> - }
>
> struct udmabuf_create create;
>
> @@ -182,7 +144,7 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size)
> create.offset = 0;
> create.size = size;
>
> - ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> + int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create);
> if (ret < 0) {
> ret = errno;
> LOG(DmaBufAllocator, Error)
> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
> index 809fbdaf95de..022645e71a35 100644
> --- a/src/libcamera/shared_mem_object.cpp
> +++ b/src/libcamera/shared_mem_object.cpp
> @@ -13,9 +13,9 @@
> #include <stddef.h>
> #include <stdint.h>
> #include <sys/mman.h>
> -#include <sys/syscall.h>
> #include <sys/types.h>
> -#include <unistd.h>
> +
> +#include <libcamera/base/memfd.h>
>
> /**
> * \file shared_mem_object.cpp
> @@ -58,22 +58,11 @@ SharedMem::SharedMem() = default;
> */
> SharedMem::SharedMem(const std::string &name, std::size_t size)
> {
> -#if HAVE_MEMFD_CREATE
> - int fd = memfd_create(name.c_str(), MFD_CLOEXEC);
> -#else
> - int fd = syscall(SYS_memfd_create, name.c_str(), MFD_CLOEXEC);
> -#endif
> - if (fd < 0)
> + UniqueFD memfd = MemFd::create(name.c_str(), size);
> + if (!memfd.isValid())
> return;
>
> - fd_ = SharedFD(std::move(fd));
> - if (!fd_.isValid())
> - return;
> -
> - if (ftruncate(fd_.get(), size) < 0) {
> - fd_ = SharedFD();
> - return;
> - }
> + fd_ = SharedFD(std::move(memfd));
>
> void *mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> fd_.get(), 0);
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list