[libcamera-devel] [PATCH] [WIP] libcamera: FileDescriptor: Make FileDescriptor own a fd

[Hirokazu Honda]

Multiple FileDescriptors can share the same file descriptor. It
leads that the ownership of a fd managed by the class is unclear.
This makes FileDescriptor own a fd, similar to std::unique_ptr.

Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
---
 include/libcamera/file_descriptor.h |  26 ++-----
 src/libcamera/file_descriptor.cpp   | 112 ++++------------------------
 2 files changed, 22 insertions(+), 116 deletions(-)

diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h
index d514aac7..aaaf7f10 100644
--- a/include/libcamera/file_descriptor.h
+++ b/include/libcamera/file_descriptor.h
@@ -13,34 +13,22 @@ namespace libcamera {
 
 class FileDescriptor final
 {
+	static constexpr int kInvalidFd = -1;
 public:
-	explicit FileDescriptor(const int &fd = -1);
-	explicit FileDescriptor(int &&fd);
-	FileDescriptor(const FileDescriptor &other);
+	explicit FileDescriptor(const int fd = kInvalidFd);
 	FileDescriptor(FileDescriptor &&other);
 	~FileDescriptor();
 
-	FileDescriptor &operator=(const FileDescriptor &other);
 	FileDescriptor &operator=(FileDescriptor &&other);
+	FileDescriptor &operator=(const FileDescriptor &other) = delete;
+	FileDescriptor(const FileDescriptor &other) = delete;
 
-	bool isValid() const { return fd_ != nullptr; }
-	int fd() const { return fd_ ? fd_->fd() : -1; }
+	bool isValid() const { return fd_ != kInvalidFd; }
+	int fd() const { return fd_; }
 	FileDescriptor dup() const;
 
 private:
-	class Descriptor
-	{
-	public:
-		Descriptor(int fd, bool duplicate);
-		~Descriptor();
-
-		int fd() const { return fd_; }
-
-	private:
-		int fd_;
-	};
-
-	std::shared_ptr<Descriptor> fd_;
+	int fd_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp
index 8b505ed3..5757aca0 100644
--- a/src/libcamera/file_descriptor.cpp
+++ b/src/libcamera/file_descriptor.cpp
@@ -7,6 +7,7 @@
 
 #include <libcamera/file_descriptor.h>
 
+#include <algorithm>
 #include <string.h>
 #include <unistd.h>
 #include <utility>
@@ -35,13 +36,7 @@ LOG_DEFINE_CATEGORY(FileDescriptor)
  * When constructed from a numerical file descriptor, the FileDescriptor
  * instance either duplicates or takes over the file descriptor:
  *
- * - The FileDescriptor(const int &) constructor duplicates the numerical file
- *   descriptor and wraps the duplicate in a Descriptor. The caller is
- *   responsible for closing the original file descriptor, and the value
- *   returned by fd() will be different from the value passed to the
- *   constructor.
- *
- * - The FileDescriptor(int &&) constructor takes over the numerical file
+ * - The FileDescriptor(const int &) constructor takes over the numerical file
  *   descriptor and wraps it in a Descriptor. The caller shall not touch the
  *   original file descriptor once the function returns, and the value returned
  *   by fd() will be identical to the value passed to the constructor.
@@ -70,56 +65,9 @@ LOG_DEFINE_CATEGORY(FileDescriptor)
  * If the \a fd is negative, the FileDescriptor is constructed as invalid and
  * the fd() method will return -1.
  */
-FileDescriptor::FileDescriptor(const int &fd)
-{
-	if (fd < 0)
-		return;
-
-	fd_ = std::make_shared<Descriptor>(fd, true);
-	if (fd_->fd() < 0)
-		fd_.reset();
-}
 
-/**
- * \brief Create a FileDescriptor taking ownership of a given \a fd
- * \param[in] fd File descriptor
- *
- * Construct a FileDescriptor from a numerical file descriptor by taking
- * ownership of the \a fd. The original \a fd is set to -1 and shall not be
- * touched by the caller anymore. In particular, the caller shall not close the
- * original \a fd manually. The duplicated file descriptor will be closed
- * automatically when all FileDescriptor instances that reference it are
- * destroyed.
- *
- * If the \a fd is negative, the FileDescriptor is constructed as invalid and
- * the fd() method will return -1.
- */
-FileDescriptor::FileDescriptor(int &&fd)
-{
-	if (fd < 0)
-		return;
-
-	fd_ = std::make_shared<Descriptor>(fd, false);
-	/*
-	 * The Descriptor constructor can't have failed here, as it took over
-	 * the fd without duplicating it. Just set the original fd to -1 to
-	 * implement move semantics.
-	 */
-	fd = -1;
-}
-
-/**
- * \brief Copy constructor, create a FileDescriptor from a copy of \a other
- * \param[in] other The other FileDescriptor
- *
- * Copying a FileDescriptor implicitly shares ownership of the wrapped file
- * descriptor. The original FileDescriptor is left untouched, and the caller is
- * responsible for destroying it when appropriate. The wrapped file descriptor
- * will be closed automatically when all FileDescriptor instances that
- * reference it are destroyed.
- */
-FileDescriptor::FileDescriptor(const FileDescriptor &other)
-	: fd_(other.fd_)
+FileDescriptor::FileDescriptor(const int fd)
+	: fd_(fd)
 {
 }
 
@@ -134,8 +82,9 @@ FileDescriptor::FileDescriptor(const FileDescriptor &other)
  * reference it are destroyed.
  */
 FileDescriptor::FileDescriptor(FileDescriptor &&other)
-	: fd_(std::move(other.fd_))
+	: fd_(other.fd_)
 {
+	other.fd_ = -1;
 }
 
 /**
@@ -147,27 +96,8 @@ FileDescriptor::FileDescriptor(FileDescriptor &&other)
  */
 FileDescriptor::~FileDescriptor()
 {
-}
-
-/**
- * \brief Copy assignment operator, replace the wrapped file descriptor with a
- * copy of \a other
- * \param[in] other The other FileDescriptor
- *
- * Copying a FileDescriptor creates a new reference to the wrapped file
- * descriptor owner by \a other. If \a other is invalid, *this will also be
- * invalid. The original FileDescriptor is left untouched, and the caller is
- * responsible for destroying it when appropriate. The wrapped file descriptor
- * will be closed automatically when all FileDescriptor instances that
- * reference it are destroyed.
- *
- * \return A reference to this FileDescriptor
- */
-FileDescriptor &FileDescriptor::operator=(const FileDescriptor &other)
-{
-	fd_ = other.fd_;
-
-	return *this;
+	if (isValid())
+		close(fd_);
 }
 
 /**
@@ -186,7 +116,7 @@ FileDescriptor &FileDescriptor::operator=(const FileDescriptor &other)
  */
 FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other)
 {
-	fd_ = std::move(other.fd_);
+	std::swap(fd_, other.fd_);
 
 	return *this;
 }
@@ -218,29 +148,17 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other)
  */
 FileDescriptor FileDescriptor::dup() const
 {
-	return FileDescriptor(fd());
-}
+	if (!isValid())
+		return FileDescriptor();
 
-FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
-{
-	if (!duplicate) {
-		fd_ = fd;
-		return;
-	}
-
-	/* Failing to dup() a fd should not happen and is fatal. */
-	fd_ = ::dup(fd);
-	if (fd_ == -1) {
+	int dupFd = ::dup(fd);
+	if (dupFd == -1) {
 		int ret = -errno;
 		LOG(FileDescriptor, Fatal)
 			<< "Failed to dup() fd: " << strerror(-ret);
+		return FileDescriptor();
 	}
-}
 
-FileDescriptor::Descriptor::~Descriptor()
-{
-	if (fd_ != -1)
-		close(fd_);
+	return FileDescriptor(dupFd);
 }
-
 } /* namespace libcamera */
-- 
2.31.1.295.g9ea45b61b8-goog