[{"id":17411,"web_url":"https://patchwork.libcamera.org/comment/17411/","msgid":"","date":"2021-06-06T18:11:29","subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: MediaDevice:\n\tManage fd by ScopedFD","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:38PM +0900, Hirokazu Honda wrote:\n> MediaDevice owns a file descriptor for a media device node. It\n> should be managed by ScopedFD to avoid the leakage.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/media_device.h | 3 ++-\n> src/libcamera/media_device.cpp | 33 ++++++++++-------------\n> 2 files changed, 16 insertions(+), 20 deletions(-)\n> \n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index c3292508..efdfbf36 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -14,6 +14,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/log.h\"\n> @@ -82,7 +83,7 @@ private:\n> \tunsigned int version_;\n> \tunsigned int hwRevision_;\n> \n> -\tint fd_;\n> +\tScopedFD fd_;\n> \tbool valid_;\n> \tbool acquired_;\n> \tbool lockOwner_;\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 9ec84e56..ad5efbf6 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -63,15 +63,14 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * populate() before the media graph can be queried.\n> */\n> MediaDevice::MediaDevice(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false),\n> +\t: deviceNode_(deviceNode), valid_(false), acquired_(false),\n> \t lockOwner_(false)\n> {\n> }\n> \n> MediaDevice::~MediaDevice()\n> {\n> -\tif (fd_ != -1)\n> -\t\t::close(fd_);\n> +\tfd_.reset();\n> \tclear();\n> }\n> \n> @@ -143,14 +142,14 @@ void MediaDevice::release()\n> */\n> bool MediaDevice::lock()\n> {\n> -\tif (fd_ == -1)\n> +\tif (!fd_.isValid())\n> \t\treturn false;\n> \n> \t/* Do not allow nested locking in the same libcamera instance. */\n> \tif (lockOwner_)\n> \t\treturn false;\n> \n> -\tif (lockf(fd_, F_TLOCK, 0))\n> +\tif (lockf(fd_.get(), F_TLOCK, 0))\n> \t\treturn false;\n> \n> \tlockOwner_ = true;\n> @@ -169,7 +168,7 @@ bool MediaDevice::lock()\n> */\n> void MediaDevice::unlock()\n> {\n> -\tif (fd_ == -1)\n> +\tif (!fd_.isValid())\n> \t\treturn;\n> \n> \tif (!lockOwner_)\n> @@ -177,7 +176,7 @@ void MediaDevice::unlock()\n> \n> \tlockOwner_ = false;\n> \n> -\tlockf(fd_, F_ULOCK, 0);\n> +\tlockf(fd_.get(), F_ULOCK, 0);\n> }\n> \n> /**\n> @@ -220,7 +219,7 @@ int MediaDevice::populate()\n> \t\treturn ret;\n> \n> \tstruct media_device_info info = {};\n> -\tret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);\n> +\tret = ioctl(fd_.get(), MEDIA_IOC_DEVICE_INFO, &info);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(MediaDevice, Error)\n> @@ -243,7 +242,7 @@ int MediaDevice::populate()\n> \t\ttopology.ptr_links = reinterpret_cast(links);\n> \t\ttopology.ptr_pads = reinterpret_cast(pads);\n> \n> -\t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> +\t\tret = ioctl(fd_.get(), MEDIA_IOC_G_TOPOLOGY, &topology);\n> \t\tif (ret < 0) {\n> \t\t\tret = -errno;\n> \t\t\tLOG(MediaDevice, Error)\n> @@ -481,20 +480,20 @@ int MediaDevice::disableLinks()\n> */\n> int MediaDevice::open()\n> {\n> -\tif (fd_ != -1) {\n> +\tif (!fd_.isValid()) {\n\nThis should be\n\n\tif (fd_.isValid()) {\n\n> \t\tLOG(MediaDevice, Error) << \"MediaDevice already open\";\n> \t\treturn -EBUSY;\n> \t}\n> \n> \tint ret = ::open(deviceNode_.c_str(), O_RDWR);\n> \tif (ret < 0) {\n> -\t\tret = -errno;\n\nThis doesn't seem correct.\n\n> \t\tLOG(MediaDevice, Error)\n> \t\t\t<< \"Failed to open media device at \"\n> \t\t\t<< deviceNode_ << \": \" << strerror(-ret);\n> \t\treturn ret;\n> \t}\n> -\tfd_ = ret;\n> +\n> +\tfd_ = ScopedFD(ret);\n> \n> \treturn 0;\n> }\n> @@ -514,11 +513,7 @@ int MediaDevice::open()\n> */\n> void MediaDevice::close()\n> {\n> -\tif (fd_ == -1)\n> -\t\treturn;\n> -\n> -\t::close(fd_);\n> -\tfd_ = -1;\n> +\tfd_.reset();\n> }\n> \n> /**\n> @@ -763,7 +758,7 @@ void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)\n> \tstruct media_entity_desc desc = {};\n> \tdesc.id = entity->id;\n> \n> -\tint ret = ioctl(fd_, MEDIA_IOC_ENUM_ENTITIES, &desc);\n> +\tint ret = ioctl(fd_.get(), MEDIA_IOC_ENUM_ENTITIES, &desc);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(MediaDevice, Debug)\n> @@ -806,7 +801,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)\n> \n> \tlinkDesc.flags = flags;\n> \n> -\tint ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);\n> +\tint ret = ioctl(fd_.get(), MEDIA_IOC_SETUP_LINK, &linkDesc);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(MediaDevice, Error)","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 83E12C320B\n\tfor ;\n\tSun, 6 Jun 2021 18:11:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 632A06892B;\n\tSun, 6 Jun 2021 20:11:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A7CFE602A7\n\tfor ;\n\tSun, 6 Jun 2021 20:11:42 +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 205A7EF;\n\tSun, 6 Jun 2021 20:11:42 +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=\"jp+8RJpK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623003102;\n\tbh=lQDdwx4aanXqCM8eayv2FHCBhIvhKx6vDZ92XMejAyk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jp+8RJpKCXfvJKjOD0vmfk33mXlAXDasOwRbqcgEDq7h01b81QQwAIyCocnxn2jTd\n\tSG/MF7XjirKskEEAO/VXo3u3z1jfEWIShli8fwJDawynZ0J+8Pp/yu/uMxAAlx6UtI\n\t6YHOePbb3OodqTRTrQiex/tpBuiKJrWIpAXTVIgo=","Date":"Sun, 6 Jun 2021 21:11:29 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-5-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210415083843.3399502-5-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 05/10] libcamera: MediaDevice:\n\tManage fd by ScopedFD","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\" "}}]