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

[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 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 ?

Not really, as the two are very different beasts.

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

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 */

-- 
Regards,

Laurent Pinchart