[{"id":21323,"web_url":"https://patchwork.libcamera.org/comment/21323/","msgid":"","date":"2021-11-29T13:58:28","subject":"Re: [libcamera-devel] [PATCH v3 13/17] libcamera: v4l2_device: Use\n\tUniqueFD 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 8:58 AM Laurent Pinchart\n wrote:\n>\n> From: Hirokazu Honda \n>\n> Manages a file descriptor owned by V4L2Device for a v4l2 device node\n> by UniqueFD.\n>\n> Signed-off-by: Hirokazu Honda \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Hirokazu Honda if applicable.\n> ---\n> Changes since v2:\n>\n> - Fix errno handling\n> ---\n> include/libcamera/internal/v4l2_device.h | 9 +++++----\n> src/libcamera/v4l2_device.cpp | 23 ++++++++++-------------\n> src/libcamera/v4l2_videodevice.cpp | 16 ++++++----------\n> 3 files changed, 21 insertions(+), 27 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 7816a290141d..8886b750ae29 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n>\n> #include \n>\n> @@ -27,7 +28,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> @@ -49,11 +50,11 @@ protected:\n> ~V4L2Device();\n>\n> int open(unsigned int flags);\n> - int setFd(int fd);\n> + int setFd(UniqueFD fd);\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> static ControlType v4l2CtrlType(uint32_t ctrlType);\n> @@ -72,7 +73,7 @@ private:\n> ControlIdMap controlIdMap_;\n> ControlInfoMap controls_;\n> std::string deviceNode_;\n> - int fd_;\n> + UniqueFD 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 9c783c9cbed1..39f360091f64 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> @@ -81,15 +81,15 @@ int V4L2Device::open(unsigned int flags)\n> return -EBUSY;\n> }\n>\n> - int ret = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags);\n> - if (ret < 0) {\n> - ret = -errno;\n> + UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));\n> + if (!fd.isValid()) {\n> + int ret = -errno;\n> LOG(V4L2, Error) << \"Failed to open V4L2 device: \"\n> << strerror(-ret);\n> return ret;\n> }\n>\n> - setFd(ret);\n> + setFd(std::move(fd));\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(UniqueFD 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> @@ -440,7 +437,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 0a85bcf6b3ff..c95626d33cc3 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \"libcamera/internal/formats.h\"\n> @@ -620,22 +621,17 @@ int V4L2VideoDevice::open()\n> */\n> int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> {\n> - int ret;\n> - int newFd;\n> -\n> - newFd = dup(handle);\n> - if (newFd < 0) {\n> - ret = -errno;\n> + UniqueFD newFd(dup(handle));\n> + if (!newFd.isValid()) {\n> LOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> - << strerror(-ret);\n> - return ret;\n> + << strerror(errno);\n> + return -errno;\n> }\n>\n> - ret = V4L2Device::setFd(newFd);\n> + int ret = V4L2Device::setFd(std::move(newFd));\n> if (ret < 0) {\n> LOG(V4L2, Error) << \"Failed to set file handle: \"\n> << strerror(-ret);\n> - ::close(newFd);\n> return ret;\n> }\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 5DAD7BDB13\n\tfor ;\n\tMon, 29 Nov 2021 13:58:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FBB3605A4;\n\tMon, 29 Nov 2021 14:58:42 +0100 (CET)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B257F60592\n\tfor ;\n\tMon, 29 Nov 2021 14:58:40 +0100 (CET)","by mail-ed1-x533.google.com with SMTP id x15so72209832edv.1\n\tfor ;\n\tMon, 29 Nov 2021 05:58:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"NhM8OEUm\"; 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=kAwlqVfkLE6hXldSX7TGyMQKWXV8fOD5DCvLTReFL0M=;\n\tb=NhM8OEUmXZJ2lj6MR1y4aSTwziV0QVajQa293GmW2xs/OZvD/Qsflzxga4nvndJUed\n\tfYBcpW/NXokNA7uVdqDoKYupJScrfmvdjT3p7UjThlVHi21oDoYEA6CUcVc3HyzXzBxB\n\teUvrEEN4GVw1D1vJzYVnhlvl6y6dM/X4/fU0c=","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=kAwlqVfkLE6hXldSX7TGyMQKWXV8fOD5DCvLTReFL0M=;\n\tb=BM9qgou2RYxkgEtrtojOfraUErNKn0NMmA4FNI+9N2SuRUu0wVQXwqcZgRHRpcSSa+\n\t0lowaan+U6wQ3KkiWSFb86T/WfQjb/ircZsa3gxRMUjaahX30xrKY/oCSr/hVgd+YEaS\n\tF9LHACd/zyHFKQCFmMF/C435frp6EEtpTPT/jjMU1HEfUZjrEeZ0aJZqFzEtWHUBVQfv\n\tu6NlFSI1MJYWAHubsADwLhEcZrkagiURAdglXrxMTwBk5Mq4FBwCoIQ7VvZJHd4Bf5qP\n\tp6oTIy0NW5bRPeWGKeamQpuks6pHdK/9vXM4cKea0x/L6iChfG2SVOUMv0tbPtGrEhVe\n\tPd6Q==","X-Gm-Message-State":"AOAM5308Y3baO1w2dnoLFuG3BEPRra6lBWmMyj1LC3y2sp6VbBbXf8gG\n\t6vPtgPtBtPRmG+HbXLseBabVQ5IBvrUUTQWmehEWLHlAYdA=","X-Google-Smtp-Source":"ABdhPJyJi1D7NVSLWa+kjhAOXuq9ttZTEl+W3ZyHz6BOSbTS72JGTM6S0qiJ7Xs+9cv476bAp+f41LKrk2JGmbbPziM=","X-Received":"by 2002:a17:906:7310:: with SMTP id\n\tdi16mr58395963ejc.92.1638194320229; \n\tMon, 29 Nov 2021 05:58:40 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-14-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211128235752.10836-14-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 22:58:28 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 13/17] libcamera: v4l2_device: Use\n\tUniqueFD 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":21344,"web_url":"https://patchwork.libcamera.org/comment/21344/","msgid":"<20211129155103.zg7zwhgzf4zummzb@uno.localdomain>","date":"2021-11-29T15:51:03","subject":"Re: [libcamera-devel] [PATCH v3 13/17] libcamera: v4l2_device: Use\n\tUniqueFD for a file descriptor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Nov 29, 2021 at 01:57:48AM +0200, Laurent Pinchart wrote:\n> From: Hirokazu Honda \n>\n> Manages a file descriptor owned by V4L2Device for a v4l2 device node\n> by UniqueFD.\n>\n> Signed-off-by: Hirokazu Honda \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> Changes since v2:\n>\n> - Fix errno handling\n> ---\n> include/libcamera/internal/v4l2_device.h | 9 +++++----\n> src/libcamera/v4l2_device.cpp | 23 ++++++++++-------------\n> src/libcamera/v4l2_videodevice.cpp | 16 ++++++----------\n> 3 files changed, 21 insertions(+), 27 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 7816a290141d..8886b750ae29 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n>\n> #include \n>\n> @@ -27,7 +28,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> @@ -49,11 +50,11 @@ protected:\n> \t~V4L2Device();\n>\n> \tint open(unsigned int flags);\n> -\tint setFd(int fd);\n> +\tint setFd(UniqueFD fd);\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> \tstatic ControlType v4l2CtrlType(uint32_t ctrlType);\n> @@ -72,7 +73,7 @@ private:\n> \tControlIdMap controlIdMap_;\n> \tControlInfoMap controls_;\n> \tstd::string deviceNode_;\n> -\tint fd_;\n> +\tUniqueFD 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 9c783c9cbed1..39f360091f64 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> @@ -81,15 +81,15 @@ int V4L2Device::open(unsigned int flags)\n> \t\treturn -EBUSY;\n> \t}\n>\n> -\tint ret = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags);\n> -\tif (ret < 0) {\n> -\t\tret = -errno;\n> +\tUniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));\n> +\tif (!fd.isValid()) {\n> +\t\tint ret = -errno;\n> \t\tLOG(V4L2, Error) << \"Failed to open V4L2 device: \"\n> \t\t\t\t << strerror(-ret);\n> \t\treturn ret;\n> \t}\n>\n> -\tsetFd(ret);\n> +\tsetFd(std::move(fd));\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(UniqueFD 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> @@ -440,7 +437,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 0a85bcf6b3ff..c95626d33cc3 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \"libcamera/internal/formats.h\"\n> @@ -620,22 +621,17 @@ int V4L2VideoDevice::open()\n> */\n> int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> {\n> -\tint ret;\n> -\tint newFd;\n> -\n> -\tnewFd = dup(handle);\n> -\tif (newFd < 0) {\n> -\t\tret = -errno;\n> +\tUniqueFD newFd(dup(handle));\n> +\tif (!newFd.isValid()) {\n> \t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> -\t\t\t\t << strerror(-ret);\n> -\t\treturn ret;\n> +\t\t\t\t << strerror(errno);\n> +\t\treturn -errno;\n> \t}\n>\n> -\tret = V4L2Device::setFd(newFd);\n> +\tint ret = V4L2Device::setFd(std::move(newFd));\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error) << \"Failed to set file handle: \"\n> \t\t\t\t << strerror(-ret);\n> -\t\t::close(newFd);\n> \t\treturn ret;\n> \t}\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 AB957BDB13\n\tfor ;\n\tMon, 29 Nov 2021 15:50:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F12C3605A4;\n\tMon, 29 Nov 2021 16:50:12 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49B6160592\n\tfor ;\n\tMon, 29 Nov 2021 16:50:11 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 9F402100014;\n\tMon, 29 Nov 2021 15:50:10 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 16:51:03 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129155103.zg7zwhgzf4zummzb@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-14-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211128235752.10836-14-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 13/17] libcamera: v4l2_device: Use\n\tUniqueFD 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\" "}}]