Message ID | 20211128235752.10836-5-laurent.pinchart@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 */
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 */