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

14819 diff mbox series

From: Hirokazu Honda <hiroh@chromium.org>

This introduces UniqueFD. It acts like unique_ptr to a file descriptor.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Hi Laurent, thank you for the patch.

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> From: Hirokazu Honda <hiroh@chromium.org>
>
> This introduces UniqueFD. It acts like unique_ptr to a file descriptor.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good to me.

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> ---
> 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;
> +       }
> +
> +       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; }
> +
> +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
> + * 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)
> +{
> +       ASSERT(!isValid() || fd != fd_);
> +
> +       std::swap(fd, fd_);
> +
> +       if (fd >= 0)
> +               close(fd);
> +}
> +
> +/**
> + * \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
>

Hi Laurent,

On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:
> From: Hirokazu Honda <hiroh@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@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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
>

Hi Jacopo,

On Mon, Nov 29, 2021 at 03:58:26PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:
> > From: Hirokazu Honda <hiroh@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 ?

Not really, as the two are very different beasts.

> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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

That's why I sent this series ;-)

> > +
> > +	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

I've ordered the functions is in the std::unique_ptr<> documentation,
(with isValid() replacing the bool operator), but I can change that if
desired.

> > +
> > +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 think talking about file descriptor ownership makes sense, as that's
what it's all about. It's not about owning the variable passed to the
constructor.

> I would specify that the fd passed in is instead reset to -1

It isn't though. I have thought about it, and researched why
std::unique_ptr<> had a constructor taking a pointer, not an rvalue
reference to the pointer. It seems the C++ authors have decided that it
wouldn't really bring much additional safety to automatically set the
pointer to null. Maybe it should be different here ? I'm not sure.

> > + * 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 ?

Same reasoning as for the constructor. We should use rvalue references
for both or none.

> > +{
> > +	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;
> ?

I went for swap() to match the behaviour of std::unique_ptr<>, but there
may be no need to in this case. If you prefer the above, I can change
it.

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

Hi Laurent

On Mon, Nov 29, 2021 at 06:41:10PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Nov 29, 2021 at 03:58:26PM +0100, Jacopo Mondi wrote:
> > On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:
> > > From: Hirokazu Honda <hiroh@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 ?
>
> Not really, as the two are very different beasts.
>
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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
>
> That's why I sent this series ;-)
>
> > > +
> > > +	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
>
> I've ordered the functions is in the std::unique_ptr<> documentation,
> (with isValid() replacing the bool operator), but I can change that if
> desired.
>

Not a big deal, but usually in the code base I've noticed that (not
that I recall we ever really established a rule about that)

> > > +
> > > +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 think talking about file descriptor ownership makes sense, as that's
> what it's all about. It's not about owning the variable passed to the
> constructor.
>
> > I would specify that the fd passed in is instead reset to -1
>
> It isn't though. I have thought about it, and researched why

I noticed later and suggested that in the review of a later patch :)

> std::unique_ptr<> had a constructor taking a pointer, not an rvalue
> reference to the pointer. It seems the C++ authors have decided that it
> wouldn't really bring much additional safety to automatically set the
> pointer to null. Maybe it should be different here ? I'm not sure.

Not sure, but if the fd argument stays valid, users might be tempted
to close it.

>
> > > + * 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 ?
>
> Same reasoning as for the constructor. We should use rvalue references
> for both or none.
>

I agree they should be the same, that's for sure.

On the semantic, I always thought that we should have had this class
take a && and reset to -1 the argument, but now that I think about
that it's mostly because I was thinking about subclassing
FileDescriptor and only restrict it to use the move contructor.

As this class doesn't have other constructors, passing the parameter
by value is ok. I like less the fact the parameter stays valid after
being 'moved' to UniqueFD (as the class 'owns' the fd, I would expect
I have to move it..) for the above said reasons.

On one side we could have an

        int filedesc = ....;
        UniqueFD fd(filedesc);
        close(filedesc);

On the other side

        int filedesc = ....;
        UniqueFD fd(std::move(filedesc));

is indeed more clunky

For sake of correctness I would go for the second, but I recognize my
motivations are a bit fragile, so I won't push!

Thanks
  j


> > > +{
> > > +	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;
> > ?
>
> I went for swap() to match the behaviour of std::unique_ptr<>, but there
> may be no need to in this case. If you prefer the above, I can change
> it.
>
> > > +}
> > > +
> > > +/**
> > > + * \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

Hi Jacopo,

On Mon, Nov 29, 2021 at 06:21:53PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 29, 2021 at 06:41:10PM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 29, 2021 at 03:58:26PM +0100, Jacopo Mondi wrote:
> > > On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:
> > > > From: Hirokazu Honda <hiroh@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 ?
> >
> > Not really, as the two are very different beasts.
> >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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
> >
> > That's why I sent this series ;-)
> >
> > > > +
> > > > +	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
> >
> > I've ordered the functions is in the std::unique_ptr<> documentation,
> > (with isValid() replacing the bool operator), but I can change that if
> > desired.
> 
> Not a big deal, but usually in the code base I've noticed that (not
> that I recall we ever really established a rule about that)
> 
> > > > +
> > > > +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 think talking about file descriptor ownership makes sense, as that's
> > what it's all about. It's not about owning the variable passed to the
> > constructor.
> >
> > > I would specify that the fd passed in is instead reset to -1
> >
> > It isn't though. I have thought about it, and researched why
> 
> I noticed later and suggested that in the review of a later patch :)
> 
> > std::unique_ptr<> had a constructor taking a pointer, not an rvalue
> > reference to the pointer. It seems the C++ authors have decided that it
> > wouldn't really bring much additional safety to automatically set the
> > pointer to null. Maybe it should be different here ? I'm not sure.
> 
> Not sure, but if the fd argument stays valid, users might be tempted
> to close it.
>
> > > > + * 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 ?
> >
> > Same reasoning as for the constructor. We should use rvalue references
> > for both or none.
> 
> I agree they should be the same, that's for sure.
> 
> On the semantic, I always thought that we should have had this class
> take a && and reset to -1 the argument, but now that I think about
> that it's mostly because I was thinking about subclassing
> FileDescriptor and only restrict it to use the move contructor.
> 
> As this class doesn't have other constructors, passing the parameter
> by value is ok. I like less the fact the parameter stays valid after
> being 'moved' to UniqueFD (as the class 'owns' the fd, I would expect
> I have to move it..) for the above said reasons.
> 
> On one side we could have an
> 
>         int filedesc = ....;
>         UniqueFD fd(filedesc);
>         close(filedesc);
> 
> On the other side
> 
>         int filedesc = ....;
>         UniqueFD fd(std::move(filedesc));
> 
> is indeed more clunky
> 
> For sake of correctness I would go for the second, but I recognize my
> motivations are a bit fragile, so I won't push!

I'll include this change in v4 as part of a separate patch, we can
discuss it there.

> > > > +{
> > > > +	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;
> > > ?
> >
> > I went for swap() to match the behaviour of std::unique_ptr<>, but there
> > may be no need to in this case. If you prefer the above, I can change
> > it.
> >
> > > > +}
> > > > +
> > > > +/**
> > > > + * \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 */