[libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce UniqueFD

[Jacopo Mondi]

Hi Laurent,

On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:
> From: Hirokazu Honda <hiroh at chromium.org>
>
> This introduces UniqueFD. It acts like unique_ptr to a file descriptor.

Have you considered subclassing FileDescriptor and restrict its
interface to support move-only semantic ?

>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v2:
>
> - Rename ScopedFD to UniqueFD
> - Inline most functions to allow compiler optimizations
> - Bring the API closer to unique_ptr<>
> - Add swap()
> - Documentation cleanups
> - Slip FileDescriptor constructor to separate patch
> - Fix isValid()
> ---
>  include/libcamera/base/meson.build |   1 +
>  include/libcamera/base/unique_fd.h |  69 ++++++++++++++++
>  src/libcamera/base/meson.build     |   1 +
>  src/libcamera/base/unique_fd.cpp   | 123 +++++++++++++++++++++++++++++
>  4 files changed, 194 insertions(+)
>  create mode 100644 include/libcamera/base/unique_fd.h
>  create mode 100644 src/libcamera/base/unique_fd.cpp
>
> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> index f73b00917409..cca374a769cc 100644
> --- a/include/libcamera/base/meson.build
> +++ b/include/libcamera/base/meson.build
> @@ -22,6 +22,7 @@ libcamera_base_headers = files([
>      'span.h',
>      'thread.h',
>      'timer.h',
> +    'unique_fd.h',
>      'utils.h',
>  ])
>
> diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h
> new file mode 100644
> index 000000000000..ae4d96b75797
> --- /dev/null
> +++ b/include/libcamera/base/unique_fd.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * unique_fd.h - File descriptor wrapper that owns a file descriptor.
> + */
> +
> +#pragma once
> +
> +#include <utility>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/compiler.h>
> +
> +namespace libcamera {
> +
> +class UniqueFD final
> +{
> +public:
> +	UniqueFD()
> +		: fd_(-1)
> +	{
> +	}
> +
> +	explicit UniqueFD(int fd)
> +		: fd_(fd)
> +	{
> +	}
> +
> +	UniqueFD(UniqueFD &&other)
> +		: fd_(other.release())
> +	{
> +	}
> +
> +	~UniqueFD()
> +	{
> +		reset();
> +	}
> +
> +	UniqueFD &operator=(UniqueFD &&other)
> +	{
> +		reset(other.release());
> +		return *this;
> +	}
> +
> +	__nodiscard int release()
> +	{
> +		int fd = fd_;
> +		fd_ = -1;
> +		return fd;
> +	}

Thanks, this will be great for fences

> +
> +	void reset(int fd = -1);
> +
> +	void swap(UniqueFD &other)
> +	{
> +		std::swap(fd_, other.fd_);
> +	}
> +
> +	int get() const { return fd_; }
> +	bool isValid() const { return fd_ >= 0; }

nit: const functions are usually put first

> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(UniqueFD)
> +
> +	int fd_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> index d5254fda9cbf..b0d85bc19245 100644
> --- a/src/libcamera/base/meson.build
> +++ b/src/libcamera/base/meson.build
> @@ -17,6 +17,7 @@ libcamera_base_sources = files([
>      'signal.cpp',
>      'thread.cpp',
>      'timer.cpp',
> +    'unique_fd.cpp',
>      'utils.cpp',
>  ])
>
> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
> new file mode 100644
> index 000000000000..83d6919cf623
> --- /dev/null
> +++ b/src/libcamera/base/unique_fd.cpp
> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * unique_fd.cpp - File descriptor wrapper that owns a file descriptor
> + */
> +
> +#include <libcamera/base/unique_fd.h>
> +
> +#include <unistd.h>
> +#include <utility>
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file base/unique_fd.h
> + * \brief File descriptor wrapper that owns a file descriptor
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(UniqueFD)
> +
> +/**
> + * \class UniqueFD
> + * \brief unique_ptr-like wrapper for a file descriptor
> + *
> + * The UniqueFD is a wrapper that owns and manages the lifetime of a file
> + * descriptor. It is constructed from a numerical file descriptor, and takes
> + * over its ownership. The file descriptor is closed when the UniqueFD is

Is talking about ownship with raw int correct ?
I would specify that the fd passed in is instead reset to -1

> + * destroyed, or when it is assigned another file descriptor with operator=()
> + * or reset().
> + */
> +
> +/**
> + * \fn UniqueFD::UniqueFD()
> + * \brief Construct a UniqueFD that owns no file descriptor
> + */
> +
> +/**
> + * \fn UniqueFD::UniqueFD(int fd)
> + * \brief Construct a UniqueFD that owns \a fd
> + * \param[in] fd A file descriptor to manage
> + */
> +
> +/**
> + * \fn UniqueFD::UniqueFD(UniqueFD &&other)
> + * \brief Move constructor, create a UniqueFD by taking over \a other
> + * \param[in] other The other UniqueFD
> + *
> + * Create a UniqueFD by transferring ownership of the file descriptor owned by
> + * \a other. Upon return, the \a other UniqueFD is invalid.
> + */
> +
> +/**
> + * \fn UniqueFD::~UniqueFD()
> + * \brief Destroy the UniqueFD instance
> + *
> + * If a file descriptor is owned, it is closed.
> + */
> +
> +/**
> + * \fn UniqueFD::operator=(UniqueFD &&other)
> + * \brief Move assignment operator, replace a UniqueFD by taking over \a other
> + * \param[in] other The other UniqueFD
> + *
> + * If this UniqueFD owns a file descriptor, the file descriptor is closed
> + * first. The file descriptor is then replaced by the one of \a other. Upon
> + * return, \a other is invalid.
> + *
> + * \return A reference to this UniqueFD
> + */
> +
> +/**
> + * \fn UniqueFD::release()
> + * \brief Release ownership of the file descriptor without closing it
> + *
> + * This function releases and returns the owned file descriptor without closing
> + * it. The caller owns the returned value and must take care of handling its
> + * life time to avoid file descriptor leakages. Upon return this UniqueFD is
> + * invalid.
> + *
> + * \return The managed file descriptor, or -1 if no file descriptor was owned
> + */
> +
> +/**
> + * \brief Replace the managed file descriptor
> + * \param[in] fd The new file descriptor to manage
> + *
> + * Close the managed file descriptor, if any, and replace it with the new \a fd.
> + *
> + * Self-resetting (passing an \a fd already managed by this instance) is invalid
> + * and results in undefined behaviour.
> + */
> +void UniqueFD::reset(int fd)

Has we enforce move semantics, shouldn't this take a && and reset fd
to -1 ?

> +{
> +	ASSERT(!isValid() || fd != fd_);
> +
> +	std::swap(fd, fd_);
> +
> +	if (fd >= 0)
> +		close(fd);

wouldn't it be more clear to just:

        if (fd_ > 0)
                fd_.close();
        fd_ = fd;

        /* with the above suggestion: */
        fd = -1;
?

> +}
> +
> +/**
> + * \fn UniqueFD::swap(UniqueFD &other)
> + * \brief Swap the managed file descriptors with another UniqueFD
> + * \param[in] other Another UniqueFD to swap the file descriptor with
> + */
> +
> +/**
> + * \fn UniqueFD::get()
> + * \brief Retrieve the managed file descriptor
> + * \return The managed file descriptor, or -1 if no file descriptor is owned
> + */
> +
> +/**
> + * \fn UniqueFD::isValid()
> + * \brief Check if the UniqueFD owns a valid file descriptor
> + * \return True if the UniqueFD owns a valid file descriptor, false otherwise
> + */
> +
> +} /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>