[{"id":3266,"web_url":"https://patchwork.libcamera.org/comment/3266/","msgid":"<20191216171551.GC4856@pendragon.ideasonboard.com>","date":"2019-12-16T17:15:51","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add FileDescriptor to\n\thelp pass numerical fds around","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Dec 16, 2019 at 01:10:28PM +0100, Niklas Söderlund wrote:\n> Add a helper to make it easier to pass file descriptors around. The\n> helper class duplicates the fd which decouples it from the original fd\n> which could be closed by its owner while the new FileDescriptor remains\n> valid.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/file_descriptor.h | 33 ++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/file_descriptor.cpp | 158 ++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 193 insertions(+)\n> create mode 100644 include/libcamera/file_descriptor.h\n> create mode 100644 src/libcamera/file_descriptor.cpp\n> \n> diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h\n> new file mode 100644\n> index 0000000000000000..e05f111d128e0ab1\n> --- /dev/null\n> +++ b/include/libcamera/file_descriptor.h\n> @@ -0,0 +1,33 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * file_descriptor.h - File descriptor wrapper\n> + */\n> +#ifndef __LIBCAMERA_FILE_DESCRIPTOR_H__\n> +#define __LIBCAMERA_FILE_DESCRIPTOR_H__\n> +\n> +namespace libcamera {\n> +\n> +class FileDescriptor final\n> +{\n> +public:\n> +\texplicit FileDescriptor(int fd = -1);\n> +\texplicit FileDescriptor(const FileDescriptor &other);\n> +\texplicit FileDescriptor(FileDescriptor &&other);\n> +\t~FileDescriptor();\n> +\n> +\tFileDescriptor &operator=(const FileDescriptor &other);\n> +\tFileDescriptor &operator=(FileDescriptor &&other);\n> +\n> +\tint fd() const { return fd_; }\n> +\n> +private:\n> +\tvoid duplicate(int fd);\n> +\n> +\tint fd_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_FILE_DESCRIPTOR_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 99abf06099407c1f..543e6773cc5158a0 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -6,6 +6,7 @@ libcamera_api = files([\n> 'controls.h',\n> 'event_dispatcher.h',\n> 'event_notifier.h',\n> + 'file_descriptor.h',\n> 'geometry.h',\n> 'logging.h',\n> 'object.h',\n> diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp\n> new file mode 100644\n> index 0000000000000000..fd87753b456fc3b7\n> --- /dev/null\n> +++ b/src/libcamera/file_descriptor.cpp\n> @@ -0,0 +1,158 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * file_descriptor.cpp - File descriptor wrapper\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file file_descriptor.h\n> + * \\brief File descriptor wrapper\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(FileDescriptor)\n> +\n> +/**\n> + * \\class FileDescriptor\n> + * \\brief RAII-style wrapper for file descriptors\n> + *\n> + * The FileDescriptor class wraps a file descriptor (expressed as a signed\n> + * integer) to provide an RAII-style mechanism for owning the file descriptor.\n> + * When constructed, the FileDescriptor instance duplicates a given file\n> + * descriptor and takes ownership of the duplicate as a resource. The copy\n> + * constructor and assignment operator duplicate the file descriptor, while the\n> + * move version of those methods move the resource and make the original\n\ns/version/versions/\n\n> + * FileDescriptor invalid. When the FileDescriptor is deleted, it closes the\n> + * file descriptor it owns, if any.\n> + */\n> +\n> +/**\n> + * \\brief Create a FileDescriptor wrapping a copy of a given \\a fd\n> + * \\param[in] fd File descriptor\n> + *\n> + * Constructing a FileDescriptor from a numerical file descriptor duplicates the\n> + * \\a fd and takes ownership of the copy. The original \\a fd is left untouched,\n> + * and the caller is responsible for closing it when appropriate. The duplicated\n> + * file descriptor will be closed automatically when the FileDescriptor instance\n> + * is destroyed.\n> + */\n> +FileDescriptor::FileDescriptor(int fd)\n> +\t: fd_(-1)\n> +{\n> +\tduplicate(fd);\n> +}\n> +\n> +/**\n> + * \\brief Copy constructor, create a FileDescriptor from a copy of \\a other\n> + * \\param[in] other The other FileDescriptor\n> + *\n> + * Constructing a FileDescriptor from another FileDescriptor duplicates the\n> + * wrapped numerical file descriptor and takes ownership of the copy. The\n> + * original FileDescriptor is left untouched, and the caller is responsible for\n> + * closing it when appropriate. The duplicated file descriptor will be closed\n> + * automatically when the FileDescriptor instance is destroyed.\n\ns/the FileDescriptor instance/this FileDescriptor instance/ to avoid any\nambiguity with \\a other ? Same for the move constructor below.\n\n> + */\n> +FileDescriptor::FileDescriptor(const FileDescriptor &other)\n> +\t: FileDescriptor(other.fd_)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Move constructor, create a FileDescriptor by taking over \\a other\n> + * \\param[in] other The other FileDescriptor\n> + *\n> + * Constructing a FileDescriptor by taking over a wrapped numerical file\n\ns/Constructing/Construct/\n\n> + * descriptor and taking ownership of it. The \\a other FileDescriptor is\n> + * invalidated and set to -1. The taken over file descriptor will be closed\n> + * automatically when the FileDescriptor instance is destroyed.\n> + */\n> +FileDescriptor::FileDescriptor(FileDescriptor &&other)\n> +\t: fd_(utils::exchange(other.fd_, -1))\n> +{\n\nYou could also have written this\n\n\tfd_ = other.fd_;\n\tother.fd_ = -1;\n\nand similarly below in operator=(), to avoid introducing\nstd::exchange(). Up to you.\n\n> +}\n> +\n\nHow about documenting the destructor too ?\n\n/**\n * \\brief Destroy the managed file descriptor\n *\n * If the managed file descriptor, as returned by fd(), is not equal to -1, the\n * file descriptor is closed.\n */\n\nThen we could remove the \"The file descriptor will be closed\nautomatically when the FileDescriptor instance is destroyed.\" sentence\nfrom all constructors, if desired.\n\n> +FileDescriptor::~FileDescriptor()\n> +{\n> +\tif (fd_ != -1)\n> +\t\tclose(fd_);\n> +}\n> +\n> +/**\n> + * \\brief Copy assignment operator, replace the wrapped file descriptor with a\n> + * duplicate from \\a other\n> + * \\param[in] other The other FileDescriptor\n> + *\n> + * Close the currently wrapped numerical file descriptor if any and duplicate\n\ns/currently wrapped numerical file descriptor/wrapped file descriptor/ ?\ns/ if any/, if any,/ of s/if any/(if any)/\n\nBoth comments apply to the move assignment operator below.\n\n> + * the file descriptor from \\a other. The original FileDescriptor is left\n\ns/original/\\a other/\n\n> + * untouched, and the caller is responsible for closing it when appropriate.\n\ns/closing/destroying/ as FileDescriptor has no close method ?\n\n> + * The duplicated file descriptor will be closed automatically when the\n> + * FileDescriptor instance is destroyed.\n> + *\n> + * \\return A reference to the FileDescriptor\n\ns/the FileDescriptor/this FileDescriptor/\n\n> + */\n> +FileDescriptor &FileDescriptor::operator=(const FileDescriptor &other)\n> +{\n> +\tif (fd_ != -1)\n> +\t\tclose(fd_);\n\nI would assign fd_ to -1 here to avoid depending on duplicate() doing\nso.\n\n> +\n> +\tduplicate(other.fd_);\n> +\n> +\treturn *this;\n> +}\n> +\n> +/**\n> + * \\brief Move assignment operator, replace the wrapped file descriptor by\n> + * taking over \\a other\n> + * \\param[in] other The other FileDescriptor\n> + *\n> + * Close the currently wrapped numerical file descriptor if any and take over\n> + * the file descriptor from \\a other. The \\a other FileDescriptor is invalidated\n> + * and set to -1. The taken over file descriptor will be closed automatically\n> + * when the FileDescriptor instance is destroyed.\n> + *\n> + * \\return A reference to the FileDescriptor\n\ns/the FileDescriptor/this FileDescriptor/\n\n> + */\n> +FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other)\n> +{\n> +\tif (fd_ != -1)\n> +\t\tclose(fd_);\n> +\n> +\tfd_ = utils::exchange(other.fd_, -1);\n> +\n> +\treturn *this;\n> +}\n> +\n> +/**\n> + * \\fn FileDescriptor::fd()\n> + * \\brief Retrieve the numerical file descriptor\n> + * \\return The numerical file descriptor, which may be -1 if the FileDescriptor\n> + * instance doesn't own a file descriptor\n> + */\n> +\n> +void FileDescriptor::duplicate(int fd)\n> +{\n> +\tif (fd < 0) {\n> +\t\tfd_ = -1;\n> +\t\treturn;\n> +\t}\n\nTo make this method more self-contained, I think it should either\nclose(fd_) if fd_ != -1 (removing the close from the copy assignment\noperator), or rely on fd_ being == -1 when called, thus removing the fd_\n= -1 assignment above. Otherwise duplicate() requires fd_ to be closed\nbut tolerates it being != -1, which is confusing.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\t/* Failing to dup() a fd should not happen and is fatal. */\n> +\tfd_ = dup(fd);\n> +\tif (fd_ == -1) {\n> +\t\tint ret = -errno;\n> +\t\tLOG(FileDescriptor, Fatal)\n> +\t\t\t<< \"Failed to dup() fd: \" << strerror(-ret);\n> +\t}\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index c4f965bd7413b37e..722c5bc15afe52ef 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n> 'event_dispatcher.cpp',\n> 'event_dispatcher_poll.cpp',\n> 'event_notifier.cpp',\n> + 'file_descriptor.cpp',\n> 'formats.cpp',\n> 'geometry.cpp',\n> 'ipa_context_wrapper.cpp',","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C809C601E4\n\tfor ;\n\tMon, 16 Dec 2019 18:16:01 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C46CA34;\n\tMon, 16 Dec 2019 18:16:01 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576516561;\n\tbh=u6kum3X+ne/BGWkrhqmOP1R3lzUN8KAwmOQYQ8L6sKM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K3VtIf5+tkHUr+VMyZCpYjRGCGTsXmRwqdjl4PzSUqB9tDtEOgmMnv4+nCZ17NQqf\n\tMQkEgVqn/tZZiy1mflu9l5Juh2ZZoklwQhpxTDJ8qOGlPjofMafWhu21PSpfvXOpAa\n\tQNsO+4IGrKOMur/ZLZ/qHUDds0HUcaNBpwvFY63U=","Date":"Mon, 16 Dec 2019 19:15:51 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191216171551.GC4856@pendragon.ideasonboard.com>","References":"<20191216121029.1048242-1-niklas.soderlund@ragnatech.se>\n\t<20191216121029.1048242-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191216121029.1048242-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add FileDescriptor to\n\thelp pass numerical fds around","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 16 Dec 2019 17:16:02 -0000"}}]