[{"id":21322,"web_url":"https://patchwork.libcamera.org/comment/21322/","msgid":"","date":"2021-11-29T13:57:43","subject":"Re: [libcamera-devel] [PATCH v3 12/17] libcamera: media_device:\n\tManage fd by UniqueFD","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 MediaDevice for a media device\n> node 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/media_device.h | 3 +-\n> src/libcamera/media_device.cpp | 36 ++++++++++-------------\n> 2 files changed, 17 insertions(+), 22 deletions(-)\n>\n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index d636e34a8573..af0b25b731eb 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -16,6 +16,7 @@\n>\n> #include \n> #include \n> +#include \n>\n> #include \"libcamera/internal/media_object.h\"\n>\n> @@ -82,7 +83,7 @@ private:\n> unsigned int version_;\n> unsigned int hwRevision_;\n>\n> - int fd_;\n> + UniqueFD fd_;\n> bool valid_;\n> bool acquired_;\n> bool lockOwner_;\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index aa93da75c593..4df0a27fe193 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> - : deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false),\n> + : deviceNode_(deviceNode), valid_(false), acquired_(false),\n> lockOwner_(false)\n> {\n> }\n>\n> MediaDevice::~MediaDevice()\n> {\n> - if (fd_ != -1)\n> - ::close(fd_);\n> + fd_.reset();\n> clear();\n> }\n>\n> @@ -143,14 +142,14 @@ void MediaDevice::release()\n> */\n> bool MediaDevice::lock()\n> {\n> - if (fd_ == -1)\n> + if (!fd_.isValid())\n> return false;\n>\n> /* Do not allow nested locking in the same libcamera instance. */\n> if (lockOwner_)\n> return false;\n>\n> - if (lockf(fd_, F_TLOCK, 0))\n> + if (lockf(fd_.get(), F_TLOCK, 0))\n> return false;\n>\n> lockOwner_ = true;\n> @@ -169,7 +168,7 @@ bool MediaDevice::lock()\n> */\n> void MediaDevice::unlock()\n> {\n> - if (fd_ == -1)\n> + if (!fd_.isValid())\n> return;\n>\n> if (!lockOwner_)\n> @@ -177,7 +176,7 @@ void MediaDevice::unlock()\n>\n> lockOwner_ = false;\n>\n> - lockf(fd_, F_ULOCK, 0);\n> + lockf(fd_.get(), F_ULOCK, 0);\n> }\n>\n> /**\n> @@ -220,7 +219,7 @@ int MediaDevice::populate()\n> return ret;\n>\n> struct media_device_info info = {};\n> - ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);\n> + ret = ioctl(fd_.get(), MEDIA_IOC_DEVICE_INFO, &info);\n> if (ret) {\n> ret = -errno;\n> LOG(MediaDevice, Error)\n> @@ -243,7 +242,7 @@ int MediaDevice::populate()\n> topology.ptr_links = reinterpret_cast(links);\n> topology.ptr_pads = reinterpret_cast(pads);\n>\n> - ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> + ret = ioctl(fd_.get(), MEDIA_IOC_G_TOPOLOGY, &topology);\n> if (ret < 0) {\n> ret = -errno;\n> LOG(MediaDevice, Error)\n> @@ -481,20 +480,19 @@ int MediaDevice::disableLinks()\n> */\n> int MediaDevice::open()\n> {\n> - if (fd_ != -1) {\n> + if (fd_.isValid()) {\n> LOG(MediaDevice, Error) << \"MediaDevice already open\";\n> return -EBUSY;\n> }\n>\n> - int ret = ::open(deviceNode_.c_str(), O_RDWR);\n> - if (ret < 0) {\n> - ret = -errno;\n> + fd_ = UniqueFD(::open(deviceNode_.c_str(), O_RDWR));\n> + if (!fd_.isValid()) {\n> + int ret = -errno;\n> LOG(MediaDevice, Error)\n> << \"Failed to open media device at \"\n> << deviceNode_ << \": \" << strerror(-ret);\n> return ret;\n> }\n> - fd_ = ret;\n>\n> return 0;\n> }\n> @@ -514,11 +512,7 @@ int MediaDevice::open()\n> */\n> void MediaDevice::close()\n> {\n> - if (fd_ == -1)\n> - return;\n> -\n> - ::close(fd_);\n> - fd_ = -1;\n> + fd_.reset();\n> }\n>\n> /**\n> @@ -756,7 +750,7 @@ void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)\n> struct media_entity_desc desc = {};\n> desc.id = entity->id;\n>\n> - int ret = ioctl(fd_, MEDIA_IOC_ENUM_ENTITIES, &desc);\n> + int ret = ioctl(fd_.get(), MEDIA_IOC_ENUM_ENTITIES, &desc);\n> if (ret < 0) {\n> ret = -errno;\n> LOG(MediaDevice, Debug)\n> @@ -799,7 +793,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)\n>\n> linkDesc.flags = flags;\n>\n> - int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);\n> + int ret = ioctl(fd_.get(), MEDIA_IOC_SETUP_LINK, &linkDesc);\n> if (ret) {\n> ret = -errno;\n> LOG(MediaDevice, Error)\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 BF87FBDB13\n\tfor ;\n\tMon, 29 Nov 2021 13:57:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6DAFD605A4;\n\tMon, 29 Nov 2021 14:57:56 +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 2349260592\n\tfor ;\n\tMon, 29 Nov 2021 14:57:55 +0100 (CET)","by mail-ed1-x533.google.com with SMTP id r11so72338867edd.9\n\tfor ;\n\tMon, 29 Nov 2021 05:57:55 -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=\"OuPCj0U0\"; 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=gh8YXYVR/0mTylxF78X4qGG75F+AddofmYsjgp88yYY=;\n\tb=OuPCj0U0ZkStXQtpxHJW8VvFy3c+dDdpNOgEVHGinOBFmpZmRrs/Mid+0kArp76ck7\n\teOe6HVWYVVEJr5jc4BxoRQOj4uR74a3qnju/QdAL/FmG2rxe9/8UzcUbG9cVPjydjlvq\n\tG9bN9ke/CzuHjp5sQSXfjmcl9gMDHVd8IcZwM=","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=gh8YXYVR/0mTylxF78X4qGG75F+AddofmYsjgp88yYY=;\n\tb=Ww418kqMwmD8HJjoUVZQZ6iUa5FsJPsy53X+7NkYVBQ+5T3opXgOr+8EbsnGDAFOtq\n\teC15be6WBBnwKqgbZ3AKtYt+GINloCcQIm1M7atE51o4RSjtOcVkqvpLyKHmCrwGm/Jt\n\tAmHtIvCUAXmck1bxcrW21XMsjjwi0tLowSmgqJADukzCVhdVDHVSiXB4E8ktZdB4Xq4A\n\tz9UZRnWNyhCP90dLhByFpPIsrSDsoy37ENBz0udCdRtNJvC92wCOcZoXmNbRFDSbmYuO\n\tHt8KQjrJJiCfOxToExkuHBEO7p4AyWH45Id+WL+C3pFg9/DAvnEbqs1LFl15VUDRMJUI\n\thxHA==","X-Gm-Message-State":"AOAM5320X2+i/yCDnRJpTm1Nd2ZRfux276B4F9Z+g1e/burY2MRTJHcw\n\tmuRJPzacAouoXoM2L75p4JSp4iCBJ0ar/81aDM9OisHZnkw=","X-Google-Smtp-Source":"ABdhPJzV0JS5g/oQnbXrhwINwgWswnNFr8ijza9ozxf1l11ysfpXwahQUaReq7nT5DFt39/dPw+FPE0b1zMWNquUlts=","X-Received":"by 2002:a50:ef12:: with SMTP id\n\tm18mr75644178eds.381.1638194274778; \n\tMon, 29 Nov 2021 05:57:54 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-13-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211128235752.10836-13-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 22:57:43 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 12/17] libcamera: media_device:\n\tManage fd by UniqueFD","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":21342,"web_url":"https://patchwork.libcamera.org/comment/21342/","msgid":"<20211129154324.yh2csfgs7f245uq5@uno.localdomain>","date":"2021-11-29T15:43:24","subject":"Re: [libcamera-devel] [PATCH v3 12/17] libcamera: media_device:\n\tManage fd by UniqueFD","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:47AM +0200, Laurent Pinchart wrote:\n> From: Hirokazu Honda \n>\n> Manages a file descriptor owned by MediaDevice for a media device\n> node 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/media_device.h | 3 +-\n> src/libcamera/media_device.cpp | 36 ++++++++++-------------\n> 2 files changed, 17 insertions(+), 22 deletions(-)\n>\n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index d636e34a8573..af0b25b731eb 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -16,6 +16,7 @@\n>\n> #include \n> #include \n> +#include \n>\n> #include \"libcamera/internal/media_object.h\"\n>\n> @@ -82,7 +83,7 @@ private:\n> \tunsigned int version_;\n> \tunsigned int hwRevision_;\n>\n> -\tint fd_;\n> +\tUniqueFD 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 aa93da75c593..4df0a27fe193 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,19 @@ int MediaDevice::disableLinks()\n> */\n> int MediaDevice::open()\n> {\n> -\tif (fd_ != -1) {\n> +\tif (fd_.isValid()) {\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> +\tfd_ = UniqueFD(::open(deviceNode_.c_str(), O_RDWR));\n> +\tif (!fd_.isValid()) {\n> +\t\tint ret = -errno;\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> \treturn 0;\n> }\n> @@ -514,11 +512,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> @@ -756,7 +750,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> @@ -799,7 +793,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)\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 BAFE9BF415\n\tfor ;\n\tMon, 29 Nov 2021 15:42:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DCA8605A6;\n\tMon, 29 Nov 2021 16:42:32 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B943A60592\n\tfor ;\n\tMon, 29 Nov 2021 16:42:31 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 50BE71BF208;\n\tMon, 29 Nov 2021 15:42:31 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 16:43:24 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129154324.yh2csfgs7f245uq5@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-13-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211128235752.10836-13-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 12/17] libcamera: media_device:\n\tManage fd by UniqueFD","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\" "}}]