[{"id":21489,"web_url":"https://patchwork.libcamera.org/comment/21489/","msgid":"","date":"2021-12-01T05:43:33","subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Nov 30, 2021 at 12:39 PM Laurent Pinchart\n wrote:\n>\n> To avoid the risk of using a file descriptor whose ownership has been\n> transferred to a UniqueFD instance, pass an rvalue reference to the\n> constructor taking an int, and to the reset() function. The fd argument\n> is set to -1 upon return, making incorrect usage of the file descriptor\n> impossible.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> I'm not entirely sure about this patch. If desired, it will get squashed\n> with the one introducing UniqueFD.\n> ---\n\nI am not sure either.. I would like to hear others' opinions although\nI am not against this.\n\n> include/libcamera/base/unique_fd.h | 5 +++--\n> src/libcamera/base/shared_fd.cpp | 2 +-\n> src/libcamera/base/unique_fd.cpp | 10 ++++++++--\n> src/libcamera/ipc_unixsocket.cpp | 4 ++--\n> src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++---------\n> src/libcamera/process.cpp | 4 ++--\n> src/libcamera/v4l2_videodevice.cpp | 2 +-\n> test/unique-fd.cpp | 10 ++++++----\n> 8 files changed, 29 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> index ae4d96b75797..a5ffb1a435dc 100644\n> --- a/include/libcamera/base/unique_fd.h\n> +++ b/include/libcamera/base/unique_fd.h\n> @@ -22,9 +22,10 @@ public:\n> {\n> }\n>\n> - explicit UniqueFD(int fd)\n> + explicit UniqueFD(int &&fd)\n> : fd_(fd)\n> {\n> + fd = -1;\n> }\n>\n> UniqueFD(UniqueFD &&other)\n> @@ -50,7 +51,7 @@ public:\n> return fd;\n> }\n>\n> - void reset(int fd = -1);\n> + void reset(int &&fd = -1);\n>\n> void swap(UniqueFD &other)\n> {\n> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp\n> index 0c8b93a47f85..8aecd34038bd 100644\n> --- a/src/libcamera/base/shared_fd.cpp\n> +++ b/src/libcamera/base/shared_fd.cpp\n> @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const\n> << \"Failed to dup() fd: \" << strerror(-ret);\n> }\n>\n> - return UniqueFD(dupFd);\n> + return UniqueFD(std::move(dupFd));\n> }\n>\n> SharedFD::Descriptor::Descriptor(int fd, bool duplicate)\n> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> index 83d6919cf623..7a961dbc01d0 100644\n> --- a/src/libcamera/base/unique_fd.cpp\n> +++ b/src/libcamera/base/unique_fd.cpp\n> @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\n> */\n>\n> /**\n> - * \\fn UniqueFD::UniqueFD(int fd)\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> + * Ownership of the file descriptor is transferred to the UniqueFD instance.\n> + * Upon return \\a fd is set to -1.\n> */\n>\n> /**\n> @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\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> + * Upon return \\a fd is set to -1.\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> +void UniqueFD::reset(int &&fd)\n> {\n> ASSERT(!isValid() || fd != fd_);\n>\n> @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd)\n>\n> if (fd >= 0)\n> close(fd);\n> +\n> + fd = -1;\n> }\n>\n> /**\n> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> index 1980d374cea8..32bd6533d192 100644\n> --- a/src/libcamera/ipc_unixsocket.cpp\n> +++ b/src/libcamera/ipc_unixsocket.cpp\n> @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create()\n> }\n>\n> std::array socketFds{\n> - UniqueFD(sockets[0]),\n> - UniqueFD(sockets[1]),\n> + UniqueFD(std::move(sockets[0])),\n> + UniqueFD(std::move(sockets[1])),\n> };\n>\n> if (bind(std::move(socketFds[0])) < 0)\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> index 69831dabbe75..f2a95300d187 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> @@ -37,16 +37,13 @@ namespace RPi {\n> DmaHeap::DmaHeap()\n> {\n> for (const char *name : heapNames) {\n> - int ret = ::open(name, O_RDWR, 0);\n> - if (ret < 0) {\n> - ret = errno;\n> - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> - << strerror(ret);\n> - continue;\n> - }\n> + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0));\n> + if (dmaHeapHandle_.isValid())\n> + break;\n>\n> - dmaHeapHandle_ = UniqueFD(ret);\n> - break;\n> + int err = errno;\n> + LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> + << strerror(err);\n> }\n>\n> if (!dmaHeapHandle_.isValid())\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 0e6b4e1dde58..dc8576d934b6 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -134,8 +134,8 @@ ProcessManager::ProcessManager()\n> LOG(Process, Fatal)\n> << \"Failed to initialize pipe for signal handling\";\n>\n> - pipe_[0] = UniqueFD(pipe[0]);\n> - pipe_[1] = UniqueFD(pipe[1]);\n> + pipe_[0] = UniqueFD(std::move(pipe[0]));\n> + pipe_[1] = UniqueFD(std::move(pipe[1]));\n>\n> sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index b4b89e2759b9..9d0d0077f4e4 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index,\n> return {};\n> }\n>\n> - return UniqueFD(expbuf.fd);\n> + return UniqueFD(std::move(expbuf.fd));\n> }\n>\n> /**\n> diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> index eb3b591fec28..7e94e3ca450e 100644\n> --- a/test/unique-fd.cpp\n> +++ b/test/unique-fd.cpp\n> @@ -39,7 +39,8 @@ protected:\n> }\n>\n> /* Test creating UniqueFD from numerical file descriptor. */\n> - UniqueFD fd2(fd_);\n> + int numFd = fd_;\n> + UniqueFD fd2(std::move(numFd));\n> if (!fd2.isValid() || fd2.get() != fd_) {\n> std::cout << \"Failed fd check (fd constructor)\"\n> << std::endl;\n> @@ -113,7 +114,7 @@ protected:\n> }\n>\n> /* Test release. */\n> - int numFd = fd2.release();\n> + numFd = fd2.release();\n> if (fd2.isValid() || fd2.get() != -1) {\n> std::cout << \"Failed fd check (release)\"\n> << std::endl;\n> @@ -133,7 +134,7 @@ protected:\n> }\n>\n> /* Test reset assignment. */\n> - fd.reset(numFd);\n> + fd.reset(std::move(numFd));\n> if (!fd.isValid() || fd.get() != fd_) {\n> std::cout << \"Failed fd check (reset assignment)\"\n> << std::endl;\n> @@ -168,7 +169,8 @@ protected:\n> }\n>\n> {\n> - UniqueFD fd4(fd_);\n> + numFd = fd_;\n> + UniqueFD fd4(std::move(numFd));\n> }\n>\n> if (isValidFd(fd_)) {\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 9C9B6BDB13\n\tfor ;\n\tWed, 1 Dec 2021 05:43:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E570760723;\n\tWed, 1 Dec 2021 06:43:44 +0100 (CET)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8B0460592\n\tfor ;\n\tWed, 1 Dec 2021 06:43:43 +0100 (CET)","by mail-ed1-x535.google.com with SMTP id r11so96846566edd.9\n\tfor ;\n\tTue, 30 Nov 2021 21:43:43 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"iWBf2OHp\"; 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=O+v8Ygwva7QeK2ITX0ufY88beegELyicuLcB/2X6UWw=;\n\tb=iWBf2OHpRT09GclwrD8lJviRdHM20DwPXTua6ND4fHfuPXagOy/Rsg/I+3ax0xMiXz\n\tkvltd0JQRZLK8m/oarzIt4bQj9TMh+yDBuFKiL8Hn1vliaFK4e93WrflNeJd9bM1LFSM\n\tzyroMY950wU0e38Ttt/pKFKlJhMn+HC5ohE8A=","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=O+v8Ygwva7QeK2ITX0ufY88beegELyicuLcB/2X6UWw=;\n\tb=0LBQvZPEfGb2kxwvosmP/0oCN3asWSMCdYQp4WsN1oJOVT5CvmgkmC56+XpzinLt3t\n\tWzBHqtw4O7CZ2W7yOqR1TUPOnZUILG97MqjEJOMrbFtvBnSbUI/hU+XV8eASfwoE/Gb1\n\tboBYTCoPaMQ5k/3536s1DHUvLPC+kQVDSWj1kUYHHr4JLkpamAovYzt//6gB+JthaTND\n\tv2Il0FzHOa0NhMH2Y4MZTb4vS5eaq6MVbm2ksNo3xMr9ElFRKHUZ4jhFaXriEG6ybcvF\n\theEjChn40SxkAma8i0cfj45CpxPHC8bflthdy1JbNXJeVrRP054HnS+25UQBOfcsLo+Z\n\t62eg==","X-Gm-Message-State":"AOAM531htP1gJsQebEiHaS84g2ImqxLlMV1OwuCc4F7cn2+k09ZorP+3\n\t9MvM0d8KwIl52mJ88MFmXYJ250VWJT/xIbbITqYu2zGlykI=","X-Google-Smtp-Source":"ABdhPJyuxsR2qi7l2sGTyvF7XaXR/EEOqT5vjywKCHOXJqiDw53LeRT9mSA/9G4s7bRHXzmyyTdG1MM/j1j8eei2oZA=","X-Received":"by 2002:a17:906:7688:: with SMTP id\n\to8mr4477866ejm.291.1638337423454; \n\tTue, 30 Nov 2021 21:43:43 -0800 (PST)","MIME-Version":"1.0","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 1 Dec 2021 14:43:33 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","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":21580,"web_url":"https://patchwork.libcamera.org/comment/21580/","msgid":"<163854864760.3059017.7014221997850119859@Monstersaurus>","date":"2021-12-03T16:24:07","subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-11-30 03:38:20)\n> To avoid the risk of using a file descriptor whose ownership has been\n> transferred to a UniqueFD instance, pass an rvalue reference to the\n> constructor taking an int, and to the reset() function. The fd argument\n> is set to -1 upon return, making incorrect usage of the file descriptor\n> impossible.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> I'm not entirely sure about this patch. If desired, it will get squashed\n> with the one introducing UniqueFD.\n\nI kind of like that it makes the ownership explicit. I wasn't sure about\nthe requirement to add std::move() ... but that makes it clear I guess.\n\nIf you don't provide the std::move() will the compiler warn/error?\n\nAlso down in the testing, assigning to a new int, just so that one can\nbe moved in now looks like a hack - so should either be removed or\nclarified in a comment.\n\n\n> ---\n> include/libcamera/base/unique_fd.h | 5 +++--\n> src/libcamera/base/shared_fd.cpp | 2 +-\n> src/libcamera/base/unique_fd.cpp | 10 ++++++++--\n> src/libcamera/ipc_unixsocket.cpp | 4 ++--\n> src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++---------\n> src/libcamera/process.cpp | 4 ++--\n> src/libcamera/v4l2_videodevice.cpp | 2 +-\n> test/unique-fd.cpp | 10 ++++++----\n> 8 files changed, 29 insertions(+), 23 deletions(-)\n> \n> diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> index ae4d96b75797..a5ffb1a435dc 100644\n> --- a/include/libcamera/base/unique_fd.h\n> +++ b/include/libcamera/base/unique_fd.h\n> @@ -22,9 +22,10 @@ public:\n> {\n> }\n> \n> - explicit UniqueFD(int fd)\n> + explicit UniqueFD(int &&fd)\n> : fd_(fd)\n> {\n> + fd = -1;\n> }\n> \n> UniqueFD(UniqueFD &&other)\n> @@ -50,7 +51,7 @@ public:\n> return fd;\n> }\n> \n> - void reset(int fd = -1);\n> + void reset(int &&fd = -1);\n> \n> void swap(UniqueFD &other)\n> {\n> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp\n> index 0c8b93a47f85..8aecd34038bd 100644\n> --- a/src/libcamera/base/shared_fd.cpp\n> +++ b/src/libcamera/base/shared_fd.cpp\n> @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const\n> << \"Failed to dup() fd: \" << strerror(-ret);\n> }\n> \n> - return UniqueFD(dupFd);\n> + return UniqueFD(std::move(dupFd));\n> }\n> \n> SharedFD::Descriptor::Descriptor(int fd, bool duplicate)\n> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> index 83d6919cf623..7a961dbc01d0 100644\n> --- a/src/libcamera/base/unique_fd.cpp\n> +++ b/src/libcamera/base/unique_fd.cpp\n> @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\n> */\n> \n> /**\n> - * \\fn UniqueFD::UniqueFD(int fd)\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> + * Ownership of the file descriptor is transferred to the UniqueFD instance.\n> + * Upon return \\a fd is set to -1.\n> */\n> \n> /**\n> @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\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> + * Upon return \\a fd is set to -1.\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> +void UniqueFD::reset(int &&fd)\n> {\n> ASSERT(!isValid() || fd != fd_);\n> \n> @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd)\n> \n> if (fd >= 0)\n> close(fd);\n> +\n> + fd = -1;\n> }\n> \n> /**\n> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> index 1980d374cea8..32bd6533d192 100644\n> --- a/src/libcamera/ipc_unixsocket.cpp\n> +++ b/src/libcamera/ipc_unixsocket.cpp\n> @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create()\n> }\n> \n> std::array socketFds{\n> - UniqueFD(sockets[0]),\n> - UniqueFD(sockets[1]),\n> + UniqueFD(std::move(sockets[0])),\n> + UniqueFD(std::move(sockets[1])),\n> };\n> \n> if (bind(std::move(socketFds[0])) < 0)\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> index 69831dabbe75..f2a95300d187 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> @@ -37,16 +37,13 @@ namespace RPi {\n> DmaHeap::DmaHeap()\n> {\n> for (const char *name : heapNames) {\n> - int ret = ::open(name, O_RDWR, 0);\n> - if (ret < 0) {\n> - ret = errno;\n> - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> - << strerror(ret);\n> - continue;\n> - }\n> + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0));\n> + if (dmaHeapHandle_.isValid())\n> + break;\n> \n> - dmaHeapHandle_ = UniqueFD(ret);\n> - break;\n> + int err = errno;\n> + LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> + << strerror(err);\n> }\n> \n> if (!dmaHeapHandle_.isValid())\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> index 0e6b4e1dde58..dc8576d934b6 100644\n> --- a/src/libcamera/process.cpp\n> +++ b/src/libcamera/process.cpp\n> @@ -134,8 +134,8 @@ ProcessManager::ProcessManager()\n> LOG(Process, Fatal)\n> << \"Failed to initialize pipe for signal handling\";\n> \n> - pipe_[0] = UniqueFD(pipe[0]);\n> - pipe_[1] = UniqueFD(pipe[1]);\n> + pipe_[0] = UniqueFD(std::move(pipe[0]));\n> + pipe_[1] = UniqueFD(std::move(pipe[1]));\n> \n> sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index b4b89e2759b9..9d0d0077f4e4 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index,\n> return {};\n> }\n> \n> - return UniqueFD(expbuf.fd);\n> + return UniqueFD(std::move(expbuf.fd));\n> }\n> \n> /**\n> diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> index eb3b591fec28..7e94e3ca450e 100644\n> --- a/test/unique-fd.cpp\n> +++ b/test/unique-fd.cpp\n> @@ -39,7 +39,8 @@ protected:\n> }\n> \n> /* Test creating UniqueFD from numerical file descriptor. */\n> - UniqueFD fd2(fd_);\n> + int numFd = fd_;\n> + UniqueFD fd2(std::move(numFd));\n> if (!fd2.isValid() || fd2.get() != fd_) {\n> std::cout << \"Failed fd check (fd constructor)\"\n> << std::endl;\n> @@ -113,7 +114,7 @@ protected:\n> }\n> \n> /* Test release. */\n> - int numFd = fd2.release();\n> + numFd = fd2.release();\n> if (fd2.isValid() || fd2.get() != -1) {\n> std::cout << \"Failed fd check (release)\"\n> << std::endl;\n> @@ -133,7 +134,7 @@ protected:\n> }\n> \n> /* Test reset assignment. */\n> - fd.reset(numFd);\n> + fd.reset(std::move(numFd));\n> if (!fd.isValid() || fd.get() != fd_) {\n> std::cout << \"Failed fd check (reset assignment)\"\n> << std::endl;\n> @@ -168,7 +169,8 @@ protected:\n> }\n> \n> {\n> - UniqueFD fd4(fd_);\n> + numFd = fd_;\n> + UniqueFD fd4(std::move(numFd));\n\nThis now looks like a hack ... Should it be removed? What is it really\ntesting?\n\n\n> }\n> \n> if (isValidFd(fd_)) {\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 8E37EBF415\n\tfor ;\n\tFri, 3 Dec 2021 16:24:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 912BC60832;\n\tFri, 3 Dec 2021 17:24:12 +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 8ECB760725\n\tfor ;\n\tFri, 3 Dec 2021 17:24:10 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 290AEA59;\n\tFri, 3 Dec 2021 17:24:10 +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=\"OXDZUAAU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638548650;\n\tbh=YWH7z9uVXzMQXoArai3igQUTkCNZovu4Jw0H8KWeI+c=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=OXDZUAAUe7u7P48uhDYwDDKW+2tjL0c92Ebx6OdRrNW/cWgwobActjwBBtKozqcTN\n\tO0zzwXm9+CEjSNhf4WxJpr7RF7itW3Zex2QkjeIY2zPY21YARP11tA2NfpyHD+4C9+\n\tyTwJSTwVNM3SF9acf4S5J4Wndkua7DRKqD6tHpj4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 03 Dec 2021 16:24:07 +0000","Message-ID":"<163854864760.3059017.7014221997850119859@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21581,"web_url":"https://patchwork.libcamera.org/comment/21581/","msgid":"","date":"2021-12-03T16:32:52","subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Dec 03, 2021 at 04:24:07PM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-11-30 03:38:20)\n> > To avoid the risk of using a file descriptor whose ownership has been\n> > transferred to a UniqueFD instance, pass an rvalue reference to the\n> > constructor taking an int, and to the reset() function. The fd argument\n> > is set to -1 upon return, making incorrect usage of the file descriptor\n> > impossible.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > I'm not entirely sure about this patch. If desired, it will get squashed\n> > with the one introducing UniqueFD.\n> \n> I kind of like that it makes the ownership explicit. I wasn't sure about\n> the requirement to add std::move() ... but that makes it clear I guess.\n> \n> If you don't provide the std::move() will the compiler warn/error?\n\nUniqueFd fd(std::move(num));\nUniqueFD fd(3);\nfd.reset(std::move(num));\nfd.reset(3);\n\nshould all compile, while\n\nUniqueFd fd(num);\nfd.reset(num);\n\nshould result in a compilation error.\n\n> Also down in the testing, assigning to a new int, just so that one can\n> be moved in now looks like a hack - so should either be removed or\n> clarified in a comment.\n> \n> > ---\n> > include/libcamera/base/unique_fd.h | 5 +++--\n> > src/libcamera/base/shared_fd.cpp | 2 +-\n> > src/libcamera/base/unique_fd.cpp | 10 ++++++++--\n> > src/libcamera/ipc_unixsocket.cpp | 4 ++--\n> > src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++---------\n> > src/libcamera/process.cpp | 4 ++--\n> > src/libcamera/v4l2_videodevice.cpp | 2 +-\n> > test/unique-fd.cpp | 10 ++++++----\n> > 8 files changed, 29 insertions(+), 23 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> > index ae4d96b75797..a5ffb1a435dc 100644\n> > --- a/include/libcamera/base/unique_fd.h\n> > +++ b/include/libcamera/base/unique_fd.h\n> > @@ -22,9 +22,10 @@ public:\n> > {\n> > }\n> > \n> > - explicit UniqueFD(int fd)\n> > + explicit UniqueFD(int &&fd)\n> > : fd_(fd)\n> > {\n> > + fd = -1;\n> > }\n> > \n> > UniqueFD(UniqueFD &&other)\n> > @@ -50,7 +51,7 @@ public:\n> > return fd;\n> > }\n> > \n> > - void reset(int fd = -1);\n> > + void reset(int &&fd = -1);\n> > \n> > void swap(UniqueFD &other)\n> > {\n> > diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp\n> > index 0c8b93a47f85..8aecd34038bd 100644\n> > --- a/src/libcamera/base/shared_fd.cpp\n> > +++ b/src/libcamera/base/shared_fd.cpp\n> > @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const\n> > << \"Failed to dup() fd: \" << strerror(-ret);\n> > }\n> > \n> > - return UniqueFD(dupFd);\n> > + return UniqueFD(std::move(dupFd));\n> > }\n> > \n> > SharedFD::Descriptor::Descriptor(int fd, bool duplicate)\n> > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> > index 83d6919cf623..7a961dbc01d0 100644\n> > --- a/src/libcamera/base/unique_fd.cpp\n> > +++ b/src/libcamera/base/unique_fd.cpp\n> > @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\n> > */\n> > \n> > /**\n> > - * \\fn UniqueFD::UniqueFD(int fd)\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> > + * Ownership of the file descriptor is transferred to the UniqueFD instance.\n> > + * Upon return \\a fd is set to -1.\n> > */\n> > \n> > /**\n> > @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\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> > + * Upon return \\a fd is set to -1.\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> > +void UniqueFD::reset(int &&fd)\n> > {\n> > ASSERT(!isValid() || fd != fd_);\n> > \n> > @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd)\n> > \n> > if (fd >= 0)\n> > close(fd);\n> > +\n> > + fd = -1;\n> > }\n> > \n> > /**\n> > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> > index 1980d374cea8..32bd6533d192 100644\n> > --- a/src/libcamera/ipc_unixsocket.cpp\n> > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create()\n> > }\n> > \n> > std::array socketFds{\n> > - UniqueFD(sockets[0]),\n> > - UniqueFD(sockets[1]),\n> > + UniqueFD(std::move(sockets[0])),\n> > + UniqueFD(std::move(sockets[1])),\n> > };\n> > \n> > if (bind(std::move(socketFds[0])) < 0)\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > index 69831dabbe75..f2a95300d187 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > @@ -37,16 +37,13 @@ namespace RPi {\n> > DmaHeap::DmaHeap()\n> > {\n> > for (const char *name : heapNames) {\n> > - int ret = ::open(name, O_RDWR, 0);\n> > - if (ret < 0) {\n> > - ret = errno;\n> > - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > - << strerror(ret);\n> > - continue;\n> > - }\n> > + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0));\n> > + if (dmaHeapHandle_.isValid())\n> > + break;\n> > \n> > - dmaHeapHandle_ = UniqueFD(ret);\n> > - break;\n> > + int err = errno;\n> > + LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > + << strerror(err);\n> > }\n> > \n> > if (!dmaHeapHandle_.isValid())\n> > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > index 0e6b4e1dde58..dc8576d934b6 100644\n> > --- a/src/libcamera/process.cpp\n> > +++ b/src/libcamera/process.cpp\n> > @@ -134,8 +134,8 @@ ProcessManager::ProcessManager()\n> > LOG(Process, Fatal)\n> > << \"Failed to initialize pipe for signal handling\";\n> > \n> > - pipe_[0] = UniqueFD(pipe[0]);\n> > - pipe_[1] = UniqueFD(pipe[1]);\n> > + pipe_[0] = UniqueFD(std::move(pipe[0]));\n> > + pipe_[1] = UniqueFD(std::move(pipe[1]));\n> > \n> > sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> > sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index b4b89e2759b9..9d0d0077f4e4 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index,\n> > return {};\n> > }\n> > \n> > - return UniqueFD(expbuf.fd);\n> > + return UniqueFD(std::move(expbuf.fd));\n> > }\n> > \n> > /**\n> > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> > index eb3b591fec28..7e94e3ca450e 100644\n> > --- a/test/unique-fd.cpp\n> > +++ b/test/unique-fd.cpp\n> > @@ -39,7 +39,8 @@ protected:\n> > }\n> > \n> > /* Test creating UniqueFD from numerical file descriptor. */\n> > - UniqueFD fd2(fd_);\n> > + int numFd = fd_;\n> > + UniqueFD fd2(std::move(numFd));\n> > if (!fd2.isValid() || fd2.get() != fd_) {\n> > std::cout << \"Failed fd check (fd constructor)\"\n> > << std::endl;\n> > @@ -113,7 +114,7 @@ protected:\n> > }\n> > \n> > /* Test release. */\n> > - int numFd = fd2.release();\n> > + numFd = fd2.release();\n> > if (fd2.isValid() || fd2.get() != -1) {\n> > std::cout << \"Failed fd check (release)\"\n> > << std::endl;\n> > @@ -133,7 +134,7 @@ protected:\n> > }\n> > \n> > /* Test reset assignment. */\n> > - fd.reset(numFd);\n> > + fd.reset(std::move(numFd));\n> > if (!fd.isValid() || fd.get() != fd_) {\n> > std::cout << \"Failed fd check (reset assignment)\"\n> > << std::endl;\n> > @@ -168,7 +169,8 @@ protected:\n> > }\n> > \n> > {\n> > - UniqueFD fd4(fd_);\n> > + numFd = fd_;\n> > + UniqueFD fd4(std::move(numFd));\n> \n> This now looks like a hack ... Should it be removed? What is it really\n> testing?\n\nI need to keep the fd_ value valid, as I use it on the next line. When\nfd4 gets out of scope, the destructor will close the file descriptor,\nand isValid(fd_) should then fail. Without numFd = fd_, fd_ will then\nbecome -1, which defeats the point of the test.\n\n> > }\n> > \n> > if (isValidFd(fd_)) {","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 DA808BDB13\n\tfor ;\n\tFri, 3 Dec 2021 16:33:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E41B560832;\n\tFri, 3 Dec 2021 17:33:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 284E160725\n\tfor ;\n\tFri, 3 Dec 2021 17:33:19 +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 A11C4D6E;\n\tFri, 3 Dec 2021 17:33:18 +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=\"stzmhX9x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638549198;\n\tbh=IoImx3GP8ARu7UMNVYSpDnZeJbOTVG+MDmlGUKV98j4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=stzmhX9xbO8rhAkBmoZRSAMa4MEuZD5KPm1H4n/hs0bGJz8mFrce2Zrmb7SHGORDK\n\tE6NLLkgHFcKyR81WtSS0UeutvyLjmIp1LGNComgcj+cIYoBak2MzjtQnwTCS7+BlRp\n\tte8ztcv+Bf+LKaE7/bKNgS8JDYYpPN9Y4ACkQcQc=","Date":"Fri, 3 Dec 2021 18:32:52 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>\n\t<163854864760.3059017.7014221997850119859@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163854864760.3059017.7014221997850119859@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","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":21582,"web_url":"https://patchwork.libcamera.org/comment/21582/","msgid":"<163855030881.3059017.8951968654998097053@Monstersaurus>","date":"2021-12-03T16:51:48","subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-12-03 16:32:52)\n> Hi Kieran,\n> \n> On Fri, Dec 03, 2021 at 04:24:07PM +0000, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2021-11-30 03:38:20)\n> > > To avoid the risk of using a file descriptor whose ownership has been\n> > > transferred to a UniqueFD instance, pass an rvalue reference to the\n> > > constructor taking an int, and to the reset() function. The fd argument\n> > > is set to -1 upon return, making incorrect usage of the file descriptor\n> > > impossible.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > I'm not entirely sure about this patch. If desired, it will get squashed\n> > > with the one introducing UniqueFD.\n> > \n> > I kind of like that it makes the ownership explicit. I wasn't sure about\n> > the requirement to add std::move() ... but that makes it clear I guess.\n> > \n> > If you don't provide the std::move() will the compiler warn/error?\n> \n> UniqueFd fd(std::move(num));\n> UniqueFD fd(3);\n> fd.reset(std::move(num));\n> fd.reset(3);\n> \n> should all compile, while\n> \n> UniqueFd fd(num);\n> fd.reset(num);\n> \n> should result in a compilation error.\n> \n> > Also down in the testing, assigning to a new int, just so that one can\n> > be moved in now looks like a hack - so should either be removed or\n> > clarified in a comment.\n> > \n> > > ---\n> > > include/libcamera/base/unique_fd.h | 5 +++--\n> > > src/libcamera/base/shared_fd.cpp | 2 +-\n> > > src/libcamera/base/unique_fd.cpp | 10 ++++++++--\n> > > src/libcamera/ipc_unixsocket.cpp | 4 ++--\n> > > src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++---------\n> > > src/libcamera/process.cpp | 4 ++--\n> > > src/libcamera/v4l2_videodevice.cpp | 2 +-\n> > > test/unique-fd.cpp | 10 ++++++----\n> > > 8 files changed, 29 insertions(+), 23 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> > > index ae4d96b75797..a5ffb1a435dc 100644\n> > > --- a/include/libcamera/base/unique_fd.h\n> > > +++ b/include/libcamera/base/unique_fd.h\n> > > @@ -22,9 +22,10 @@ public:\n> > > {\n> > > }\n> > > \n> > > - explicit UniqueFD(int fd)\n> > > + explicit UniqueFD(int &&fd)\n> > > : fd_(fd)\n> > > {\n> > > + fd = -1;\n> > > }\n> > > \n> > > UniqueFD(UniqueFD &&other)\n> > > @@ -50,7 +51,7 @@ public:\n> > > return fd;\n> > > }\n> > > \n> > > - void reset(int fd = -1);\n> > > + void reset(int &&fd = -1);\n> > > \n> > > void swap(UniqueFD &other)\n> > > {\n> > > diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp\n> > > index 0c8b93a47f85..8aecd34038bd 100644\n> > > --- a/src/libcamera/base/shared_fd.cpp\n> > > +++ b/src/libcamera/base/shared_fd.cpp\n> > > @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const\n> > > << \"Failed to dup() fd: \" << strerror(-ret);\n> > > }\n> > > \n> > > - return UniqueFD(dupFd);\n> > > + return UniqueFD(std::move(dupFd));\n> > > }\n> > > \n> > > SharedFD::Descriptor::Descriptor(int fd, bool duplicate)\n> > > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> > > index 83d6919cf623..7a961dbc01d0 100644\n> > > --- a/src/libcamera/base/unique_fd.cpp\n> > > +++ b/src/libcamera/base/unique_fd.cpp\n> > > @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\n> > > */\n> > > \n> > > /**\n> > > - * \\fn UniqueFD::UniqueFD(int fd)\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> > > + * Ownership of the file descriptor is transferred to the UniqueFD instance.\n> > > + * Upon return \\a fd is set to -1.\n> > > */\n> > > \n> > > /**\n> > > @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\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> > > + * Upon return \\a fd is set to -1.\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> > > +void UniqueFD::reset(int &&fd)\n> > > {\n> > > ASSERT(!isValid() || fd != fd_);\n> > > \n> > > @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd)\n> > > \n> > > if (fd >= 0)\n> > > close(fd);\n> > > +\n> > > + fd = -1;\n> > > }\n> > > \n> > > /**\n> > > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> > > index 1980d374cea8..32bd6533d192 100644\n> > > --- a/src/libcamera/ipc_unixsocket.cpp\n> > > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > > @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create()\n> > > }\n> > > \n> > > std::array socketFds{\n> > > - UniqueFD(sockets[0]),\n> > > - UniqueFD(sockets[1]),\n> > > + UniqueFD(std::move(sockets[0])),\n> > > + UniqueFD(std::move(sockets[1])),\n> > > };\n> > > \n> > > if (bind(std::move(socketFds[0])) < 0)\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > > index 69831dabbe75..f2a95300d187 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > > @@ -37,16 +37,13 @@ namespace RPi {\n> > > DmaHeap::DmaHeap()\n> > > {\n> > > for (const char *name : heapNames) {\n> > > - int ret = ::open(name, O_RDWR, 0);\n> > > - if (ret < 0) {\n> > > - ret = errno;\n> > > - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > > - << strerror(ret);\n> > > - continue;\n> > > - }\n> > > + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0));\n> > > + if (dmaHeapHandle_.isValid())\n> > > + break;\n> > > \n> > > - dmaHeapHandle_ = UniqueFD(ret);\n> > > - break;\n> > > + int err = errno;\n> > > + LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > > + << strerror(err);\n> > > }\n> > > \n> > > if (!dmaHeapHandle_.isValid())\n> > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > > index 0e6b4e1dde58..dc8576d934b6 100644\n> > > --- a/src/libcamera/process.cpp\n> > > +++ b/src/libcamera/process.cpp\n> > > @@ -134,8 +134,8 @@ ProcessManager::ProcessManager()\n> > > LOG(Process, Fatal)\n> > > << \"Failed to initialize pipe for signal handling\";\n> > > \n> > > - pipe_[0] = UniqueFD(pipe[0]);\n> > > - pipe_[1] = UniqueFD(pipe[1]);\n> > > + pipe_[0] = UniqueFD(std::move(pipe[0]));\n> > > + pipe_[1] = UniqueFD(std::move(pipe[1]));\n> > > \n> > > sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> > > sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index b4b89e2759b9..9d0d0077f4e4 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index,\n> > > return {};\n> > > }\n> > > \n> > > - return UniqueFD(expbuf.fd);\n> > > + return UniqueFD(std::move(expbuf.fd));\n> > > }\n> > > \n> > > /**\n> > > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> > > index eb3b591fec28..7e94e3ca450e 100644\n> > > --- a/test/unique-fd.cpp\n> > > +++ b/test/unique-fd.cpp\n> > > @@ -39,7 +39,8 @@ protected:\n> > > }\n> > > \n> > > /* Test creating UniqueFD from numerical file descriptor. */\n> > > - UniqueFD fd2(fd_);\n> > > + int numFd = fd_;\n> > > + UniqueFD fd2(std::move(numFd));\n> > > if (!fd2.isValid() || fd2.get() != fd_) {\n> > > std::cout << \"Failed fd check (fd constructor)\"\n> > > << std::endl;\n> > > @@ -113,7 +114,7 @@ protected:\n> > > }\n> > > \n> > > /* Test release. */\n> > > - int numFd = fd2.release();\n> > > + numFd = fd2.release();\n> > > if (fd2.isValid() || fd2.get() != -1) {\n> > > std::cout << \"Failed fd check (release)\"\n> > > << std::endl;\n> > > @@ -133,7 +134,7 @@ protected:\n> > > }\n> > > \n> > > /* Test reset assignment. */\n> > > - fd.reset(numFd);\n> > > + fd.reset(std::move(numFd));\n> > > if (!fd.isValid() || fd.get() != fd_) {\n> > > std::cout << \"Failed fd check (reset assignment)\"\n> > > << std::endl;\n> > > @@ -168,7 +169,8 @@ protected:\n> > > }\n> > > \n> > > {\n> > > - UniqueFD fd4(fd_);\n> > > + numFd = fd_;\n> > > + UniqueFD fd4(std::move(numFd));\n> > \n> > This now looks like a hack ... Should it be removed? What is it really\n> > testing?\n> \n> I need to keep the fd_ value valid, as I use it on the next line. When\n> fd4 gets out of scope, the destructor will close the file descriptor,\n> and isValid(fd_) should then fail. Without numFd = fd_, fd_ will then\n> become -1, which defeats the point of the test.\n\nOk, it wasn't obvious from the context of the patch, or the short scope.\n\nMaybe documenting the intentions would help:\n\n /*\n * Ensure that the UniqueFD closes the file handle.\n *\n * We need to work from a copy of the file handle so we can keep the\n * fd_ number to validate it was closed at the system level while the\n * construction parameter will be correctly re-assigned to -1.\n */\n\n> \n> > > }\n> > > \n> > > if (isValidFd(fd_)) {\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 28699BDB13\n\tfor ;\n\tFri, 3 Dec 2021 16:51:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7044660832;\n\tFri, 3 Dec 2021 17:51:53 +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 E3EBD60725\n\tfor ;\n\tFri, 3 Dec 2021 17:51:51 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89CCEA59;\n\tFri, 3 Dec 2021 17:51:51 +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=\"IfaRlJlr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638550311;\n\tbh=fLjb2xU8xIY+BX6YWX//zSYw7TK3e/SDWVhNsGxnhpY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=IfaRlJlrhAyDtxvFBXiu/nIr4aLbsYbptiooLCrfGTM10ikJE0CJ92PGmLy8pzfM3\n\tD/Dc7cFxJR7od79N8TancHYF5ATRVtVV8ml1qVFwCjC+jftUX/tKkUwHQbqJfV1i1e\n\tr3MbeUOvA9msZvxIimfcMBfTiYL9rJG+kI94fuUk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>\n\t<163854864760.3059017.7014221997850119859@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ","Date":"Fri, 03 Dec 2021 16:51:48 +0000","Message-ID":"<163855030881.3059017.8951968654998097053@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","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":21583,"web_url":"https://patchwork.libcamera.org/comment/21583/","msgid":"","date":"2021-12-03T17:05:37","subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 03, 2021 at 06:32:53PM +0200, Laurent Pinchart wrote:\n> On Fri, Dec 03, 2021 at 04:24:07PM +0000, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2021-11-30 03:38:20)\n> > > To avoid the risk of using a file descriptor whose ownership has been\n> > > transferred to a UniqueFD instance, pass an rvalue reference to the\n> > > constructor taking an int, and to the reset() function. The fd argument\n> > > is set to -1 upon return, making incorrect usage of the file descriptor\n> > > impossible.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > I'm not entirely sure about this patch. If desired, it will get squashed\n> > > with the one introducing UniqueFD.\n> > \n> > I kind of like that it makes the ownership explicit. I wasn't sure about\n> > the requirement to add std::move() ... but that makes it clear I guess.\n> > \n> > If you don't provide the std::move() will the compiler warn/error?\n> \n> UniqueFd fd(std::move(num));\n> UniqueFD fd(3);\n> fd.reset(std::move(num));\n> fd.reset(3);\n> \n> should all compile, while\n> \n> UniqueFd fd(num);\n> fd.reset(num);\n> \n> should result in a compilation error.\n\nI've discussed this a bit further on #c++-general on liberachat,\ncomparing UniqueFD to std::unique_ptr<> and pondering about why the\nstd::unique_ptr<> constructor that takes a pointer doesn't instead take\nan rvalue reference to the pointer, and set it to null. Three different\npeople didn't like the idea of the int && argument:\n\n- [It's] probably not a good idea to modify the caller. Consider the\n original int fd to be a \"non-owning reference\" much like a T* would\n be. Also it's occasionally useful to _use_ the original pointer\n anyway, rather than trying to juggle the unique_ptr and call .get().\n e.g. you want to actually return a T*/T& to the resource you created,\n but you're owning it internally, and you created it someplace slightly\n incovenient (e.g. a map or similar). Similarly you may want to\n have-owned an fd but still return its value for select() or something.\n tldr, \"own\" doesn't mean \"hide\".\n\n- [I] would copy what unique_ptr does. When you use that ctor of\n unique_ptr you're basically saying \"yes, I gurantee that this pointer\n can now be owned by you\". It's up to the programmer to ensure that\n that's correct.\n\n- I would just have the ctor take an int and make the ctor explicit. If\n someone's using your class, they should know what it's for.\n\nThe constructor is already explicit.\n\nI think I will get the rest of this series integrated without this\npatch, which we can discuss separately and merge on top of desired.\n\n> > Also down in the testing, assigning to a new int, just so that one can\n> > be moved in now looks like a hack - so should either be removed or\n> > clarified in a comment.\n> > \n> > > ---\n> > > include/libcamera/base/unique_fd.h | 5 +++--\n> > > src/libcamera/base/shared_fd.cpp | 2 +-\n> > > src/libcamera/base/unique_fd.cpp | 10 ++++++++--\n> > > src/libcamera/ipc_unixsocket.cpp | 4 ++--\n> > > src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++---------\n> > > src/libcamera/process.cpp | 4 ++--\n> > > src/libcamera/v4l2_videodevice.cpp | 2 +-\n> > > test/unique-fd.cpp | 10 ++++++----\n> > > 8 files changed, 29 insertions(+), 23 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h\n> > > index ae4d96b75797..a5ffb1a435dc 100644\n> > > --- a/include/libcamera/base/unique_fd.h\n> > > +++ b/include/libcamera/base/unique_fd.h\n> > > @@ -22,9 +22,10 @@ public:\n> > > {\n> > > }\n> > > \n> > > - explicit UniqueFD(int fd)\n> > > + explicit UniqueFD(int &&fd)\n> > > : fd_(fd)\n> > > {\n> > > + fd = -1;\n> > > }\n> > > \n> > > UniqueFD(UniqueFD &&other)\n> > > @@ -50,7 +51,7 @@ public:\n> > > return fd;\n> > > }\n> > > \n> > > - void reset(int fd = -1);\n> > > + void reset(int &&fd = -1);\n> > > \n> > > void swap(UniqueFD &other)\n> > > {\n> > > diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp\n> > > index 0c8b93a47f85..8aecd34038bd 100644\n> > > --- a/src/libcamera/base/shared_fd.cpp\n> > > +++ b/src/libcamera/base/shared_fd.cpp\n> > > @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const\n> > > << \"Failed to dup() fd: \" << strerror(-ret);\n> > > }\n> > > \n> > > - return UniqueFD(dupFd);\n> > > + return UniqueFD(std::move(dupFd));\n> > > }\n> > > \n> > > SharedFD::Descriptor::Descriptor(int fd, bool duplicate)\n> > > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp\n> > > index 83d6919cf623..7a961dbc01d0 100644\n> > > --- a/src/libcamera/base/unique_fd.cpp\n> > > +++ b/src/libcamera/base/unique_fd.cpp\n> > > @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\n> > > */\n> > > \n> > > /**\n> > > - * \\fn UniqueFD::UniqueFD(int fd)\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> > > + * Ownership of the file descriptor is transferred to the UniqueFD instance.\n> > > + * Upon return \\a fd is set to -1.\n> > > */\n> > > \n> > > /**\n> > > @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)\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> > > + * Upon return \\a fd is set to -1.\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> > > +void UniqueFD::reset(int &&fd)\n> > > {\n> > > ASSERT(!isValid() || fd != fd_);\n> > > \n> > > @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd)\n> > > \n> > > if (fd >= 0)\n> > > close(fd);\n> > > +\n> > > + fd = -1;\n> > > }\n> > > \n> > > /**\n> > > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> > > index 1980d374cea8..32bd6533d192 100644\n> > > --- a/src/libcamera/ipc_unixsocket.cpp\n> > > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > > @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create()\n> > > }\n> > > \n> > > std::array socketFds{\n> > > - UniqueFD(sockets[0]),\n> > > - UniqueFD(sockets[1]),\n> > > + UniqueFD(std::move(sockets[0])),\n> > > + UniqueFD(std::move(sockets[1])),\n> > > };\n> > > \n> > > if (bind(std::move(socketFds[0])) < 0)\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > > index 69831dabbe75..f2a95300d187 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > > @@ -37,16 +37,13 @@ namespace RPi {\n> > > DmaHeap::DmaHeap()\n> > > {\n> > > for (const char *name : heapNames) {\n> > > - int ret = ::open(name, O_RDWR, 0);\n> > > - if (ret < 0) {\n> > > - ret = errno;\n> > > - LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > > - << strerror(ret);\n> > > - continue;\n> > > - }\n> > > + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0));\n> > > + if (dmaHeapHandle_.isValid())\n> > > + break;\n> > > \n> > > - dmaHeapHandle_ = UniqueFD(ret);\n> > > - break;\n> > > + int err = errno;\n> > > + LOG(RPI, Debug) << \"Failed to open \" << name << \": \"\n> > > + << strerror(err);\n> > > }\n> > > \n> > > if (!dmaHeapHandle_.isValid())\n> > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> > > index 0e6b4e1dde58..dc8576d934b6 100644\n> > > --- a/src/libcamera/process.cpp\n> > > +++ b/src/libcamera/process.cpp\n> > > @@ -134,8 +134,8 @@ ProcessManager::ProcessManager()\n> > > LOG(Process, Fatal)\n> > > << \"Failed to initialize pipe for signal handling\";\n> > > \n> > > - pipe_[0] = UniqueFD(pipe[0]);\n> > > - pipe_[1] = UniqueFD(pipe[1]);\n> > > + pipe_[0] = UniqueFD(std::move(pipe[0]));\n> > > + pipe_[1] = UniqueFD(std::move(pipe[1]));\n> > > \n> > > sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);\n> > > sigEvent_->activated.connect(this, &ProcessManager::sighandler);\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index b4b89e2759b9..9d0d0077f4e4 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index,\n> > > return {};\n> > > }\n> > > \n> > > - return UniqueFD(expbuf.fd);\n> > > + return UniqueFD(std::move(expbuf.fd));\n> > > }\n> > > \n> > > /**\n> > > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp\n> > > index eb3b591fec28..7e94e3ca450e 100644\n> > > --- a/test/unique-fd.cpp\n> > > +++ b/test/unique-fd.cpp\n> > > @@ -39,7 +39,8 @@ protected:\n> > > }\n> > > \n> > > /* Test creating UniqueFD from numerical file descriptor. */\n> > > - UniqueFD fd2(fd_);\n> > > + int numFd = fd_;\n> > > + UniqueFD fd2(std::move(numFd));\n> > > if (!fd2.isValid() || fd2.get() != fd_) {\n> > > std::cout << \"Failed fd check (fd constructor)\"\n> > > << std::endl;\n> > > @@ -113,7 +114,7 @@ protected:\n> > > }\n> > > \n> > > /* Test release. */\n> > > - int numFd = fd2.release();\n> > > + numFd = fd2.release();\n> > > if (fd2.isValid() || fd2.get() != -1) {\n> > > std::cout << \"Failed fd check (release)\"\n> > > << std::endl;\n> > > @@ -133,7 +134,7 @@ protected:\n> > > }\n> > > \n> > > /* Test reset assignment. */\n> > > - fd.reset(numFd);\n> > > + fd.reset(std::move(numFd));\n> > > if (!fd.isValid() || fd.get() != fd_) {\n> > > std::cout << \"Failed fd check (reset assignment)\"\n> > > << std::endl;\n> > > @@ -168,7 +169,8 @@ protected:\n> > > }\n> > > \n> > > {\n> > > - UniqueFD fd4(fd_);\n> > > + numFd = fd_;\n> > > + UniqueFD fd4(std::move(numFd));\n> > \n> > This now looks like a hack ... Should it be removed? What is it really\n> > testing?\n> \n> I need to keep the fd_ value valid, as I use it on the next line. When\n> fd4 gets out of scope, the destructor will close the file descriptor,\n> and isValid(fd_) should then fail. Without numFd = fd_, fd_ will then\n> become -1, which defeats the point of the test.\n> \n> > > }\n> > > \n> > > if (isValidFd(fd_)) {","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 C9D8EBF415\n\tfor ;\n\tFri, 3 Dec 2021 17:06:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FB9B60832;\n\tFri, 3 Dec 2021 18:06:05 +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 1574560725\n\tfor ;\n\tFri, 3 Dec 2021 18:06:04 +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 7D05BA59;\n\tFri, 3 Dec 2021 18:06:03 +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=\"Vz5DMBNr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638551163;\n\tbh=erY+xTqy9luO3iagIiWcCAqs5ZWuZ0VnqfWf4Y4MH2g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Vz5DMBNrEwPRh42vXlfTmxkEgBGsnFV6c53ZFYtf71p6BcABrZXN3z8vWy0/t0oWw\n\thLSrXJpfrNVUNRyQbU0QPQ5OCCFSTM5ifwkatBTGteAo685LCWms8f1hflY7zqhF6L\n\tJHlhm/mhkybiQ+EUB+VPdwrDUKOdQWbukXBJJfg8=","Date":"Fri, 3 Dec 2021 19:05:37 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-23-laurent.pinchart@ideasonboard.com>\n\t<163854864760.3059017.7014221997850119859@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd:\n\tPass rvalue reference to constructor and reset()","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\" "}}]