[{"id":11772,"web_url":"https://patchwork.libcamera.org/comment/11772/","msgid":"<20200802180934.GC23801@pendragon.ideasonboard.com>","date":"2020-08-02T18:09:34","subject":"Re: [libcamera-devel] [PATCH v5 1/5] libcamera: v4l2_device: Add\n\tmethod to lookup device path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Jul 29, 2020 at 01:50:51PM +0200, Niklas Söderlund wrote:\n> Add a method to lookup a V4L2 devices path in sysfs.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> * Changes since v3\n> - s/the device path/the device path in sysfs/\n> ---\n> include/libcamera/internal/v4l2_device.h | 1 +\n> src/libcamera/v4l2_device.cpp | 24 ++++++++++++++++++++++++\n> 2 files changed, 25 insertions(+)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index bf643f2ec966bb33..3b605aab343b3b94 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -30,6 +30,7 @@ public:\n> \tint setControls(ControlList *ctrls);\n> \n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> +\tstd::string devicePath() const;\n> \n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 56ea1ddda2c1425f..205797b568a869ae 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -350,6 +351,29 @@ int V4L2Device::setControls(ControlList *ctrls)\n> \treturn ret;\n> }\n> \n> +/**\n> + * \\brief Retrieve the device path in sysfs\n> + *\n> + * The device path in sysfs describes the device backing the V4L2 device.\n\nIs it worth expanding this to mention the physical device ?\n\n * This function returns the sysfs path to the physical device backing the V4L2\n * device. The path is guaranteed to be an absolute path, without any symbolic\n * link.\n\n> + *\n> + * \\todo When switching to C++17 use std::filesystem:: in the implementation.\n> + * \\todo Query udev for this information as the path created for vdev here\n> + * might not be correct.\n\nI was going to mention this :-) Could we give it a go already ? At least\nplumbing this to the DeviceEnumerator, with your implementation being\nthe default. You can implement it in the DeviceEnumerator base class,\nand it can later be moved to the specialized classes. The function\nshould take the device node as an argument.\n\nFor udev, I think it can be as simple as\n\nstd::string DeviceEnumeratorUdev::lookupDevicePath(const std::string &deviceNode)\n{\n\tstruct stat st;\n\tint ret = stat(deviceNode.c_str(), &st);\n\tif (ret < 0) {\n\t\tLOG(DeviceEnumerator, Error)\n\t\t\t<< \"Unable to stat '\" << deviceNode << \"'\";\n\t\treturn {};\n\t}\n\n\tstruct udev_device *dev = udev_device_new_from_devnum(udev_, 'c',\n\t\t\t\t\t\t\t st.st_rdev);\n\tif (!dev) {\n\t\tLOG(DeviceEnumerator, Error)\n\t\t\t<< \"Unable to make device from devnum \" << st.st_rdev;\n\t\treturn {};\n\t}\n\n\tstd::string path{ udev_device_get_syspath(dev) };\n\tudev_device_unref(dev);\n\treturn path;\n}\n\nAnother option would be to retrieve the syspath at enumeration time and\ncache it, it would be less costly at runtime, but that could be\nunnecessary optimization.\n\n> + *\n> + * \\return The device path in sysfs\n> + */\n> +std::string V4L2Device::devicePath() const\n> +{\n> +\tstd::string vdev = basename(deviceNode_.c_str());\n> +\tstd::string sysfs = \"/sys/class/video4linux/\" + vdev + \"/device\";\n> +\n> +\tchar path[PATH_MAX];\n> +\tif (!realpath(sysfs.c_str(), path))\n\nAccording to its manpage, realpath() also requires stdlib.h.\n\nQuoting the manpage,\n\nThe POSIX.1-2001 standard version of this function is broken by design,\nsince it is impossible to determine a suitable size for the output\nbuffer, resolved_path. According to POSIX.1-2001 a buffer of size\nPATH_MAX suffices, but PATH_MAX need not be a defined constant, and may\nhave to be obtained using pathconf(3). And asking pathconf(3) does not\nreally help, since, on the one hand POSIX warns that the result of\npathconf(3) may be huge and unsuitable for mallocing memory, and on the\nother hand pathconf(3) may return -1 to signify that PATH_MAX is not\nbounded. The resolved_path == NULL feature, not standardized in\nPOSIX.1-2001, but standardized in POSIX.1-2008, allows this design\nproblem to be avoided.\n\nShould we thus do\n\n\tchar *realPath = realpath(sysfs.c_str(), nullptr);\n\tif (!realPath) {\n\t\tLOG(V4L2, Fatal) << \"Can not resolve path for \" << deviceNode_;\n\t\treturn {};\n\t}\n\n\tstd::string path{ realPath };\n\tfree(realPath);\n\treturn path;\n\n> +\t\tLOG(V4L2, Fatal) << \"Can not resolve path for \" << deviceNode_;\n> +\n> +\treturn path;\n> +}\n> +\n> /**\n> * \\brief Perform an IOCTL system call on the device node\n> * \\param[in] request The IOCTL request code","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 6B9E3BD87A\n\tfor ;\n\tSun, 2 Aug 2020 18:09:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CC4860396;\n\tSun, 2 Aug 2020 20:09:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DCC060393\n\tfor ;\n\tSun, 2 Aug 2020 20:09:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC14F296;\n\tSun, 2 Aug 2020 20:09:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WXAHWg1f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596391785;\n\tbh=r0jOs7vDIf+zNzYWTnsh0eTmbWzIke2Lw6OaLgVT9Ks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WXAHWg1fsl8MIpnk5oFQmtoZJcvTTGPYcSj3NUu3ST7OhNklQuDrQc+JYvYg6P3VA\n\tEBvqcMuG7F42VAjD9iY2zsXobNcmrww5pHa0TL9DOHtPdhzK1e1ipPOtEeiV+IPA4u\n\t5puy8Wwi2bEXMJqtpuRNnfryPt3hbXj6ikNcgiJ4=","Date":"Sun, 2 Aug 2020 21:09:34 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200802180934.GC23801@pendragon.ideasonboard.com>","References":"<20200729115055.3840110-1-niklas.soderlund@ragnatech.se>\n\t<20200729115055.3840110-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729115055.3840110-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v5 1/5] libcamera: v4l2_device: Add\n\tmethod to lookup device path","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]