[1/3] libcamera: base: Add MemFd helper class
diff mbox series

Message ID 20240730232708.17399-2-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Address soft ISP file seal TODO item
Related show

Commit Message

Laurent Pinchart July 30, 2024, 11:27 p.m. UTC 
libcamera creates memfds in two locations already, duplicating some
code. Move the code to a new MemFd helper class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Comments

Milan Zamazal July 31, 2024, 7:23 a.m. UTC | #1 
Hi Laurent,

thank you for the cleanup.

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

> libcamera creates memfds in two locations already, duplicating some
> code. Move the code to a new MemFd helper class.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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.
> + *
> + * 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

You probably meant s/created/create/ ?

> + */
> +
> +/**
> + * \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/)
> + * \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

This line should be attached to \return above.

With the typos fixed:

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> + */
> +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())
>  		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);
Kieran Bingham July 31, 2024, 8:37 a.m. UTC | #2 
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@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@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
>

Patch
diff mbox series 

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.
+ *
+ * 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/)
+ * \param[in] size The file size
+ * \param[in] seals The file seals
+ * \return The descriptor of the anonymous file is creation succeeded, or an
+ *
+ * This function is a helper that wraps anonymous file (memfd) creation and
+ * sets the file size and optional seals.
+ *
+ * invalid UniqueFD otherwise
+ */
+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())
 		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);