[{"id":21317,"web_url":"https://patchwork.libcamera.org/comment/21317/","msgid":"","date":"2021-11-29T13:20:48","subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart\n wrote:\n>\n> From: Hirokazu Honda \n>\n> This introduces UniqueFD. It acts like unique_ptr to a file descriptor.\n>\n> Signed-off-by: Hirokazu Honda \n> Signed-off-by: Laurent Pinchart \n\nLooks good to me.\n\nReviewed-by: Hirokazu Honda \n> ---\n> Changes since v2:\n>\n> - Rename ScopedFD to UniqueFD\n> - Inline most functions to allow compiler optimizations\n> - Bring the API closer to unique_ptr<>\n> - Add swap()\n> - Documentation cleanups\n> - Slip FileDescriptor constructor to separate patch\n> - Fix isValid()\n> ---\n> include/libcamera/base/meson.build | 1 +\n> include/libcamera/base/unique_fd.h | 69 ++++++++++++++++\n> src/libcamera/base/meson.build | 1 +\n> src/libcamera/base/unique_fd.cpp | 123 +++++++++++++++++++++++++++++\n> 4 files changed, 194 insertions(+)\n> create mode 100644 include/libcamera/base/unique_fd.h\n> create mode 100644 src/libcamera/base/unique_fd.cpp\n>\n> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> index f73b00917409..cca374a769cc 100644\n> --- a/include/libcamera/base/meson.build\n> +++ b/include/libcamera/base/meson.build\n> @@ -22,6 +22,7 @@ libcamera_base_headers = files([\n> 'span.h',\n> 'thread.h',\n> 'timer.h',\n> + 'unique_fd.h',\n> 'utils.h',\n> ])\n>\n> diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> new file mode 100644\n> index 000000000000..ae4d96b75797\n> --- /dev/null\n> +++ b/include/libcamera/base/unique_fd.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * unique_fd.h - File descriptor wrapper that owns a file descriptor.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class UniqueFD final\n> +{\n> +public:\n> + UniqueFD()\n> + : fd_(-1)\n> + {\n> + }\n> +\n> + explicit UniqueFD(int fd)\n> + : fd_(fd)\n> + {\n> + }\n> +\n> + UniqueFD(UniqueFD &&other)\n> + : fd_(other.release())\n> + {\n> + }\n> +\n> + ~UniqueFD()\n> + {\n> + reset();\n> + }\n> +\n> + UniqueFD &operator=(UniqueFD &&other)\n> + {\n> + reset(other.release());\n> + return *this;\n> + }\n> +\n> + __nodiscard int release()\n> + {\n> + int fd = fd_;\n> + fd_ = -1;\n> + return fd;\n> + }\n> +\n> + void reset(int fd = -1);\n> +\n> + void swap(UniqueFD &other)\n> + {\n> + std::swap(fd_, other.fd_);\n> + }\n> +\n> + int get() const { return fd_; }\n> + bool isValid() const { return fd_ >= 0; }\n> +\n> +private:\n> + LIBCAMERA_DISABLE_COPY(UniqueFD)\n> +\n> + int fd_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> index d5254fda9cbf..b0d85bc19245 100644\n> --- a/src/libcamera/base/meson.build\n> +++ b/src/libcamera/base/meson.build\n> @@ -17,6 +17,7 @@ libcamera_base_sources = files([\n> 'signal.cpp',\n> 'thread.cpp',\n> 'timer.cpp',\n> + 'unique_fd.cpp',\n> 'utils.cpp',\n> ])\n>\n> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> new file mode 100644\n> index 000000000000..83d6919cf623\n> --- /dev/null\n> +++ b/src/libcamera/base/unique_fd.cpp\n> @@ -0,0 +1,123 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * unique_fd.cpp - File descriptor wrapper that owns a file descriptor\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file base/unique_fd.h\n> + * \\brief File descriptor wrapper that owns a file descriptor\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(UniqueFD)\n> +\n> +/**\n> + * \\class UniqueFD\n> + * \\brief unique_ptr-like wrapper for a file descriptor\n> + *\n> + * The UniqueFD is a wrapper that owns and manages the lifetime of a file\n> + * descriptor. It is constructed from a numerical file descriptor, and takes\n> + * over its ownership. The file descriptor is closed when the UniqueFD is\n> + * destroyed, or when it is assigned another file descriptor with operator=()\n> + * or reset().\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::UniqueFD()\n> + * \\brief Construct a UniqueFD that owns no file descriptor\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::UniqueFD(int fd)\n> + * \\brief Construct a UniqueFD that owns \\a fd\n> + * \\param[in] fd A file descriptor to manage\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::UniqueFD(UniqueFD &&other)\n> + * \\brief Move constructor, create a UniqueFD by taking over \\a other\n> + * \\param[in] other The other UniqueFD\n> + *\n> + * Create a UniqueFD by transferring ownership of the file descriptor owned by\n> + * \\a other. Upon return, the \\a other UniqueFD is invalid.\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::~UniqueFD()\n> + * \\brief Destroy the UniqueFD instance\n> + *\n> + * If a file descriptor is owned, it is closed.\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::operator=(UniqueFD &&other)\n> + * \\brief Move assignment operator, replace a UniqueFD by taking over \\a other\n> + * \\param[in] other The other UniqueFD\n> + *\n> + * If this UniqueFD owns a file descriptor, the file descriptor is closed\n> + * first. The file descriptor is then replaced by the one of \\a other. Upon\n> + * return, \\a other is invalid.\n> + *\n> + * \\return A reference to this UniqueFD\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::release()\n> + * \\brief Release ownership of the file descriptor without closing it\n> + *\n> + * This function releases and returns the owned file descriptor without closing\n> + * it. The caller owns the returned value and must take care of handling its\n> + * life time to avoid file descriptor leakages. Upon return this UniqueFD is\n> + * invalid.\n> + *\n> + * \\return The managed file descriptor, or -1 if no file descriptor was owned\n> + */\n> +\n> +/**\n> + * \\brief Replace the managed file descriptor\n> + * \\param[in] fd The new file descriptor to manage\n> + *\n> + * Close the managed file descriptor, if any, and replace it with the new \\a fd.\n> + *\n> + * Self-resetting (passing an \\a fd already managed by this instance) is invalid\n> + * and results in undefined behaviour.\n> + */\n> +void UniqueFD::reset(int fd)\n> +{\n> + ASSERT(!isValid() || fd != fd_);\n> +\n> + std::swap(fd, fd_);\n> +\n> + if (fd >= 0)\n> + close(fd);\n> +}\n> +\n> +/**\n> + * \\fn UniqueFD::swap(UniqueFD &other)\n> + * \\brief Swap the managed file descriptors with another UniqueFD\n> + * \\param[in] other Another UniqueFD to swap the file descriptor with\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::get()\n> + * \\brief Retrieve the managed file descriptor\n> + * \\return The managed file descriptor, or -1 if no file descriptor is owned\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::isValid()\n> + * \\brief Check if the UniqueFD owns a valid file descriptor\n> + * \\return True if the UniqueFD owns a valid file descriptor, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10A38BDB13\n\tfor ;\n\tMon, 29 Nov 2021 13:21:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 35C1E605A6;\n\tMon, 29 Nov 2021 14:21:02 +0100 (CET)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5328960592\n\tfor ;\n\tMon, 29 Nov 2021 14:21:01 +0100 (CET)","by mail-ed1-x536.google.com with SMTP id w1so71928133edc.6\n\tfor ;\n\tMon, 29 Nov 2021 05:21:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"aF0EgOCp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=XqI9ewuC3IhxewoVwvX9+a7ZFxHWAj2smrWL1Xpsf24=;\n\tb=aF0EgOCpEXXbXUpkqLTjcHQXjCQ4+/90q3ZwqLXGRxjbDaJEqKs7HPDC6tnwQQMT6N\n\tKHFRS9biKm5tqLmU7qDTOQHhYludqzwjsDXU2tFfKVBahlOnj2LDapCdChHCadjE4UzW\n\t6liIgIsyTmUC6ca+/O8deHqv05EhOgahMLRpA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=XqI9ewuC3IhxewoVwvX9+a7ZFxHWAj2smrWL1Xpsf24=;\n\tb=RqUIiyk81Eg/OkET0eBuG90W9eC5E3comvBPeLL0rO/HYrdfm7k8Dj3j/zcNeQqyiS\n\tX8gidFiaqnJUioekklZPRKTC0iceXa+lGk1OspYWmZHox62Gz0E1t2WiCcO1n+f/kRSU\n\tDW/ZO+55ectCBZp8yWQyklyPJ+YUJ/LeMwSupnXcDh8x1O6uoRwCr+VqJpArjH9WGA0N\n\t97x1j+Of/ePL1uGCtjdSr8FghVEkE4sUgUlrEf00W5r9ZIPIuNac7Gtamok+emx9HQZ8\n\tUbH7Jvw1UskbvRpZle2nBWj5QYawJrkarDdWVGmpAG+6sFKn0b9S9gTos71IG7iyHFAn\n\tK2vg==","X-Gm-Message-State":"AOAM5306J8DXtjA6Ierg0lELex7tpwnhZEnexZ1pngYhNPuLStR/ne5f\n\t4NYlBQz9VLQ4Ei+GhsFzHHSNJLinPg3M00PuFzxMhg==","X-Google-Smtp-Source":"ABdhPJwHnATL+BGyqGOhrH/yAbRqCxYJZP8+/qB+8Bv1bt1bNs6Uujp9yfgM01lGw9nJWUIgF2w2gcX0hOnfVaJZaw8=","X-Received":"by 2002:a17:906:7688:: with SMTP id\n\to8mr15107496ejm.291.1638192060746; \n\tMon, 29 Nov 2021 05:21:00 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 22:20:48 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21333,"web_url":"https://patchwork.libcamera.org/comment/21333/","msgid":"<20211129145826.phr7faw6okt5yiwg@uno.localdomain>","date":"2021-11-29T14:58:26","subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:\n> From: Hirokazu Honda \n>\n> This introduces UniqueFD. It acts like unique_ptr to a file descriptor.\n\nHave you considered subclassing FileDescriptor and restrict its\ninterface to support move-only semantic ?\n\n>\n> Signed-off-by: Hirokazu Honda \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v2:\n>\n> - Rename ScopedFD to UniqueFD\n> - Inline most functions to allow compiler optimizations\n> - Bring the API closer to unique_ptr<>\n> - Add swap()\n> - Documentation cleanups\n> - Slip FileDescriptor constructor to separate patch\n> - Fix isValid()\n> ---\n> include/libcamera/base/meson.build | 1 +\n> include/libcamera/base/unique_fd.h | 69 ++++++++++++++++\n> src/libcamera/base/meson.build | 1 +\n> src/libcamera/base/unique_fd.cpp | 123 +++++++++++++++++++++++++++++\n> 4 files changed, 194 insertions(+)\n> create mode 100644 include/libcamera/base/unique_fd.h\n> create mode 100644 src/libcamera/base/unique_fd.cpp\n>\n> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> index f73b00917409..cca374a769cc 100644\n> --- a/include/libcamera/base/meson.build\n> +++ b/include/libcamera/base/meson.build\n> @@ -22,6 +22,7 @@ libcamera_base_headers = files([\n> 'span.h',\n> 'thread.h',\n> 'timer.h',\n> + 'unique_fd.h',\n> 'utils.h',\n> ])\n>\n> diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> new file mode 100644\n> index 000000000000..ae4d96b75797\n> --- /dev/null\n> +++ b/include/libcamera/base/unique_fd.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * unique_fd.h - File descriptor wrapper that owns a file descriptor.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class UniqueFD final\n> +{\n> +public:\n> +\tUniqueFD()\n> +\t\t: fd_(-1)\n> +\t{\n> +\t}\n> +\n> +\texplicit UniqueFD(int fd)\n> +\t\t: fd_(fd)\n> +\t{\n> +\t}\n> +\n> +\tUniqueFD(UniqueFD &&other)\n> +\t\t: fd_(other.release())\n> +\t{\n> +\t}\n> +\n> +\t~UniqueFD()\n> +\t{\n> +\t\treset();\n> +\t}\n> +\n> +\tUniqueFD &operator=(UniqueFD &&other)\n> +\t{\n> +\t\treset(other.release());\n> +\t\treturn *this;\n> +\t}\n> +\n> +\t__nodiscard int release()\n> +\t{\n> +\t\tint fd = fd_;\n> +\t\tfd_ = -1;\n> +\t\treturn fd;\n> +\t}\n\nThanks, this will be great for fences\n\n> +\n> +\tvoid reset(int fd = -1);\n> +\n> +\tvoid swap(UniqueFD &other)\n> +\t{\n> +\t\tstd::swap(fd_, other.fd_);\n> +\t}\n> +\n> +\tint get() const { return fd_; }\n> +\tbool isValid() const { return fd_ >= 0; }\n\nnit: const functions are usually put first\n\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(UniqueFD)\n> +\n> +\tint fd_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> index d5254fda9cbf..b0d85bc19245 100644\n> --- a/src/libcamera/base/meson.build\n> +++ b/src/libcamera/base/meson.build\n> @@ -17,6 +17,7 @@ libcamera_base_sources = files([\n> 'signal.cpp',\n> 'thread.cpp',\n> 'timer.cpp',\n> + 'unique_fd.cpp',\n> 'utils.cpp',\n> ])\n>\n> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> new file mode 100644\n> index 000000000000..83d6919cf623\n> --- /dev/null\n> +++ b/src/libcamera/base/unique_fd.cpp\n> @@ -0,0 +1,123 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * unique_fd.cpp - File descriptor wrapper that owns a file descriptor\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file base/unique_fd.h\n> + * \\brief File descriptor wrapper that owns a file descriptor\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(UniqueFD)\n> +\n> +/**\n> + * \\class UniqueFD\n> + * \\brief unique_ptr-like wrapper for a file descriptor\n> + *\n> + * The UniqueFD is a wrapper that owns and manages the lifetime of a file\n> + * descriptor. It is constructed from a numerical file descriptor, and takes\n> + * over its ownership. The file descriptor is closed when the UniqueFD is\n\nIs talking about ownship with raw int correct ?\nI would specify that the fd passed in is instead reset to -1\n\n> + * destroyed, or when it is assigned another file descriptor with operator=()\n> + * or reset().\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::UniqueFD()\n> + * \\brief Construct a UniqueFD that owns no file descriptor\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::UniqueFD(int fd)\n> + * \\brief Construct a UniqueFD that owns \\a fd\n> + * \\param[in] fd A file descriptor to manage\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::UniqueFD(UniqueFD &&other)\n> + * \\brief Move constructor, create a UniqueFD by taking over \\a other\n> + * \\param[in] other The other UniqueFD\n> + *\n> + * Create a UniqueFD by transferring ownership of the file descriptor owned by\n> + * \\a other. Upon return, the \\a other UniqueFD is invalid.\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::~UniqueFD()\n> + * \\brief Destroy the UniqueFD instance\n> + *\n> + * If a file descriptor is owned, it is closed.\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::operator=(UniqueFD &&other)\n> + * \\brief Move assignment operator, replace a UniqueFD by taking over \\a other\n> + * \\param[in] other The other UniqueFD\n> + *\n> + * If this UniqueFD owns a file descriptor, the file descriptor is closed\n> + * first. The file descriptor is then replaced by the one of \\a other. Upon\n> + * return, \\a other is invalid.\n> + *\n> + * \\return A reference to this UniqueFD\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::release()\n> + * \\brief Release ownership of the file descriptor without closing it\n> + *\n> + * This function releases and returns the owned file descriptor without closing\n> + * it. The caller owns the returned value and must take care of handling its\n> + * life time to avoid file descriptor leakages. Upon return this UniqueFD is\n> + * invalid.\n> + *\n> + * \\return The managed file descriptor, or -1 if no file descriptor was owned\n> + */\n> +\n> +/**\n> + * \\brief Replace the managed file descriptor\n> + * \\param[in] fd The new file descriptor to manage\n> + *\n> + * Close the managed file descriptor, if any, and replace it with the new \\a fd.\n> + *\n> + * Self-resetting (passing an \\a fd already managed by this instance) is invalid\n> + * and results in undefined behaviour.\n> + */\n> +void UniqueFD::reset(int fd)\n\nHas we enforce move semantics, shouldn't this take a && and reset fd\nto -1 ?\n\n> +{\n> +\tASSERT(!isValid() || fd != fd_);\n> +\n> +\tstd::swap(fd, fd_);\n> +\n> +\tif (fd >= 0)\n> +\t\tclose(fd);\n\nwouldn't it be more clear to just:\n\n if (fd_ > 0)\n fd_.close();\n fd_ = fd;\n\n /* with the above suggestion: */\n fd = -1;\n?\n\n> +}\n> +\n> +/**\n> + * \\fn UniqueFD::swap(UniqueFD &other)\n> + * \\brief Swap the managed file descriptors with another UniqueFD\n> + * \\param[in] other Another UniqueFD to swap the file descriptor with\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::get()\n> + * \\brief Retrieve the managed file descriptor\n> + * \\return The managed file descriptor, or -1 if no file descriptor is owned\n> + */\n> +\n> +/**\n> + * \\fn UniqueFD::isValid()\n> + * \\brief Check if the UniqueFD owns a valid file descriptor\n> + * \\return True if the UniqueFD owns a valid file descriptor, false otherwise\n> + */\n> +\n> +} /* namespace libcamera */\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 54013BDB13\n\tfor ;\n\tMon, 29 Nov 2021 14:57:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7666B605A5;\n\tMon, 29 Nov 2021 15:57:35 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28BAD6059E\n\tfor ;\n\tMon, 29 Nov 2021 15:57:34 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id B2A9A2000A;\n\tMon, 29 Nov 2021 14:57:33 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 15:58:26 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129145826.phr7faw6okt5yiwg@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21353,"web_url":"https://patchwork.libcamera.org/comment/21353/","msgid":"","date":"2021-11-29T16:41:10","subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 29, 2021 at 03:58:26PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:\n> > From: Hirokazu Honda \n> >\n> > This introduces UniqueFD. It acts like unique_ptr to a file descriptor.\n> \n> Have you considered subclassing FileDescriptor and restrict its\n> interface to support move-only semantic ?\n\nNot really, as the two are very different beasts.\n\n> > Signed-off-by: Hirokazu Honda \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v2:\n> >\n> > - Rename ScopedFD to UniqueFD\n> > - Inline most functions to allow compiler optimizations\n> > - Bring the API closer to unique_ptr<>\n> > - Add swap()\n> > - Documentation cleanups\n> > - Slip FileDescriptor constructor to separate patch\n> > - Fix isValid()\n> > ---\n> > include/libcamera/base/meson.build | 1 +\n> > include/libcamera/base/unique_fd.h | 69 ++++++++++++++++\n> > src/libcamera/base/meson.build | 1 +\n> > src/libcamera/base/unique_fd.cpp | 123 +++++++++++++++++++++++++++++\n> > 4 files changed, 194 insertions(+)\n> > create mode 100644 include/libcamera/base/unique_fd.h\n> > create mode 100644 src/libcamera/base/unique_fd.cpp\n> >\n> > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> > index f73b00917409..cca374a769cc 100644\n> > --- a/include/libcamera/base/meson.build\n> > +++ b/include/libcamera/base/meson.build\n> > @@ -22,6 +22,7 @@ libcamera_base_headers = files([\n> > 'span.h',\n> > 'thread.h',\n> > 'timer.h',\n> > + 'unique_fd.h',\n> > 'utils.h',\n> > ])\n> >\n> > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> > new file mode 100644\n> > index 000000000000..ae4d96b75797\n> > --- /dev/null\n> > +++ b/include/libcamera/base/unique_fd.h\n> > @@ -0,0 +1,69 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * unique_fd.h - File descriptor wrapper that owns a file descriptor.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class UniqueFD final\n> > +{\n> > +public:\n> > +\tUniqueFD()\n> > +\t\t: fd_(-1)\n> > +\t{\n> > +\t}\n> > +\n> > +\texplicit UniqueFD(int fd)\n> > +\t\t: fd_(fd)\n> > +\t{\n> > +\t}\n> > +\n> > +\tUniqueFD(UniqueFD &&other)\n> > +\t\t: fd_(other.release())\n> > +\t{\n> > +\t}\n> > +\n> > +\t~UniqueFD()\n> > +\t{\n> > +\t\treset();\n> > +\t}\n> > +\n> > +\tUniqueFD &operator=(UniqueFD &&other)\n> > +\t{\n> > +\t\treset(other.release());\n> > +\t\treturn *this;\n> > +\t}\n> > +\n> > +\t__nodiscard int release()\n> > +\t{\n> > +\t\tint fd = fd_;\n> > +\t\tfd_ = -1;\n> > +\t\treturn fd;\n> > +\t}\n> \n> Thanks, this will be great for fences\n\nThat's why I sent this series ;-)\n\n> > +\n> > +\tvoid reset(int fd = -1);\n> > +\n> > +\tvoid swap(UniqueFD &other)\n> > +\t{\n> > +\t\tstd::swap(fd_, other.fd_);\n> > +\t}\n> > +\n> > +\tint get() const { return fd_; }\n> > +\tbool isValid() const { return fd_ >= 0; }\n> \n> nit: const functions are usually put first\n\nI've ordered the functions is in the std::unique_ptr<> documentation,\n(with isValid() replacing the bool operator), but I can change that if\ndesired.\n\n> > +\n> > +private:\n> > +\tLIBCAMERA_DISABLE_COPY(UniqueFD)\n> > +\n> > +\tint fd_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> > index d5254fda9cbf..b0d85bc19245 100644\n> > --- a/src/libcamera/base/meson.build\n> > +++ b/src/libcamera/base/meson.build\n> > @@ -17,6 +17,7 @@ libcamera_base_sources = files([\n> > 'signal.cpp',\n> > 'thread.cpp',\n> > 'timer.cpp',\n> > + 'unique_fd.cpp',\n> > 'utils.cpp',\n> > ])\n> >\n> > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> > new file mode 100644\n> > index 000000000000..83d6919cf623\n> > --- /dev/null\n> > +++ b/src/libcamera/base/unique_fd.cpp\n> > @@ -0,0 +1,123 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * unique_fd.cpp - File descriptor wrapper that owns a file descriptor\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file base/unique_fd.h\n> > + * \\brief File descriptor wrapper that owns a file descriptor\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(UniqueFD)\n> > +\n> > +/**\n> > + * \\class UniqueFD\n> > + * \\brief unique_ptr-like wrapper for a file descriptor\n> > + *\n> > + * The UniqueFD is a wrapper that owns and manages the lifetime of a file\n> > + * descriptor. It is constructed from a numerical file descriptor, and takes\n> > + * over its ownership. The file descriptor is closed when the UniqueFD is\n> \n> Is talking about ownship with raw int correct ?\n\nI think talking about file descriptor ownership makes sense, as that's\nwhat it's all about. It's not about owning the variable passed to the\nconstructor.\n\n> I would specify that the fd passed in is instead reset to -1\n\nIt isn't though. I have thought about it, and researched why\nstd::unique_ptr<> had a constructor taking a pointer, not an rvalue\nreference to the pointer. It seems the C++ authors have decided that it\nwouldn't really bring much additional safety to automatically set the\npointer to null. Maybe it should be different here ? I'm not sure.\n\n> > + * destroyed, or when it is assigned another file descriptor with operator=()\n> > + * or reset().\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::UniqueFD()\n> > + * \\brief Construct a UniqueFD that owns no file descriptor\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::UniqueFD(int fd)\n> > + * \\brief Construct a UniqueFD that owns \\a fd\n> > + * \\param[in] fd A file descriptor to manage\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::UniqueFD(UniqueFD &&other)\n> > + * \\brief Move constructor, create a UniqueFD by taking over \\a other\n> > + * \\param[in] other The other UniqueFD\n> > + *\n> > + * Create a UniqueFD by transferring ownership of the file descriptor owned by\n> > + * \\a other. Upon return, the \\a other UniqueFD is invalid.\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::~UniqueFD()\n> > + * \\brief Destroy the UniqueFD instance\n> > + *\n> > + * If a file descriptor is owned, it is closed.\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::operator=(UniqueFD &&other)\n> > + * \\brief Move assignment operator, replace a UniqueFD by taking over \\a other\n> > + * \\param[in] other The other UniqueFD\n> > + *\n> > + * If this UniqueFD owns a file descriptor, the file descriptor is closed\n> > + * first. The file descriptor is then replaced by the one of \\a other. Upon\n> > + * return, \\a other is invalid.\n> > + *\n> > + * \\return A reference to this UniqueFD\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::release()\n> > + * \\brief Release ownership of the file descriptor without closing it\n> > + *\n> > + * This function releases and returns the owned file descriptor without closing\n> > + * it. The caller owns the returned value and must take care of handling its\n> > + * life time to avoid file descriptor leakages. Upon return this UniqueFD is\n> > + * invalid.\n> > + *\n> > + * \\return The managed file descriptor, or -1 if no file descriptor was owned\n> > + */\n> > +\n> > +/**\n> > + * \\brief Replace the managed file descriptor\n> > + * \\param[in] fd The new file descriptor to manage\n> > + *\n> > + * Close the managed file descriptor, if any, and replace it with the new \\a fd.\n> > + *\n> > + * Self-resetting (passing an \\a fd already managed by this instance) is invalid\n> > + * and results in undefined behaviour.\n> > + */\n> > +void UniqueFD::reset(int fd)\n> \n> Has we enforce move semantics, shouldn't this take a && and reset fd\n> to -1 ?\n\nSame reasoning as for the constructor. We should use rvalue references\nfor both or none.\n\n> > +{\n> > +\tASSERT(!isValid() || fd != fd_);\n> > +\n> > +\tstd::swap(fd, fd_);\n> > +\n> > +\tif (fd >= 0)\n> > +\t\tclose(fd);\n> \n> wouldn't it be more clear to just:\n> \n> if (fd_ > 0)\n> fd_.close();\n> fd_ = fd;\n> \n> /* with the above suggestion: */\n> fd = -1;\n> ?\n\nI went for swap() to match the behaviour of std::unique_ptr<>, but there\nmay be no need to in this case. If you prefer the above, I can change\nit.\n\n> > +}\n> > +\n> > +/**\n> > + * \\fn UniqueFD::swap(UniqueFD &other)\n> > + * \\brief Swap the managed file descriptors with another UniqueFD\n> > + * \\param[in] other Another UniqueFD to swap the file descriptor with\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::get()\n> > + * \\brief Retrieve the managed file descriptor\n> > + * \\return The managed file descriptor, or -1 if no file descriptor is owned\n> > + */\n> > +\n> > +/**\n> > + * \\fn UniqueFD::isValid()\n> > + * \\brief Check if the UniqueFD owns a valid file descriptor\n> > + * \\return True if the UniqueFD owns a valid file descriptor, false otherwise\n> > + */\n> > +\n> > +} /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D5E30BF415\n\tfor ;\n\tMon, 29 Nov 2021 16:41:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F52F605A4;\n\tMon, 29 Nov 2021 17:41:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30E7360592\n\tfor ;\n\tMon, 29 Nov 2021 17:41:35 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90E222A5;\n\tMon, 29 Nov 2021 17:41:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oSwJwedK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638204094;\n\tbh=KIUaAnz+JjFWzoipWsQRDlZKjp79JKQx+9woc5Ep0Cg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oSwJwedKkt7o422o4qE6LJNM2RQfzwG5ls7VfWN9IhCr9zP135KP3fklyM5fjzJGz\n\tQig7z4aRSPmGyxY35DpIkTIbyw5+jCYGUSzc4DvZkOP+BUAPpSUfqDVM9A1B1dKpUs\n\tveHo2LNJ4AcfE564gZfOpG4R8p4kxU8XvG7IZSEA=","Date":"Mon, 29 Nov 2021 18:41:10 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>\n\t<20211129145826.phr7faw6okt5yiwg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129145826.phr7faw6okt5yiwg@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21363,"web_url":"https://patchwork.libcamera.org/comment/21363/","msgid":"<20211129172153.as5yctkbx4frbe32@uno.localdomain>","date":"2021-11-29T17:21:53","subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Nov 29, 2021 at 06:41:10PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Nov 29, 2021 at 03:58:26PM +0100, Jacopo Mondi wrote:\n> > On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:\n> > > From: Hirokazu Honda \n> > >\n> > > This introduces UniqueFD. It acts like unique_ptr to a file descriptor.\n> >\n> > Have you considered subclassing FileDescriptor and restrict its\n> > interface to support move-only semantic ?\n>\n> Not really, as the two are very different beasts.\n>\n> > > Signed-off-by: Hirokazu Honda \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Changes since v2:\n> > >\n> > > - Rename ScopedFD to UniqueFD\n> > > - Inline most functions to allow compiler optimizations\n> > > - Bring the API closer to unique_ptr<>\n> > > - Add swap()\n> > > - Documentation cleanups\n> > > - Slip FileDescriptor constructor to separate patch\n> > > - Fix isValid()\n> > > ---\n> > > include/libcamera/base/meson.build | 1 +\n> > > include/libcamera/base/unique_fd.h | 69 ++++++++++++++++\n> > > src/libcamera/base/meson.build | 1 +\n> > > src/libcamera/base/unique_fd.cpp | 123 +++++++++++++++++++++++++++++\n> > > 4 files changed, 194 insertions(+)\n> > > create mode 100644 include/libcamera/base/unique_fd.h\n> > > create mode 100644 src/libcamera/base/unique_fd.cpp\n> > >\n> > > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> > > index f73b00917409..cca374a769cc 100644\n> > > --- a/include/libcamera/base/meson.build\n> > > +++ b/include/libcamera/base/meson.build\n> > > @@ -22,6 +22,7 @@ libcamera_base_headers = files([\n> > > 'span.h',\n> > > 'thread.h',\n> > > 'timer.h',\n> > > + 'unique_fd.h',\n> > > 'utils.h',\n> > > ])\n> > >\n> > > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> > > new file mode 100644\n> > > index 000000000000..ae4d96b75797\n> > > --- /dev/null\n> > > +++ b/include/libcamera/base/unique_fd.h\n> > > @@ -0,0 +1,69 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * unique_fd.h - File descriptor wrapper that owns a file descriptor.\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class UniqueFD final\n> > > +{\n> > > +public:\n> > > +\tUniqueFD()\n> > > +\t\t: fd_(-1)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\texplicit UniqueFD(int fd)\n> > > +\t\t: fd_(fd)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tUniqueFD(UniqueFD &&other)\n> > > +\t\t: fd_(other.release())\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\t~UniqueFD()\n> > > +\t{\n> > > +\t\treset();\n> > > +\t}\n> > > +\n> > > +\tUniqueFD &operator=(UniqueFD &&other)\n> > > +\t{\n> > > +\t\treset(other.release());\n> > > +\t\treturn *this;\n> > > +\t}\n> > > +\n> > > +\t__nodiscard int release()\n> > > +\t{\n> > > +\t\tint fd = fd_;\n> > > +\t\tfd_ = -1;\n> > > +\t\treturn fd;\n> > > +\t}\n> >\n> > Thanks, this will be great for fences\n>\n> That's why I sent this series ;-)\n>\n> > > +\n> > > +\tvoid reset(int fd = -1);\n> > > +\n> > > +\tvoid swap(UniqueFD &other)\n> > > +\t{\n> > > +\t\tstd::swap(fd_, other.fd_);\n> > > +\t}\n> > > +\n> > > +\tint get() const { return fd_; }\n> > > +\tbool isValid() const { return fd_ >= 0; }\n> >\n> > nit: const functions are usually put first\n>\n> I've ordered the functions is in the std::unique_ptr<> documentation,\n> (with isValid() replacing the bool operator), but I can change that if\n> desired.\n>\n\nNot a big deal, but usually in the code base I've noticed that (not\nthat I recall we ever really established a rule about that)\n\n> > > +\n> > > +private:\n> > > +\tLIBCAMERA_DISABLE_COPY(UniqueFD)\n> > > +\n> > > +\tint fd_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> > > index d5254fda9cbf..b0d85bc19245 100644\n> > > --- a/src/libcamera/base/meson.build\n> > > +++ b/src/libcamera/base/meson.build\n> > > @@ -17,6 +17,7 @@ libcamera_base_sources = files([\n> > > 'signal.cpp',\n> > > 'thread.cpp',\n> > > 'timer.cpp',\n> > > + 'unique_fd.cpp',\n> > > 'utils.cpp',\n> > > ])\n> > >\n> > > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> > > new file mode 100644\n> > > index 000000000000..83d6919cf623\n> > > --- /dev/null\n> > > +++ b/src/libcamera/base/unique_fd.cpp\n> > > @@ -0,0 +1,123 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * unique_fd.cpp - File descriptor wrapper that owns a file descriptor\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file base/unique_fd.h\n> > > + * \\brief File descriptor wrapper that owns a file descriptor\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(UniqueFD)\n> > > +\n> > > +/**\n> > > + * \\class UniqueFD\n> > > + * \\brief unique_ptr-like wrapper for a file descriptor\n> > > + *\n> > > + * The UniqueFD is a wrapper that owns and manages the lifetime of a file\n> > > + * descriptor. It is constructed from a numerical file descriptor, and takes\n> > > + * over its ownership. The file descriptor is closed when the UniqueFD is\n> >\n> > Is talking about ownship with raw int correct ?\n>\n> I think talking about file descriptor ownership makes sense, as that's\n> what it's all about. It's not about owning the variable passed to the\n> constructor.\n>\n> > I would specify that the fd passed in is instead reset to -1\n>\n> It isn't though. I have thought about it, and researched why\n\nI noticed later and suggested that in the review of a later patch :)\n\n> std::unique_ptr<> had a constructor taking a pointer, not an rvalue\n> reference to the pointer. It seems the C++ authors have decided that it\n> wouldn't really bring much additional safety to automatically set the\n> pointer to null. Maybe it should be different here ? I'm not sure.\n\nNot sure, but if the fd argument stays valid, users might be tempted\nto close it.\n\n>\n> > > + * destroyed, or when it is assigned another file descriptor with operator=()\n> > > + * or reset().\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::UniqueFD()\n> > > + * \\brief Construct a UniqueFD that owns no file descriptor\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::UniqueFD(int fd)\n> > > + * \\brief Construct a UniqueFD that owns \\a fd\n> > > + * \\param[in] fd A file descriptor to manage\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::UniqueFD(UniqueFD &&other)\n> > > + * \\brief Move constructor, create a UniqueFD by taking over \\a other\n> > > + * \\param[in] other The other UniqueFD\n> > > + *\n> > > + * Create a UniqueFD by transferring ownership of the file descriptor owned by\n> > > + * \\a other. Upon return, the \\a other UniqueFD is invalid.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::~UniqueFD()\n> > > + * \\brief Destroy the UniqueFD instance\n> > > + *\n> > > + * If a file descriptor is owned, it is closed.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::operator=(UniqueFD &&other)\n> > > + * \\brief Move assignment operator, replace a UniqueFD by taking over \\a other\n> > > + * \\param[in] other The other UniqueFD\n> > > + *\n> > > + * If this UniqueFD owns a file descriptor, the file descriptor is closed\n> > > + * first. The file descriptor is then replaced by the one of \\a other. Upon\n> > > + * return, \\a other is invalid.\n> > > + *\n> > > + * \\return A reference to this UniqueFD\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::release()\n> > > + * \\brief Release ownership of the file descriptor without closing it\n> > > + *\n> > > + * This function releases and returns the owned file descriptor without closing\n> > > + * it. The caller owns the returned value and must take care of handling its\n> > > + * life time to avoid file descriptor leakages. Upon return this UniqueFD is\n> > > + * invalid.\n> > > + *\n> > > + * \\return The managed file descriptor, or -1 if no file descriptor was owned\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Replace the managed file descriptor\n> > > + * \\param[in] fd The new file descriptor to manage\n> > > + *\n> > > + * Close the managed file descriptor, if any, and replace it with the new \\a fd.\n> > > + *\n> > > + * Self-resetting (passing an \\a fd already managed by this instance) is invalid\n> > > + * and results in undefined behaviour.\n> > > + */\n> > > +void UniqueFD::reset(int fd)\n> >\n> > Has we enforce move semantics, shouldn't this take a && and reset fd\n> > to -1 ?\n>\n> Same reasoning as for the constructor. We should use rvalue references\n> for both or none.\n>\n\nI agree they should be the same, that's for sure.\n\nOn the semantic, I always thought that we should have had this class\ntake a && and reset to -1 the argument, but now that I think about\nthat it's mostly because I was thinking about subclassing\nFileDescriptor and only restrict it to use the move contructor.\n\nAs this class doesn't have other constructors, passing the parameter\nby value is ok. I like less the fact the parameter stays valid after\nbeing 'moved' to UniqueFD (as the class 'owns' the fd, I would expect\nI have to move it..) for the above said reasons.\n\nOn one side we could have an\n\n int filedesc = ....;\n UniqueFD fd(filedesc);\n close(filedesc);\n\nOn the other side\n\n int filedesc = ....;\n UniqueFD fd(std::move(filedesc));\n\nis indeed more clunky\n\nFor sake of correctness I would go for the second, but I recognize my\nmotivations are a bit fragile, so I won't push!\n\nThanks\n j\n\n\n> > > +{\n> > > +\tASSERT(!isValid() || fd != fd_);\n> > > +\n> > > +\tstd::swap(fd, fd_);\n> > > +\n> > > +\tif (fd >= 0)\n> > > +\t\tclose(fd);\n> >\n> > wouldn't it be more clear to just:\n> >\n> > if (fd_ > 0)\n> > fd_.close();\n> > fd_ = fd;\n> >\n> > /* with the above suggestion: */\n> > fd = -1;\n> > ?\n>\n> I went for swap() to match the behaviour of std::unique_ptr<>, but there\n> may be no need to in this case. If you prefer the above, I can change\n> it.\n>\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::swap(UniqueFD &other)\n> > > + * \\brief Swap the managed file descriptors with another UniqueFD\n> > > + * \\param[in] other Another UniqueFD to swap the file descriptor with\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::get()\n> > > + * \\brief Retrieve the managed file descriptor\n> > > + * \\return The managed file descriptor, or -1 if no file descriptor is owned\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn UniqueFD::isValid()\n> > > + * \\brief Check if the UniqueFD owns a valid file descriptor\n> > > + * \\return True if the UniqueFD owns a valid file descriptor, false otherwise\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2D1B5BDB13\n\tfor ;\n\tMon, 29 Nov 2021 17:21:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 621F5605A6;\n\tMon, 29 Nov 2021 18:21:02 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D938D60592\n\tfor ;\n\tMon, 29 Nov 2021 18:21:00 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 59406240005;\n\tMon, 29 Nov 2021 17:21:00 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 18:21:53 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129172153.as5yctkbx4frbe32@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>\n\t<20211129145826.phr7faw6okt5yiwg@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21387,"web_url":"https://patchwork.libcamera.org/comment/21387/","msgid":"","date":"2021-11-30T02:09:41","subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 29, 2021 at 06:21:53PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 29, 2021 at 06:41:10PM +0200, Laurent Pinchart wrote:\n> > On Mon, Nov 29, 2021 at 03:58:26PM +0100, Jacopo Mondi wrote:\n> > > On Mon, Nov 29, 2021 at 01:57:39AM +0200, Laurent Pinchart wrote:\n> > > > From: Hirokazu Honda \n> > > >\n> > > > This introduces UniqueFD. It acts like unique_ptr to a file descriptor.\n> > >\n> > > Have you considered subclassing FileDescriptor and restrict its\n> > > interface to support move-only semantic ?\n> >\n> > Not really, as the two are very different beasts.\n> >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > Changes since v2:\n> > > >\n> > > > - Rename ScopedFD to UniqueFD\n> > > > - Inline most functions to allow compiler optimizations\n> > > > - Bring the API closer to unique_ptr<>\n> > > > - Add swap()\n> > > > - Documentation cleanups\n> > > > - Slip FileDescriptor constructor to separate patch\n> > > > - Fix isValid()\n> > > > ---\n> > > > include/libcamera/base/meson.build | 1 +\n> > > > include/libcamera/base/unique_fd.h | 69 ++++++++++++++++\n> > > > src/libcamera/base/meson.build | 1 +\n> > > > src/libcamera/base/unique_fd.cpp | 123 +++++++++++++++++++++++++++++\n> > > > 4 files changed, 194 insertions(+)\n> > > > create mode 100644 include/libcamera/base/unique_fd.h\n> > > > create mode 100644 src/libcamera/base/unique_fd.cpp\n> > > >\n> > > > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> > > > index f73b00917409..cca374a769cc 100644\n> > > > --- a/include/libcamera/base/meson.build\n> > > > +++ b/include/libcamera/base/meson.build\n> > > > @@ -22,6 +22,7 @@ libcamera_base_headers = files([\n> > > > 'span.h',\n> > > > 'thread.h',\n> > > > 'timer.h',\n> > > > + 'unique_fd.h',\n> > > > 'utils.h',\n> > > > ])\n> > > >\n> > > > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> > > > new file mode 100644\n> > > > index 000000000000..ae4d96b75797\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/base/unique_fd.h\n> > > > @@ -0,0 +1,69 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * unique_fd.h - File descriptor wrapper that owns a file descriptor.\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class UniqueFD final\n> > > > +{\n> > > > +public:\n> > > > +\tUniqueFD()\n> > > > +\t\t: fd_(-1)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > +\texplicit UniqueFD(int fd)\n> > > > +\t\t: fd_(fd)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > +\tUniqueFD(UniqueFD &&other)\n> > > > +\t\t: fd_(other.release())\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > +\t~UniqueFD()\n> > > > +\t{\n> > > > +\t\treset();\n> > > > +\t}\n> > > > +\n> > > > +\tUniqueFD &operator=(UniqueFD &&other)\n> > > > +\t{\n> > > > +\t\treset(other.release());\n> > > > +\t\treturn *this;\n> > > > +\t}\n> > > > +\n> > > > +\t__nodiscard int release()\n> > > > +\t{\n> > > > +\t\tint fd = fd_;\n> > > > +\t\tfd_ = -1;\n> > > > +\t\treturn fd;\n> > > > +\t}\n> > >\n> > > Thanks, this will be great for fences\n> >\n> > That's why I sent this series ;-)\n> >\n> > > > +\n> > > > +\tvoid reset(int fd = -1);\n> > > > +\n> > > > +\tvoid swap(UniqueFD &other)\n> > > > +\t{\n> > > > +\t\tstd::swap(fd_, other.fd_);\n> > > > +\t}\n> > > > +\n> > > > +\tint get() const { return fd_; }\n> > > > +\tbool isValid() const { return fd_ >= 0; }\n> > >\n> > > nit: const functions are usually put first\n> >\n> > I've ordered the functions is in the std::unique_ptr<> documentation,\n> > (with isValid() replacing the bool operator), but I can change that if\n> > desired.\n> \n> Not a big deal, but usually in the code base I've noticed that (not\n> that I recall we ever really established a rule about that)\n> \n> > > > +\n> > > > +private:\n> > > > +\tLIBCAMERA_DISABLE_COPY(UniqueFD)\n> > > > +\n> > > > +\tint fd_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build\n> > > > index d5254fda9cbf..b0d85bc19245 100644\n> > > > --- a/src/libcamera/base/meson.build\n> > > > +++ b/src/libcamera/base/meson.build\n> > > > @@ -17,6 +17,7 @@ libcamera_base_sources = files([\n> > > > 'signal.cpp',\n> > > > 'thread.cpp',\n> > > > 'timer.cpp',\n> > > > + 'unique_fd.cpp',\n> > > > 'utils.cpp',\n> > > > ])\n> > > >\n> > > > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..83d6919cf623\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/base/unique_fd.cpp\n> > > > @@ -0,0 +1,123 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * unique_fd.cpp - File descriptor wrapper that owns a file descriptor\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +/**\n> > > > + * \\file base/unique_fd.h\n> > > > + * \\brief File descriptor wrapper that owns a file descriptor\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(UniqueFD)\n> > > > +\n> > > > +/**\n> > > > + * \\class UniqueFD\n> > > > + * \\brief unique_ptr-like wrapper for a file descriptor\n> > > > + *\n> > > > + * The UniqueFD is a wrapper that owns and manages the lifetime of a file\n> > > > + * descriptor. It is constructed from a numerical file descriptor, and takes\n> > > > + * over its ownership. The file descriptor is closed when the UniqueFD is\n> > >\n> > > Is talking about ownship with raw int correct ?\n> >\n> > I think talking about file descriptor ownership makes sense, as that's\n> > what it's all about. It's not about owning the variable passed to the\n> > constructor.\n> >\n> > > I would specify that the fd passed in is instead reset to -1\n> >\n> > It isn't though. I have thought about it, and researched why\n> \n> I noticed later and suggested that in the review of a later patch :)\n> \n> > std::unique_ptr<> had a constructor taking a pointer, not an rvalue\n> > reference to the pointer. It seems the C++ authors have decided that it\n> > wouldn't really bring much additional safety to automatically set the\n> > pointer to null. Maybe it should be different here ? I'm not sure.\n> \n> Not sure, but if the fd argument stays valid, users might be tempted\n> to close it.\n>\n> > > > + * destroyed, or when it is assigned another file descriptor with operator=()\n> > > > + * or reset().\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::UniqueFD()\n> > > > + * \\brief Construct a UniqueFD that owns no file descriptor\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::UniqueFD(int fd)\n> > > > + * \\brief Construct a UniqueFD that owns \\a fd\n> > > > + * \\param[in] fd A file descriptor to manage\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::UniqueFD(UniqueFD &&other)\n> > > > + * \\brief Move constructor, create a UniqueFD by taking over \\a other\n> > > > + * \\param[in] other The other UniqueFD\n> > > > + *\n> > > > + * Create a UniqueFD by transferring ownership of the file descriptor owned by\n> > > > + * \\a other. Upon return, the \\a other UniqueFD is invalid.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::~UniqueFD()\n> > > > + * \\brief Destroy the UniqueFD instance\n> > > > + *\n> > > > + * If a file descriptor is owned, it is closed.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::operator=(UniqueFD &&other)\n> > > > + * \\brief Move assignment operator, replace a UniqueFD by taking over \\a other\n> > > > + * \\param[in] other The other UniqueFD\n> > > > + *\n> > > > + * If this UniqueFD owns a file descriptor, the file descriptor is closed\n> > > > + * first. The file descriptor is then replaced by the one of \\a other. Upon\n> > > > + * return, \\a other is invalid.\n> > > > + *\n> > > > + * \\return A reference to this UniqueFD\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::release()\n> > > > + * \\brief Release ownership of the file descriptor without closing it\n> > > > + *\n> > > > + * This function releases and returns the owned file descriptor without closing\n> > > > + * it. The caller owns the returned value and must take care of handling its\n> > > > + * life time to avoid file descriptor leakages. Upon return this UniqueFD is\n> > > > + * invalid.\n> > > > + *\n> > > > + * \\return The managed file descriptor, or -1 if no file descriptor was owned\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Replace the managed file descriptor\n> > > > + * \\param[in] fd The new file descriptor to manage\n> > > > + *\n> > > > + * Close the managed file descriptor, if any, and replace it with the new \\a fd.\n> > > > + *\n> > > > + * Self-resetting (passing an \\a fd already managed by this instance) is invalid\n> > > > + * and results in undefined behaviour.\n> > > > + */\n> > > > +void UniqueFD::reset(int fd)\n> > >\n> > > Has we enforce move semantics, shouldn't this take a && and reset fd\n> > > to -1 ?\n> >\n> > Same reasoning as for the constructor. We should use rvalue references\n> > for both or none.\n> \n> I agree they should be the same, that's for sure.\n> \n> On the semantic, I always thought that we should have had this class\n> take a && and reset to -1 the argument, but now that I think about\n> that it's mostly because I was thinking about subclassing\n> FileDescriptor and only restrict it to use the move contructor.\n> \n> As this class doesn't have other constructors, passing the parameter\n> by value is ok. I like less the fact the parameter stays valid after\n> being 'moved' to UniqueFD (as the class 'owns' the fd, I would expect\n> I have to move it..) for the above said reasons.\n> \n> On one side we could have an\n> \n> int filedesc = ....;\n> UniqueFD fd(filedesc);\n> close(filedesc);\n> \n> On the other side\n> \n> int filedesc = ....;\n> UniqueFD fd(std::move(filedesc));\n> \n> is indeed more clunky\n> \n> For sake of correctness I would go for the second, but I recognize my\n> motivations are a bit fragile, so I won't push!\n\nI'll include this change in v4 as part of a separate patch, we can\ndiscuss it there.\n\n> > > > +{\n> > > > +\tASSERT(!isValid() || fd != fd_);\n> > > > +\n> > > > +\tstd::swap(fd, fd_);\n> > > > +\n> > > > +\tif (fd >= 0)\n> > > > +\t\tclose(fd);\n> > >\n> > > wouldn't it be more clear to just:\n> > >\n> > > if (fd_ > 0)\n> > > fd_.close();\n> > > fd_ = fd;\n> > >\n> > > /* with the above suggestion: */\n> > > fd = -1;\n> > > ?\n> >\n> > I went for swap() to match the behaviour of std::unique_ptr<>, but there\n> > may be no need to in this case. If you prefer the above, I can change\n> > it.\n> >\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::swap(UniqueFD &other)\n> > > > + * \\brief Swap the managed file descriptors with another UniqueFD\n> > > > + * \\param[in] other Another UniqueFD to swap the file descriptor with\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::get()\n> > > > + * \\brief Retrieve the managed file descriptor\n> > > > + * \\return The managed file descriptor, or -1 if no file descriptor is owned\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn UniqueFD::isValid()\n> > > > + * \\brief Check if the UniqueFD owns a valid file descriptor\n> > > > + * \\return True if the UniqueFD owns a valid file descriptor, false otherwise\n> > > > + */\n> > > > +\n> > > > +} /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 56262BDB13\n\tfor ;\n\tTue, 30 Nov 2021 02:10:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 901B3605B7;\n\tTue, 30 Nov 2021 03:10:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CFA7604FC\n\tfor ;\n\tTue, 30 Nov 2021 03:10:06 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D79CE2A5;\n\tTue, 30 Nov 2021 03:10:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Qp8iM+Wd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638238206;\n\tbh=7FirQlj5pE4V6rZoquDyahNB8mvnVNm40+gBMLG99lw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qp8iM+WdzsU17/vNTbPeRdoOichbEMV6o8JOlPV2r1x6tsFWDA+bD6g5u9ynbXnh2\n\tuaPw2yKEUpw/eXwQnlUqVkA+Y4CNiVHn6272xSiFk2qMPPkhDQ8VPJ/1NxcttTE8X8\n\txcKkimxfVZtza0lEYjAZ+AtEBWexfIxDviYQvPsw=","Date":"Tue, 30 Nov 2021 04:09:41 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-5-laurent.pinchart@ideasonboard.com>\n\t<20211129145826.phr7faw6okt5yiwg@uno.localdomain>\n\t\n\t<20211129172153.as5yctkbx4frbe32@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129172153.as5yctkbx4frbe32@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 04/17] libcamera: base: Introduce\n\tUniqueFD","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]