[libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce UniqueFD
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 30 03:09:41 CET 2021
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 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.
>
> 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list