[{"id":17412,"web_url":"https://patchwork.libcamera.org/comment/17412/","msgid":"","date":"2021-06-06T18:18:48","subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:\n> V4L2Device owns a file descriptor for a v4l2 device node. It\n> should be managed by ScopedFD avoid the leakage.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/v4l2_device.h | 9 +++++----\n> src/libcamera/v4l2_device.cpp | 17 +++++++----------\n> src/libcamera/v4l2_videodevice.cpp | 3 ++-\n> 3 files changed, 14 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index d006bf68..e0262de0 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -13,6 +13,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/log.h\"\n> @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable\n> {\n> public:\n> \tvoid close();\n> -\tbool isOpen() const { return fd_ != -1; }\n> +\tbool isOpen() const { return fd_.isValid(); }\n> \n> \tconst ControlInfoMap &controls() const { return controls_; }\n> \n> @@ -46,11 +47,11 @@ protected:\n> \t~V4L2Device();\n> \n> \tint open(unsigned int flags);\n> -\tint setFd(int fd);\n> +\tint setFd(ScopedFD fd);\n\nShould this take an rvalue reference to enable move semantics ?\n\n> \n> \tint ioctl(unsigned long request, void *argp);\n> \n> -\tint fd() const { return fd_; }\n> +\tint fd() const { return fd_.get(); }\n> \n> private:\n> \tvoid listControls();\n> @@ -64,7 +65,7 @@ private:\n> \tstd::vector> controlIds_;\n> \tControlInfoMap controls_;\n> \tstd::string deviceNode_;\n> -\tint fd_;\n> +\tScopedFD fd_;\n> \n> \tEventNotifier *fdEventNotifier_;\n> \tbool frameStartEnabled_;\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..4fbb2d60 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)\n> * at open() time, and the \\a logTag to prefix log messages with.\n> */\n> V4L2Device::V4L2Device(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> +\t: deviceNode_(deviceNode), fdEventNotifier_(nullptr),\n> \t frameStartEnabled_(false)\n> {\n> }\n> @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)\n> \t\treturn ret;\n> \t}\n> \n> -\tsetFd(ret);\n> +\tsetFd(ScopedFD(ret));\n> \n> \tlistControls();\n> \n> @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2Device::setFd(int fd)\n> +int V4L2Device::setFd(ScopedFD fd)\n> {\n> \tif (isOpen())\n> \t\treturn -EBUSY;\n> \n> -\tfd_ = fd;\n> +\tfd_ = std::move(fd);\n> \n> -\tfdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> +\tfdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);\n> \tfdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> \tfdEventNotifier_->setEnabled(false);\n> \n> @@ -138,10 +138,7 @@ void V4L2Device::close()\n> \n> \tdelete fdEventNotifier_;\n> \n> -\tif (::close(fd_) < 0)\n> -\t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> -\t\t\t\t << strerror(errno);\n> -\tfd_ = -1;\n> +\tfd_.reset();\n> }\n> \n> /**\n> @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> \t * Printing out an error message is usually better performed\n> \t * in the caller, which can provide more context.\n> \t */\n> -\tif (::ioctl(fd_, request, argp) < 0)\n> +\tif (::ioctl(fd_.get(), request, argp) < 0)\n> \t\treturn -errno;\n> \n> \treturn 0;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 12c09dc7..0bf3b5f5 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -22,6 +22,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \"libcamera/internal/event_notifier.h\"\n> #include \"libcamera/internal/log.h\"\n> @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> \t\treturn ret;\n> \t}\n> \n> -\tret = V4L2Device::setFd(newFd);\n> +\tret = V4L2Device::setFd(ScopedFD(newFd));\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> \t\t\t\t << strerror(-ret);","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 18D8EC3206\n\tfor ;\n\tSun, 6 Jun 2021 18:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75C0B6892B;\n\tSun, 6 Jun 2021 20:19:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A8DD602A7\n\tfor ;\n\tSun, 6 Jun 2021 20:19:02 +0200 (CEST)","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 7E8C8EF;\n\tSun, 6 Jun 2021 20:19:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"C5GzkNZZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623003541;\n\tbh=Gr0ttcEmid4vLb/o2r7ScVlHgol5Z3xt8q1Vf/v7jmE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C5GzkNZZL8qo14IrlgLdw+i6Fn4jk7BqaLIXuBqs+xgSXC9iBDyTebnvXTp1dQ533\n\t5DoXdTOSrDiqd0HOeAmDaJnoHr9pvQfIcIlOjpbXOp6xEX3cNCUBBrqm5IIOSweW5M\n\to1RQl4cYMLEopzvIhzgyJORI3t5v6QOOl1fPVsn4=","Date":"Sun, 6 Jun 2021 21:18:48 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-6-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210415083843.3399502-6-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","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":17432,"web_url":"https://patchwork.libcamera.org/comment/17432/","msgid":"","date":"2021-06-07T03:34:00","subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:\n> > V4L2Device owns a file descriptor for a v4l2 device node. It\n> > should be managed by ScopedFD avoid the leakage.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 9 +++++----\n> > src/libcamera/v4l2_device.cpp | 17 +++++++----------\n> > src/libcamera/v4l2_videodevice.cpp | 3 ++-\n> > 3 files changed, 14 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h\n> b/include/libcamera/internal/v4l2_device.h\n> > index d006bf68..e0262de0 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -13,6 +13,7 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/log.h\"\n> > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable\n> > {\n> > public:\n> > void close();\n> > - bool isOpen() const { return fd_ != -1; }\n> > + bool isOpen() const { return fd_.isValid(); }\n> >\n> > const ControlInfoMap &controls() const { return controls_; }\n> >\n> > @@ -46,11 +47,11 @@ protected:\n> > ~V4L2Device();\n> >\n> > int open(unsigned int flags);\n> > - int setFd(int fd);\n> > + int setFd(ScopedFD fd);\n>\n> Should this take an rvalue reference to enable move semantics ?\n>\n>\nSince ScopedFD is move-only, when declaring ScopedFD, it forces a caller to\npass with move semantics.\nI would take a value as if ScopedFD is unique_ptr.\n\n-Hiro\n\n> >\n> > int ioctl(unsigned long request, void *argp);\n> >\n> > - int fd() const { return fd_; }\n> > + int fd() const { return fd_.get(); }\n> >\n> > private:\n> > void listControls();\n> > @@ -64,7 +65,7 @@ private:\n> > std::vector> controlIds_;\n> > ControlInfoMap controls_;\n> > std::string deviceNode_;\n> > - int fd_;\n> > + ScopedFD fd_;\n> >\n> > EventNotifier *fdEventNotifier_;\n> > bool frameStartEnabled_;\n> > diff --git a/src/libcamera/v4l2_device.cpp\n> b/src/libcamera/v4l2_device.cpp\n> > index decd19ef..4fbb2d60 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)\n> > * at open() time, and the \\a logTag to prefix log messages with.\n> > */\n> > V4L2Device::V4L2Device(const std::string &deviceNode)\n> > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),\n> > frameStartEnabled_(false)\n> > {\n> > }\n> > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)\n> > return ret;\n> > }\n> >\n> > - setFd(ret);\n> > + setFd(ScopedFD(ret));\n> >\n> > listControls();\n> >\n> > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2Device::setFd(int fd)\n> > +int V4L2Device::setFd(ScopedFD fd)\n> > {\n> > if (isOpen())\n> > return -EBUSY;\n> >\n> > - fd_ = fd;\n> > + fd_ = std::move(fd);\n> >\n> > - fdEventNotifier_ = new EventNotifier(fd_,\n> EventNotifier::Exception);\n> > + fdEventNotifier_ = new EventNotifier(fd_.get(),\n> EventNotifier::Exception);\n> > fdEventNotifier_->activated.connect(this,\n> &V4L2Device::eventAvailable);\n> > fdEventNotifier_->setEnabled(false);\n> >\n> > @@ -138,10 +138,7 @@ void V4L2Device::close()\n> >\n> > delete fdEventNotifier_;\n> >\n> > - if (::close(fd_) < 0)\n> > - LOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > - << strerror(errno);\n> > - fd_ = -1;\n> > + fd_.reset();\n> > }\n> >\n> > /**\n> > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void\n> *argp)\n> > * Printing out an error message is usually better performed\n> > * in the caller, which can provide more context.\n> > */\n> > - if (::ioctl(fd_, request, argp) < 0)\n> > + if (::ioctl(fd_.get(), request, argp) < 0)\n> > return -errno;\n> >\n> > return 0;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp\n> b/src/libcamera/v4l2_videodevice.cpp\n> > index 12c09dc7..0bf3b5f5 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -22,6 +22,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \"libcamera/internal/event_notifier.h\"\n> > #include \"libcamera/internal/log.h\"\n> > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum\n> v4l2_buf_type type)\n> > return ret;\n> > }\n> >\n> > - ret = V4L2Device::setFd(newFd);\n> > + ret = V4L2Device::setFd(ScopedFD(newFd));\n> > if (ret < 0) {\n> > LOG(V4L2, Error) << \"Failed to set file handle: \"\n> > << strerror(-ret);\n>\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 68201C320B\n\tfor ;\n\tMon, 7 Jun 2021 03:34:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCFD968926;\n\tMon, 7 Jun 2021 05:34:12 +0200 (CEST)","from mail-ej1-x632.google.com (mail-ej1-x632.google.com\n\t[IPv6:2a00:1450:4864:20::632])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20DE56029E\n\tfor ;\n\tMon, 7 Jun 2021 05:34:11 +0200 (CEST)","by mail-ej1-x632.google.com with SMTP id k25so18750595eja.9\n\tfor ;\n\tSun, 06 Jun 2021 20:34:11 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Oo3c66r8\"; 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=2K+lZ+C+qsa+p6bJlcCRoHQpo37/L0tXjv6tdPwoxZQ=;\n\tb=Oo3c66r8ctCgC67XfQ9F4q+heTxoGRuiuCXtURNUYisxHOPUzGKzeel8J0+dXxT5Xc\n\tZp+nurQGzv/owNU2mYwLXlbAm5ANdr6vZgVUn0S/mpwIRlRI8QJyqP0Qk28WLj/q1tBY\n\tGS3AXXpNubOMXApzDhxw+DNCnLTiOzeiTKVsQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=2K+lZ+C+qsa+p6bJlcCRoHQpo37/L0tXjv6tdPwoxZQ=;\n\tb=aJATPTzNByG/IMAuckmL6AXdN/4AFphXfC3TGZdlGs8SYGokNHsMx/J5LN+Ei8a9ZB\n\tJegX+QXa4HgV6DLhxVMVyyRrt8JkImsuiG+Oz+vhdOPDHRPkjIR7dDU3HhkmSXUGARfR\n\tyV3uyLafBKMredOpJit7WmW/JmtwSDWEGR+p/AabkRTDoefugGdcqthKUbciekC3TuUA\n\t8Olvzxabm1KXU1YEfOIf1mNmHXPp53tJ1u7HusJxr01R7MBGlk30xYWwdvqCWQSiKJu5\n\taWy2mjMTqmdagc9Hfv40I7cPmmLLwKPOKhzo19egkfG19IgKPssFqdzjPCPZLKM2dpje\n\ttt/A==","X-Gm-Message-State":"AOAM533EEq8b6avy4BOLwLjRxQ9F98bq0b9eQesG3hz2AFOCVtquOKOi\n\tuw7u1qYcL8TxvrYs4GPcaL4RQOnrmaiJq7H0E5S9tA==","X-Google-Smtp-Source":"ABdhPJwKRaTOBiEOJdrfGQQLOQcjdE8WFKfCouqOp/hwvO+6jBrx1FEMJn5B6lhsROMPw0JFdZPf6fDgpEWZLFav1YE=","X-Received":"by 2002:a17:906:2459:: with SMTP id\n\ta25mr15599059ejb.306.1623036850671; \n\tSun, 06 Jun 2021 20:34:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-6-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 7 Jun 2021 12:34:00 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000f126d705c424b6f6\"","Subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17840,"web_url":"https://patchwork.libcamera.org/comment/17840/","msgid":"","date":"2021-06-28T02:49:54","subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote:\n> On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote:\n> > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:\n> > > V4L2Device owns a file descriptor for a v4l2 device node. It\n> > > should be managed by ScopedFD avoid the leakage.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/internal/v4l2_device.h | 9 +++++----\n> > > src/libcamera/v4l2_device.cpp | 17 +++++++----------\n> > > src/libcamera/v4l2_videodevice.cpp | 3 ++-\n> > > 3 files changed, 14 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index d006bf68..e0262de0 100644\n> > > --- a/include/libcamera/internal/v4l2_device.h\n> > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > @@ -13,6 +13,7 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \"libcamera/internal/log.h\"\n> > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable\n> > > {\n> > > public:\n> > > void close();\n> > > - bool isOpen() const { return fd_ != -1; }\n> > > + bool isOpen() const { return fd_.isValid(); }\n> > >\n> > > const ControlInfoMap &controls() const { return controls_; }\n> > >\n> > > @@ -46,11 +47,11 @@ protected:\n> > > ~V4L2Device();\n> > >\n> > > int open(unsigned int flags);\n> > > - int setFd(int fd);\n> > > + int setFd(ScopedFD fd);\n> >\n> > Should this take an rvalue reference to enable move semantics ?\n>\n> Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to\n> pass with move semantics.\n\nThis means that a new ScopedFD instance will be created using the move\nconstructor, to be passed to the setFd() function, which will internally\nuse the move assignment operator. We can avoid the move constructor by\npassing an rvalue reference here. It would also clearly indicate to the\nuser that a move is the expected behaviour of setFd(), while this is\ncurrently implicit only, as a consequence of ScopedFD deleting the copy\nconstructor.\n\n> I would take a value as if ScopedFD is unique_ptr.\n> \n> > >\n> > > int ioctl(unsigned long request, void *argp);\n> > >\n> > > - int fd() const { return fd_; }\n> > > + int fd() const { return fd_.get(); }\n> > >\n> > > private:\n> > > void listControls();\n> > > @@ -64,7 +65,7 @@ private:\n> > > std::vector> controlIds_;\n> > > ControlInfoMap controls_;\n> > > std::string deviceNode_;\n> > > - int fd_;\n> > > + ScopedFD fd_;\n> > >\n> > > EventNotifier *fdEventNotifier_;\n> > > bool frameStartEnabled_;\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index decd19ef..4fbb2d60 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)\n> > > * at open() time, and the \\a logTag to prefix log messages with.\n> > > */\n> > > V4L2Device::V4L2Device(const std::string &deviceNode)\n> > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),\n> > > frameStartEnabled_(false)\n> > > {\n> > > }\n> > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)\n> > > return ret;\n> > > }\n> > >\n> > > - setFd(ret);\n> > > + setFd(ScopedFD(ret));\n> > >\n> > > listControls();\n> > >\n> > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)\n> > > *\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int V4L2Device::setFd(int fd)\n> > > +int V4L2Device::setFd(ScopedFD fd)\n> > > {\n> > > if (isOpen())\n> > > return -EBUSY;\n> > >\n> > > - fd_ = fd;\n> > > + fd_ = std::move(fd);\n> > >\n> > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);\n> > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> > > fdEventNotifier_->setEnabled(false);\n> > >\n> > > @@ -138,10 +138,7 @@ void V4L2Device::close()\n> > >\n> > > delete fdEventNotifier_;\n> > >\n> > > - if (::close(fd_) < 0)\n> > > - LOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > > - << strerror(errno);\n> > > - fd_ = -1;\n> > > + fd_.reset();\n> > > }\n> > >\n> > > /**\n> > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> > > * Printing out an error message is usually better performed\n> > > * in the caller, which can provide more context.\n> > > */\n> > > - if (::ioctl(fd_, request, argp) < 0)\n> > > + if (::ioctl(fd_.get(), request, argp) < 0)\n> > > return -errno;\n> > >\n> > > return 0;\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 12c09dc7..0bf3b5f5 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -22,6 +22,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \"libcamera/internal/event_notifier.h\"\n> > > #include \"libcamera/internal/log.h\"\n> > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > > return ret;\n> > > }\n> > >\n> > > - ret = V4L2Device::setFd(newFd);\n> > > + ret = V4L2Device::setFd(ScopedFD(newFd));\n> > > if (ret < 0) {\n> > > LOG(V4L2, Error) << \"Failed to set file handle: \"\n> > > << strerror(-ret);","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 3EDBFC321A\n\tfor ;\n\tMon, 28 Jun 2021 02:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B7A2684D5;\n\tMon, 28 Jun 2021 04:49:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B1EF6028C\n\tfor ;\n\tMon, 28 Jun 2021 04:49:56 +0200 (CEST)","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 0EAE457E;\n\tMon, 28 Jun 2021 04:49:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YZLWDhkA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624848596;\n\tbh=MqAGVHWvXdsyA3HnURxxEcxXWjptNIRKdZFt1CnU20U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YZLWDhkAMvfdGnb7C1XuTbt1+uehMkX2u2RhjX8M7pAVUulM3TsCTrUJmpxb8TnBV\n\tjitGCB64bOFoMmstOuP0iZ3y3uFsWpJx0FHncRmJeASmboKGjrcDAdqqulI6poXtxu\n\t/LeDZ+s0uSdOMOABvk7NMdBR0ZDakZnMROwa7Z6Y=","Date":"Mon, 28 Jun 2021 05:49:54 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-6-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17904,"web_url":"https://patchwork.libcamera.org/comment/17904/","msgid":"","date":"2021-06-28T21:05:48","subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 28, 2021 at 11:50 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote:\n> > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote:\n> > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:\n> > > > V4L2Device owns a file descriptor for a v4l2 device node. It\n> > > > should be managed by ScopedFD avoid the leakage.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/internal/v4l2_device.h | 9 +++++----\n> > > > src/libcamera/v4l2_device.cpp | 17 +++++++----------\n> > > > src/libcamera/v4l2_videodevice.cpp | 3 ++-\n> > > > 3 files changed, 14 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > index d006bf68..e0262de0 100644\n> > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > @@ -13,6 +13,7 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \"libcamera/internal/log.h\"\n> > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable\n> > > > {\n> > > > public:\n> > > > void close();\n> > > > - bool isOpen() const { return fd_ != -1; }\n> > > > + bool isOpen() const { return fd_.isValid(); }\n> > > >\n> > > > const ControlInfoMap &controls() const { return controls_; }\n> > > >\n> > > > @@ -46,11 +47,11 @@ protected:\n> > > > ~V4L2Device();\n> > > >\n> > > > int open(unsigned int flags);\n> > > > - int setFd(int fd);\n> > > > + int setFd(ScopedFD fd);\n> > >\n> > > Should this take an rvalue reference to enable move semantics ?\n> >\n> > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to\n> > pass with move semantics.\n>\n> This means that a new ScopedFD instance will be created using the move\n> constructor, to be passed to the setFd() function, which will internally\n> use the move assignment operator. We can avoid the move constructor by\n> passing an rvalue reference here. It would also clearly indicate to the\n> user that a move is the expected behaviour of setFd(), while this is\n> currently implicit only, as a consequence of ScopedFD deleting the copy\n> constructor.\n>\n\nYou're right. If we think about the overhead, we should avoid this\npass-by-value.\nI generally pass-by-value to clarify the ownership. There is no chance\nthat a caller's ScopedFD is valid when the function returns.\nWith pass-by-rvalue-reference, if a function doesn't invalidate the\nScopedFD, it can still be valid when the function returns.\nI think that's why Google C++ style guide suggests rvalue reference in\nspecific functions only.\nhttps://google.github.io/styleguide/cppguide.html#Rvalue_references\nAlthough setFd can be regarded as \"&&-qualified methods that logically\n\"consume\" *this\".\nI am fine to use rvalue reference here as setFd should consume the\npassed ScopedFD.\nWhat do you think?\n\n-Hiro\n> > I would take a value as if ScopedFD is unique_ptr.\n> >\n> > > >\n> > > > int ioctl(unsigned long request, void *argp);\n> > > >\n> > > > - int fd() const { return fd_; }\n> > > > + int fd() const { return fd_.get(); }\n> > > >\n> > > > private:\n> > > > void listControls();\n> > > > @@ -64,7 +65,7 @@ private:\n> > > > std::vector> controlIds_;\n> > > > ControlInfoMap controls_;\n> > > > std::string deviceNode_;\n> > > > - int fd_;\n> > > > + ScopedFD fd_;\n> > > >\n> > > > EventNotifier *fdEventNotifier_;\n> > > > bool frameStartEnabled_;\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index decd19ef..4fbb2d60 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)\n> > > > * at open() time, and the \\a logTag to prefix log messages with.\n> > > > */\n> > > > V4L2Device::V4L2Device(const std::string &deviceNode)\n> > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),\n> > > > frameStartEnabled_(false)\n> > > > {\n> > > > }\n> > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - setFd(ret);\n> > > > + setFd(ScopedFD(ret));\n> > > >\n> > > > listControls();\n> > > >\n> > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)\n> > > > *\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2Device::setFd(int fd)\n> > > > +int V4L2Device::setFd(ScopedFD fd)\n> > > > {\n> > > > if (isOpen())\n> > > > return -EBUSY;\n> > > >\n> > > > - fd_ = fd;\n> > > > + fd_ = std::move(fd);\n> > > >\n> > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);\n> > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> > > > fdEventNotifier_->setEnabled(false);\n> > > >\n> > > > @@ -138,10 +138,7 @@ void V4L2Device::close()\n> > > >\n> > > > delete fdEventNotifier_;\n> > > >\n> > > > - if (::close(fd_) < 0)\n> > > > - LOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > > > - << strerror(errno);\n> > > > - fd_ = -1;\n> > > > + fd_.reset();\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> > > > * Printing out an error message is usually better performed\n> > > > * in the caller, which can provide more context.\n> > > > */\n> > > > - if (::ioctl(fd_, request, argp) < 0)\n> > > > + if (::ioctl(fd_.get(), request, argp) < 0)\n> > > > return -errno;\n> > > >\n> > > > return 0;\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index 12c09dc7..0bf3b5f5 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -22,6 +22,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \"libcamera/internal/event_notifier.h\"\n> > > > #include \"libcamera/internal/log.h\"\n> > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - ret = V4L2Device::setFd(newFd);\n> > > > + ret = V4L2Device::setFd(ScopedFD(newFd));\n> > > > if (ret < 0) {\n> > > > LOG(V4L2, Error) << \"Failed to set file handle: \"\n> > > > << strerror(-ret);\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 2F8E7C3220\n\tfor ;\n\tMon, 28 Jun 2021 21:06:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46310684D4;\n\tMon, 28 Jun 2021 23:06:01 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 027C36028C\n\tfor ;\n\tMon, 28 Jun 2021 23:05:59 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id t3so6506246edt.12\n\tfor ;\n\tMon, 28 Jun 2021 14:05:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"LU5ZtO5g\"; 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=nDl5dh+G/zSzgdSH526x6QopWlyAZ9Osl1lToAH3oGM=;\n\tb=LU5ZtO5gQkUw7iRigddPPOEfpzkc2BkknezBwQmxjF2gUMc/2qzULtXuPNLFCLaCMO\n\tGpdH3hRxTVrCWxsP5PzG+vYsTBLu1wrsLgj9RQsJIqfgbcjWR6SX0+TFeHMa3v1mxZjO\n\t2ngLrm2AuO54I/ULsVM30PpqGIfAZv47y6b3E=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=nDl5dh+G/zSzgdSH526x6QopWlyAZ9Osl1lToAH3oGM=;\n\tb=m0+KQ7vsDva2MfgLCOn+1uJefaKk2SJ8gXdQdj7HnHXmL9/izvP+GMaME5oBYYWen6\n\tU+6nmg2kdir3t3SxHG/Qd8ft5005gXHa7bDCd8JvcdTYsV6G+PiVHLbha5ZGGaAwg+hs\n\tYIRlLUwp4rvvlIXRT+GYU1W7cD7gpUVZ9qT7I0lLbFhD90uTnIQPP3jQznaaVEtO5u8H\n\ttT9RQ09Ot+vTqxjfU9hHU1BZVpU7GmWMgvk5eyAesLWju2Uebal9BxUle1E5oFF5LRXy\n\t2hhR+qZJjXZLnXxLCfdZoACG5gaVN3BJgYXhiDDDlxMr6sWZ+myCj80bLpk0aiWHb3re\n\tWSCw==","X-Gm-Message-State":"AOAM531O5rWdrEByWtcjII9LNzy6NGV8mKCrPnzzJVIosYWfHp5JbBmu\n\t+u8vzWaBtoEiFSihzXbuiKNEv8nsKDEFSRmdFh4EpQ==","X-Google-Smtp-Source":"ABdhPJx/VdrvKYLlBXR8cNTrfp80GN3mQPR/37NLq90sOVh3VoITc6VR/rbr5vFYS0lqH9Ll0PCVCjeNfLiaZ+3OONw=","X-Received":"by 2002:a05:6402:151:: with SMTP id\n\ts17mr35145786edu.233.1624914359576; \n\tMon, 28 Jun 2021 14:05:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-6-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 29 Jun 2021 06:05:48 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21295,"web_url":"https://patchwork.libcamera.org/comment/21295/","msgid":"","date":"2021-11-28T15:49:48","subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Replying to an old e-mail,\n\nOn Mon, Jun 28, 2021 at 05:49:56AM +0300, Laurent Pinchart wrote:\n> On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote:\n> > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote:\n> > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:\n> > > > V4L2Device owns a file descriptor for a v4l2 device node. It\n> > > > should be managed by ScopedFD avoid the leakage.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/internal/v4l2_device.h | 9 +++++----\n> > > > src/libcamera/v4l2_device.cpp | 17 +++++++----------\n> > > > src/libcamera/v4l2_videodevice.cpp | 3 ++-\n> > > > 3 files changed, 14 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > index d006bf68..e0262de0 100644\n> > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > @@ -13,6 +13,7 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \"libcamera/internal/log.h\"\n> > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable\n> > > > {\n> > > > public:\n> > > > void close();\n> > > > - bool isOpen() const { return fd_ != -1; }\n> > > > + bool isOpen() const { return fd_.isValid(); }\n> > > >\n> > > > const ControlInfoMap &controls() const { return controls_; }\n> > > >\n> > > > @@ -46,11 +47,11 @@ protected:\n> > > > ~V4L2Device();\n> > > >\n> > > > int open(unsigned int flags);\n> > > > - int setFd(int fd);\n> > > > + int setFd(ScopedFD fd);\n> > >\n> > > Should this take an rvalue reference to enable move semantics ?\n> >\n> > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to\n> > pass with move semantics.\n> \n> This means that a new ScopedFD instance will be created using the move\n> constructor, to be passed to the setFd() function, which will internally\n> use the move assignment operator. We can avoid the move constructor by\n> passing an rvalue reference here. It would also clearly indicate to the\n> user that a move is the expected behaviour of setFd(), while this is\n> currently implicit only, as a consequence of ScopedFD deleting the copy\n> constructor.\n\nIt recently came to my attention that passing a std::unique_ptr<> by\nrvalue reference is usually a bad idea. The class is cheap to move, and\nwith a pass-by-value call, it's clear that move is being implemented as\nstd::unique_ptr<> has no copy constructor. When passing by rvalue\nreference, the caller has to use std::move(), but it's up to the callee\nto actually perform the move. Compare the following:\n\nvoid foo(std::unique_ptr ptr)\n{\n}\n\nvoid foo(std::unique_ptr &&ptr)\n{\n}\n\nwith foo being called as\n\n\tstd::unique_ptr f = ...;\n\tfoo(std::move(f));\n\nIn the first case, ptr is constructed by moving f. ptr gets destroyed at\nthe end of foo(), and because foo() is empty, it still owns the pointer,\nwhich then gets deleted. When foo() returns, f doesn't own a pointer\nanymore.\n\nIn the second case, f will still own the pointer after foo() returns. Of\ncourse, if foo() was implemented as\n\nvoid foo(std::unique_ptr &&ptr)\n{\n\tglobalPtr = std::move(ptr);\n}\n\nthen f would not own the pointer after foo() returns. The point here is\nthat passing a std::unique_ptr<> rvalue reference makes the behaviour\nmore dependent on the implementation of the function, while passing a\nvalue ensures that f will not own the pointer anymore when foo()\nreturns, regardless of how it's implemented.\n\nThe same applies to ScopedFD.\n\nOf course, if the class was expensive to move, it would be a different\ncases.\n\n> > I would take a value as if ScopedFD is unique_ptr.\n> > \n> > > >\n> > > > int ioctl(unsigned long request, void *argp);\n> > > >\n> > > > - int fd() const { return fd_; }\n> > > > + int fd() const { return fd_.get(); }\n> > > >\n> > > > private:\n> > > > void listControls();\n> > > > @@ -64,7 +65,7 @@ private:\n> > > > std::vector> controlIds_;\n> > > > ControlInfoMap controls_;\n> > > > std::string deviceNode_;\n> > > > - int fd_;\n> > > > + ScopedFD fd_;\n> > > >\n> > > > EventNotifier *fdEventNotifier_;\n> > > > bool frameStartEnabled_;\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index decd19ef..4fbb2d60 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)\n> > > > * at open() time, and the \\a logTag to prefix log messages with.\n> > > > */\n> > > > V4L2Device::V4L2Device(const std::string &deviceNode)\n> > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),\n> > > > frameStartEnabled_(false)\n> > > > {\n> > > > }\n> > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - setFd(ret);\n> > > > + setFd(ScopedFD(ret));\n> > > >\n> > > > listControls();\n> > > >\n> > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)\n> > > > *\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2Device::setFd(int fd)\n> > > > +int V4L2Device::setFd(ScopedFD fd)\n> > > > {\n> > > > if (isOpen())\n> > > > return -EBUSY;\n> > > >\n> > > > - fd_ = fd;\n> > > > + fd_ = std::move(fd);\n> > > >\n> > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);\n> > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> > > > fdEventNotifier_->setEnabled(false);\n> > > >\n> > > > @@ -138,10 +138,7 @@ void V4L2Device::close()\n> > > >\n> > > > delete fdEventNotifier_;\n> > > >\n> > > > - if (::close(fd_) < 0)\n> > > > - LOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > > > - << strerror(errno);\n> > > > - fd_ = -1;\n> > > > + fd_.reset();\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> > > > * Printing out an error message is usually better performed\n> > > > * in the caller, which can provide more context.\n> > > > */\n> > > > - if (::ioctl(fd_, request, argp) < 0)\n> > > > + if (::ioctl(fd_.get(), request, argp) < 0)\n> > > > return -errno;\n> > > >\n> > > > return 0;\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index 12c09dc7..0bf3b5f5 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -22,6 +22,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \"libcamera/internal/event_notifier.h\"\n> > > > #include \"libcamera/internal/log.h\"\n> > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - ret = V4L2Device::setFd(newFd);\n> > > > + ret = V4L2Device::setFd(ScopedFD(newFd));\n> > > > if (ret < 0) {\n> > > > LOG(V4L2, Error) << \"Failed to set file handle: \"\n> > > > << strerror(-ret);","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 B7DF6BF415\n\tfor ;\n\tSun, 28 Nov 2021 15:50:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 122AD604FC;\n\tSun, 28 Nov 2021 16:50:15 +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 E41516011C\n\tfor ;\n\tSun, 28 Nov 2021 16:50:12 +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 4CFB8F1;\n\tSun, 28 Nov 2021 16:50:12 +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=\"oQEzuO7Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638114612;\n\tbh=VTGuYGqTIXb9Yapbz59eNJrN52DSxZ88dnGOYvvxQAk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oQEzuO7ZOtZnTc3dggqyFHs5Xn5P/35adleoekT7UNO2YH8auyGMsIMkiPrA+Etkd\n\tUA/EyRYFdpFwlmquad/jnqOx39KKM/yF4F4bmZtstf362ZEQcCzbzuTDdDKQV3q10j\n\tkXlsHq90qcx9G9BIkNskdbgmVAlcfLcwLUtPJes0=","Date":"Sun, 28 Nov 2021 17:49:48 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-6-hiroh@chromium.org>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21312,"web_url":"https://patchwork.libcamera.org/comment/21312/","msgid":"","date":"2021-11-29T12:53:53","subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Nov 29, 2021 at 12:50 AM Laurent Pinchart\n wrote:\n>\n> Replying to an old e-mail,\n>\n> On Mon, Jun 28, 2021 at 05:49:56AM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote:\n> > > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote:\n> > > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:\n> > > > > V4L2Device owns a file descriptor for a v4l2 device node. It\n> > > > > should be managed by ScopedFD avoid the leakage.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > include/libcamera/internal/v4l2_device.h | 9 +++++----\n> > > > > src/libcamera/v4l2_device.cpp | 17 +++++++----------\n> > > > > src/libcamera/v4l2_videodevice.cpp | 3 ++-\n> > > > > 3 files changed, 14 insertions(+), 15 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > > index d006bf68..e0262de0 100644\n> > > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > > @@ -13,6 +13,7 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \"libcamera/internal/log.h\"\n> > > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable\n> > > > > {\n> > > > > public:\n> > > > > void close();\n> > > > > - bool isOpen() const { return fd_ != -1; }\n> > > > > + bool isOpen() const { return fd_.isValid(); }\n> > > > >\n> > > > > const ControlInfoMap &controls() const { return controls_; }\n> > > > >\n> > > > > @@ -46,11 +47,11 @@ protected:\n> > > > > ~V4L2Device();\n> > > > >\n> > > > > int open(unsigned int flags);\n> > > > > - int setFd(int fd);\n> > > > > + int setFd(ScopedFD fd);\n> > > >\n> > > > Should this take an rvalue reference to enable move semantics ?\n> > >\n> > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to\n> > > pass with move semantics.\n> >\n> > This means that a new ScopedFD instance will be created using the move\n> > constructor, to be passed to the setFd() function, which will internally\n> > use the move assignment operator. We can avoid the move constructor by\n> > passing an rvalue reference here. It would also clearly indicate to the\n> > user that a move is the expected behaviour of setFd(), while this is\n> > currently implicit only, as a consequence of ScopedFD deleting the copy\n> > constructor.\n>\n> It recently came to my attention that passing a std::unique_ptr<> by\n> rvalue reference is usually a bad idea. The class is cheap to move, and\n> with a pass-by-value call, it's clear that move is being implemented as\n> std::unique_ptr<> has no copy constructor. When passing by rvalue\n> reference, the caller has to use std::move(), but it's up to the callee\n> to actually perform the move. Compare the following:\n>\n> void foo(std::unique_ptr ptr)\n> {\n> }\n>\n> void foo(std::unique_ptr &&ptr)\n> {\n> }\n>\n> with foo being called as\n>\n> std::unique_ptr f = ...;\n> foo(std::move(f));\n>\n> In the first case, ptr is constructed by moving f. ptr gets destroyed at\n> the end of foo(), and because foo() is empty, it still owns the pointer,\n> which then gets deleted. When foo() returns, f doesn't own a pointer\n> anymore.\n>\n> In the second case, f will still own the pointer after foo() returns. Of\n> course, if foo() was implemented as\n>\n> void foo(std::unique_ptr &&ptr)\n> {\n> globalPtr = std::move(ptr);\n> }\n>\n> then f would not own the pointer after foo() returns. The point here is\n> that passing a std::unique_ptr<> rvalue reference makes the behaviour\n> more dependent on the implementation of the function, while passing a\n> value ensures that f will not own the pointer anymore when foo()\n> returns, regardless of how it's implemented.\n>\n> The same applies to ScopedFD.\n>\n> Of course, if the class was expensive to move, it would be a different\n> cases.\n>\n\nYou're right. I was told the same thing a few years ago by my\ncolleague that an argument must be a value if the function makes sure\nto take the ownership.\nOtherwise, a destructor of passed value by std::move() can be called\nin the end of scope in the caller side.\n\n-Hiro\n> > > I would take a value as if ScopedFD is unique_ptr.\n> > >\n> > > > >\n> > > > > int ioctl(unsigned long request, void *argp);\n> > > > >\n> > > > > - int fd() const { return fd_; }\n> > > > > + int fd() const { return fd_.get(); }\n> > > > >\n> > > > > private:\n> > > > > void listControls();\n> > > > > @@ -64,7 +65,7 @@ private:\n> > > > > std::vector> controlIds_;\n> > > > > ControlInfoMap controls_;\n> > > > > std::string deviceNode_;\n> > > > > - int fd_;\n> > > > > + ScopedFD fd_;\n> > > > >\n> > > > > EventNotifier *fdEventNotifier_;\n> > > > > bool frameStartEnabled_;\n> > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > > index decd19ef..4fbb2d60 100644\n> > > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)\n> > > > > * at open() time, and the \\a logTag to prefix log messages with.\n> > > > > */\n> > > > > V4L2Device::V4L2Device(const std::string &deviceNode)\n> > > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> > > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),\n> > > > > frameStartEnabled_(false)\n> > > > > {\n> > > > > }\n> > > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)\n> > > > > return ret;\n> > > > > }\n> > > > >\n> > > > > - setFd(ret);\n> > > > > + setFd(ScopedFD(ret));\n> > > > >\n> > > > > listControls();\n> > > > >\n> > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)\n> > > > > *\n> > > > > * \\return 0 on success or a negative error code otherwise\n> > > > > */\n> > > > > -int V4L2Device::setFd(int fd)\n> > > > > +int V4L2Device::setFd(ScopedFD fd)\n> > > > > {\n> > > > > if (isOpen())\n> > > > > return -EBUSY;\n> > > > >\n> > > > > - fd_ = fd;\n> > > > > + fd_ = std::move(fd);\n> > > > >\n> > > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> > > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);\n> > > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> > > > > fdEventNotifier_->setEnabled(false);\n> > > > >\n> > > > > @@ -138,10 +138,7 @@ void V4L2Device::close()\n> > > > >\n> > > > > delete fdEventNotifier_;\n> > > > >\n> > > > > - if (::close(fd_) < 0)\n> > > > > - LOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> > > > > - << strerror(errno);\n> > > > > - fd_ = -1;\n> > > > > + fd_.reset();\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> > > > > * Printing out an error message is usually better performed\n> > > > > * in the caller, which can provide more context.\n> > > > > */\n> > > > > - if (::ioctl(fd_, request, argp) < 0)\n> > > > > + if (::ioctl(fd_.get(), request, argp) < 0)\n> > > > > return -errno;\n> > > > >\n> > > > > return 0;\n> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > index 12c09dc7..0bf3b5f5 100644\n> > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > @@ -22,6 +22,7 @@\n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > #include \"libcamera/internal/event_notifier.h\"\n> > > > > #include \"libcamera/internal/log.h\"\n> > > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > > > > return ret;\n> > > > > }\n> > > > >\n> > > > > - ret = V4L2Device::setFd(newFd);\n> > > > > + ret = V4L2Device::setFd(ScopedFD(newFd));\n> > > > > if (ret < 0) {\n> > > > > LOG(V4L2, Error) << \"Failed to set file handle: \"\n> > > > > << strerror(-ret);\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 E3109BDB13\n\tfor ;\n\tMon, 29 Nov 2021 12:54:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4FB4605A6;\n\tMon, 29 Nov 2021 13:54:07 +0100 (CET)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5674E60592\n\tfor ;\n\tMon, 29 Nov 2021 13:54:06 +0100 (CET)","by mail-ed1-x534.google.com with SMTP id t5so71678862edd.0\n\tfor ;\n\tMon, 29 Nov 2021 04:54:06 -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=\"eQJ5oFZz\"; 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=a8gvc8ANucp82fMqevYvM0+v7rKp3XzdhbWwXWrbS3U=;\n\tb=eQJ5oFZz9zKz1rHWb/bizVOguHJu2VTG//nV7XeGbsF8ypGsVtvZ7yiPR5MoJzhj6R\n\tIzdfXBVHbJPltayJz9/WI/l4FTFyTIpyAWEo5lr2hpdNJXfzhkixjBoc73dcPkC8KNuz\n\t7+v+uOVW7JfbKxOxF6z1+cr51PjLN+wO7uWVU=","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=a8gvc8ANucp82fMqevYvM0+v7rKp3XzdhbWwXWrbS3U=;\n\tb=vDC45ubllhWIx12RtnXwIClFIp/lJmi0lIvtudl1QvJ/gxNlcDaA3Dg2hjwK27jD1z\n\tK51zgMWPuwWYSPyTwLaDqo8n688WSjHICFcSHXaRAy9ro272qo0qFWWwNeKj/uCpxuyK\n\twwhLvbHVmzHGuHBd1iBZM2jl81OFHYP4f+JvoRFs4tgLBPQ6vkA50aHk5PRdMXv9odZz\n\tPZxqT36sKKYqnxJ+/KlULUidefF4+ua8x+9Fv6kHHxQZY/8Fwv1wngvoeJQi5Hbsc2/2\n\tfaTe+ufJhiHiwsYAarMf0veuyeTz6QsFJ+is1ff6MUi8pZOdCKVcwgCC+kZEFN3QEwoe\n\tQyTQ==","X-Gm-Message-State":"AOAM533fCacR1V7/5BoiC7siXwDr7JFSqQW7sUnMu5ujK8KO1cO6fxHp\n\txGaWbX+s4JjzpVoIwiAPywKuJjDTIcCb6AYsuKegm/LDXX0=","X-Google-Smtp-Source":"ABdhPJwb2WicYymMpYzfD2uYBbxjDKle9Yjdx43U0sz8fNtU1kcyLpdWJc95pPpMDP8haEC+I6k7HpG1zEIRj4j4tOQ=","X-Received":"by 2002:a17:906:7310:: with SMTP id\n\tdi16mr57980299ejc.92.1638190444759; \n\tMon, 29 Nov 2021 04:54:04 -0800 (PST)","MIME-Version":"1.0","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-6-hiroh@chromium.org>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 21:53:53 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use\n\tScopedFD for a file descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]