[{"id":11440,"web_url":"https://patchwork.libcamera.org/comment/11440/","msgid":"<20200720131253.lgalofgpgijl4lmm@uno.localdomain>","date":"2020-07-20T13:12:53","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: v4l2_device: Add\n\tmethod to lookup device path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sat, Jul 18, 2020 at 03:23:16PM +0200, Niklas Söderlund wrote:\n> Add a method to lookup a V4L2 devices path in sysfs. The device path\n> describes the bus and device backing the V4L2 device and is guaranteed\n> to be unique in the system and is persistent between reboots of the\n> system.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/v4l2_device.h | 1 +\n> src/libcamera/v4l2_device.cpp | 27 ++++++++++++++++++++++++\n> 2 files changed, 28 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..2f95e45261463f34 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,32 @@ int V4L2Device::setControls(ControlList *ctrls)\n> \treturn ret;\n> }\n>\n> +/**\n> + * \\brief Retrieve the device path\n\nunique device path in sysfs\n\n> + *\n> + * The device path describes the bus and device backing the V4L2 device and is\n> + * guaranteed to be unique in the system and is persistent between reboots of\n> + * the system.\n\ndrop the last 'the system' as it is repeated.\n\n> + *\n> + * The path is located by utilising that every V4L2 device have an entry under\n> + * /sys/class/video4linux/ that contains a symlink the device backing it.\n\nI'm not sure if it is required to state this, if that's part of the V4L2\nspecification. I would however change 'utilising' with 'assuming', but\nmaybe it's me not knowing how that word should be used.\n\n> + *\n> + * \\todo When switching to C++17 use std::filesystem:: in the implementation.\n> + *\n> + * \\return The device path\n\nThe unique device path in sysfs\n\nDo we want to use 'predictable' as systemd does with network interface\nnames ?\n\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> +\t\tLOG(V4L2, Fatal) << \"Can not resolve path for \" << deviceNode_;\n\nI would cache the error and report why we failed (ie path non existing\nor permission issues).\n\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\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 A2A02C0109\n\tfor ;\n\tMon, 20 Jul 2020 13:09:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A322605B7;\n\tMon, 20 Jul 2020 15:09:22 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A5A66039F\n\tfor ;\n\tMon, 20 Jul 2020 15:09:20 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id B29281C0017;\n\tMon, 20 Jul 2020 13:09:19 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 20 Jul 2020 15:12:53 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200720131253.lgalofgpgijl4lmm@uno.localdomain>","References":"<20200718132324.867815-1-niklas.soderlund@ragnatech.se>\n\t<20200718132324.867815-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200718132324.867815-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 1/9] 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\" "}},{"id":11590,"web_url":"https://patchwork.libcamera.org/comment/11590/","msgid":"<20200724233934.GK5921@pendragon.ideasonboard.com>","date":"2020-07-24T23:39:34","subject":"Re: [libcamera-devel] [PATCH 1/9] 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 Mon, Jul 20, 2020 at 03:12:53PM +0200, Jacopo Mondi wrote:\n> On Sat, Jul 18, 2020 at 03:23:16PM +0200, Niklas Söderlund wrote:\n> > Add a method to lookup a V4L2 devices path in sysfs. The device path\n> > describes the bus and device backing the V4L2 device and is guaranteed\n> > to be unique in the system and is persistent between reboots of the\n> > system.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 1 +\n> > src/libcamera/v4l2_device.cpp | 27 ++++++++++++++++++++++++\n> > 2 files changed, 28 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..2f95e45261463f34 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,32 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > \treturn ret;\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve the device path\n> \n> unique device path in sysfs\n> \n> > + *\n> > + * The device path describes the bus and device backing the V4L2 device and is\n> > + * guaranteed to be unique in the system and is persistent between reboots of\n> > + * the system.\n> \n> drop the last 'the system' as it is repeated.\n> \n> > + *\n> > + * The path is located by utilising that every V4L2 device have an entry under\n> > + * /sys/class/video4linux/ that contains a symlink the device backing it.\n> \n> I'm not sure if it is required to state this, if that's part of the V4L2\n> specification. I would however change 'utilising' with 'assuming', but\n> maybe it's me not knowing how that word should be used.\n\nI would indeed drop this paragraph, or move it to the commit message, or\nto a normal comment in the function.\n\n> > + *\n> > + * \\todo When switching to C++17 use std::filesystem:: in the implementation.\n> > + *\n> > + * \\return The device path\n> \n> The unique device path in sysfs\n> \n> Do we want to use 'predictable' as systemd does with network interface\n> names ?\n> \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\nI'm not sure this can always be guaranteed to be valid. It's entirely\npossible to set udev rules that will rename the device node. One option\nto prepare for that would be to query the device path from the device\nenumerator. For the udev enumerator that would be formward to udev\n(which should know about all these details), for the sysfs enumerator\nthe above code would be used, as a fallback.\n\n> > +\n> > +\tchar path[PATH_MAX];\n> > +\tif (!realpath(sysfs.c_str(), path))\n> > +\t\tLOG(V4L2, Fatal) << \"Can not resolve path for \" << deviceNode_;\n> \n> I would cache the error and report why we failed (ie path non existing\n> or permission issues).\n> \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 4BD67BD878\n\tfor ;\n\tFri, 24 Jul 2020 23:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D718C61223;\n\tSat, 25 Jul 2020 01:39:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D867260496\n\tfor ;\n\tSat, 25 Jul 2020 01:39:41 +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 4BA40538;\n\tSat, 25 Jul 2020 01:39:41 +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=\"qcdN2Ep8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595633981;\n\tbh=UNitbK7pXEo85joMUl1KCkl4lIjzxXma8Mv64E5hN3c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qcdN2Ep8ApZeyigXHgJUzz8N0iKM8E5q5RLkxugcBGxdPgr0simLNbBU/3D6VuJLU\n\tDZOL5kYK23Gtnih4uBBNCg+B/ErXbV0dyKd2ZWIE/Ln6BAPGdaDUBejz7/pjug0fvJ\n\th5HsHkqvX0Audor2X5dsLbEdIjxXCxATjric+3eU=","Date":"Sat, 25 Jul 2020 02:39:34 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200724233934.GK5921@pendragon.ideasonboard.com>","References":"<20200718132324.867815-1-niklas.soderlund@ragnatech.se>\n\t<20200718132324.867815-2-niklas.soderlund@ragnatech.se>\n\t<20200720131253.lgalofgpgijl4lmm@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200720131253.lgalofgpgijl4lmm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/9] 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\" "}}]