[{"id":21409,"web_url":"https://patchwork.libcamera.org/comment/21409/","msgid":"","date":"2021-11-30T05:25:19","subject":"Re: [libcamera-devel] [PATCH v4 16/22] libcamera: v4l2_videodevice:\n\tPass SharedFD to open()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Tue, Nov 30, 2021 at 12:39 PM Laurent Pinchart\n wrote:\n>\n> The V4L2VideoDevice::open() function that takes an open file handle\n> duplicates it internally, and leaves the original handle untouched. This\n> is documented but not enforced through language constructs. Fix it by\n> passing a SharedFD to the function.\n\nFileDescriptor.\nditto for the commit title.\n\n>\n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Hirokazu Honda \n\n\n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> src/libcamera/v4l2_videodevice.cpp | 22 ++++++++-----------\n> 2 files changed, 10 insertions(+), 14 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 80b665771782..9f556f99587c 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -184,7 +184,7 @@ public:\n> ~V4L2VideoDevice();\n>\n> int open();\n> - int open(int handle, enum v4l2_buf_type type);\n> + int open(FileDescriptor handle, enum v4l2_buf_type type);\n> void close();\n>\n> const char *driverName() const { return caps_.driver(); }\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 3966483a365f..5f1cc6e2f47f 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -612,16 +612,14 @@ int V4L2VideoDevice::open()\n> * its type from the capabilities. This can be used to force a given device type\n> * for memory-to-memory devices.\n> *\n> - * The file descriptor \\a handle is duplicated, and the caller is responsible\n> - * for closing the \\a handle when it has no further use for it. The close()\n> - * function will close the duplicated file descriptor, leaving \\a handle\n> - * untouched.\n> + * The file descriptor \\a handle is duplicated, no reference to the original\n> + * handle is kept.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> +int V4L2VideoDevice::open(FileDescriptor handle, enum v4l2_buf_type type)\n> {\n> - UniqueFD newFd(dup(handle));\n> + UniqueFD newFd = handle.dup();\n> if (!newFd.isValid()) {\n> LOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> << strerror(errno);\n> @@ -1900,12 +1898,10 @@ int V4L2M2MDevice::open()\n>\n> /*\n> * The output and capture V4L2VideoDevice instances use the same file\n> - * handle for the same device node. The local file handle can be closed\n> - * as the V4L2VideoDevice::open() retains a handle by duplicating the\n> - * fd passed in.\n> + * handle for the same device node.\n> */\n> - UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),\n> - O_RDWR | O_NONBLOCK));\n> + FileDescriptor fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),\n> + O_RDWR | O_NONBLOCK));\n> if (!fd.isValid()) {\n> ret = -errno;\n> LOG(V4L2, Error) << \"Failed to open V4L2 M2M device: \"\n> @@ -1913,11 +1909,11 @@ int V4L2M2MDevice::open()\n> return ret;\n> }\n>\n> - ret = output_->open(fd.get(), V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> + ret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> if (ret)\n> goto err;\n>\n> - ret = capture_->open(fd.get(), V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> + ret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> if (ret)\n> goto err;\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 E5F3FBDB13\n\tfor ;\n\tTue, 30 Nov 2021 05:25:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F1C7605B4;\n\tTue, 30 Nov 2021 06:25:31 +0100 (CET)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79569604FC\n\tfor ;\n\tTue, 30 Nov 2021 06:25:30 +0100 (CET)","by mail-ed1-x531.google.com with SMTP id y13so81604863edd.13\n\tfor ;\n\tMon, 29 Nov 2021 21:25:30 -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=\"ANKhlH7t\"; 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=dWnTHrospzFxWKC4m60AA94xjKataA2XJ/rHFHDA/q4=;\n\tb=ANKhlH7tnhC0FgesqPZkp1x0Lv/K+dgIHt7mfdtCpiQ1olh90dTByeZtXPZiknalmh\n\tp32XKe1fiWb+5+MwCeI2Lfk9ntDS3eXQZqbPdXbKidU6m9B2OWKLQuLL94s9uO2mRVlf\n\tM5xxYwIIiTrMVW4W33ATVw+9VNXcCaip16Qhg=","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=dWnTHrospzFxWKC4m60AA94xjKataA2XJ/rHFHDA/q4=;\n\tb=NwcC7sqfi3LKTGDS4g7hEg29GraI8PrGCNZvhszySzxkJPhoY8Y/Sg6gHcPjd6+7fM\n\tEHXrKxNri072dgW/4R/izky/OVTM2nndtkSE9M7BETQpDSfSz34b19abzPQzH6aLMjIQ\n\tob1LlFch4niTZzp3+c0H6rPi/63P0HGXuPjaNV5ey62YELP+RPGoUA6bYGuNA58cseJM\n\t3slrnlmMGX9wzKG6xR+6VVV1Z/oP7+NXhpop4cCQ8Xp2KKGTCKAtWxXNhDqbJLWshh+p\n\tgUx92+HsQeUMsObXEIqhCnA8r+PaqLryMKhkT1m+wD/27Q7Hbyd7WtRgK+OgJPIiP8nh\n\tVhfg==","X-Gm-Message-State":"AOAM5300habDa2o/54pUZLid2xX3/3iUaB+BYhSLYoEmwG+qoM2uKQc8\n\t46DHET06WqBVJD/3dq2EnP3Yrw3Y9BG5TR/tBBiNvTjAEVA=","X-Google-Smtp-Source":"ABdhPJyF/fFKWzyflFU9Yqb1xl02EB1bwuyo/YPlayFhF34fV9QsqIZVLr4LcVlK+4qWKzOvfYnBywJe0eBL9LzEbl0=","X-Received":"by 2002:a17:906:7688:: with SMTP id\n\to8mr20090919ejm.291.1638249930042; \n\tMon, 29 Nov 2021 21:25:30 -0800 (PST)","MIME-Version":"1.0","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-17-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211130033820.18235-17-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 30 Nov 2021 14:25:19 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 16/22] libcamera: v4l2_videodevice:\n\tPass SharedFD to open()","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":21421,"web_url":"https://patchwork.libcamera.org/comment/21421/","msgid":"<20211130080423.arknv7qjuqslyu2h@uno.localdomain>","date":"2021-11-30T08:04:23","subject":"Re: [libcamera-devel] [PATCH v4 16/22] libcamera: v4l2_videodevice:\n\tPass SharedFD to open()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Nov 30, 2021 at 05:38:14AM +0200, Laurent Pinchart wrote:\n> The V4L2VideoDevice::open() function that takes an open file handle\n> duplicates it internally, and leaves the original handle untouched. This\n> is documented but not enforced through language constructs. Fix it by\n> passing a SharedFD to the function.\n>\n> Signed-off-by: Laurent Pinchart \n\nWith Hiro comments addressed\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> src/libcamera/v4l2_videodevice.cpp | 22 ++++++++-----------\n> 2 files changed, 10 insertions(+), 14 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 80b665771782..9f556f99587c 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -184,7 +184,7 @@ public:\n> \t~V4L2VideoDevice();\n>\n> \tint open();\n> -\tint open(int handle, enum v4l2_buf_type type);\n> +\tint open(FileDescriptor handle, enum v4l2_buf_type type);\n> \tvoid close();\n>\n> \tconst char *driverName() const { return caps_.driver(); }\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 3966483a365f..5f1cc6e2f47f 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -612,16 +612,14 @@ int V4L2VideoDevice::open()\n> * its type from the capabilities. This can be used to force a given device type\n> * for memory-to-memory devices.\n> *\n> - * The file descriptor \\a handle is duplicated, and the caller is responsible\n> - * for closing the \\a handle when it has no further use for it. The close()\n> - * function will close the duplicated file descriptor, leaving \\a handle\n> - * untouched.\n> + * The file descriptor \\a handle is duplicated, no reference to the original\n> + * handle is kept.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> +int V4L2VideoDevice::open(FileDescriptor handle, enum v4l2_buf_type type)\n> {\n> -\tUniqueFD newFd(dup(handle));\n> +\tUniqueFD newFd = handle.dup();\n> \tif (!newFd.isValid()) {\n> \t\tLOG(V4L2, Error) << \"Failed to duplicate file handle: \"\n> \t\t\t\t << strerror(errno);\n> @@ -1900,12 +1898,10 @@ int V4L2M2MDevice::open()\n>\n> \t/*\n> \t * The output and capture V4L2VideoDevice instances use the same file\n> -\t * handle for the same device node. The local file handle can be closed\n> -\t * as the V4L2VideoDevice::open() retains a handle by duplicating the\n> -\t * fd passed in.\n> +\t * handle for the same device node.\n> \t */\n> -\tUniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),\n> -\t\t\t O_RDWR | O_NONBLOCK));\n> +\tFileDescriptor fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),\n> +\t\t\t\t O_RDWR | O_NONBLOCK));\n> \tif (!fd.isValid()) {\n> \t\tret = -errno;\n> \t\tLOG(V4L2, Error) << \"Failed to open V4L2 M2M device: \"\n> @@ -1913,11 +1909,11 @@ int V4L2M2MDevice::open()\n> \t\treturn ret;\n> \t}\n>\n> -\tret = output_->open(fd.get(), V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> +\tret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);\n> \tif (ret)\n> \t\tgoto err;\n>\n> -\tret = capture_->open(fd.get(), V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> +\tret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);\n> \tif (ret)\n> \t\tgoto err;\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 D49E8BF415\n\tfor ;\n\tTue, 30 Nov 2021 08:03:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28DCD605B4;\n\tTue, 30 Nov 2021 09:03:32 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2780F60230\n\tfor ;\n\tTue, 30 Nov 2021 09:03:31 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id A165E40014;\n\tTue, 30 Nov 2021 08:03:30 +0000 (UTC)"],"Date":"Tue, 30 Nov 2021 09:04:23 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211130080423.arknv7qjuqslyu2h@uno.localdomain>","References":"<20211130033820.18235-1-laurent.pinchart@ideasonboard.com>\n\t<20211130033820.18235-17-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211130033820.18235-17-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 16/22] libcamera: v4l2_videodevice:\n\tPass SharedFD to open()","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\" "}}]