[{"id":11807,"web_url":"https://patchwork.libcamera.org/comment/11807/","msgid":"<20200804073518.mvpqijumub73gf7e@uno.localdomain>","date":"2020-08-04T07:35:18","subject":"Re: [libcamera-devel] [PATCH v6 2/9] libcamera:\n\tdevice_enumerator_udev: Add specialization for sysfs path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Aug 03, 2020 at 11:17:26PM +0200, Niklas Söderlund wrote:\n> As udev may rename the V4L2 devices in /dev add a specialization that\n> asks udev for the sysfs path instead of using the default method that\n> makes the assumption that V4L2 devices are never renamed.\n>\n> Suggested-by: Laurent Pinchart \n> Signed-off-by: Niklas Söderlund \n> ---\n> .../internal/device_enumerator_udev.h | 2 ++\n> src/libcamera/device_enumerator_udev.cpp | 24 +++++++++++++++++++\n> 2 files changed, 26 insertions(+)\n>\n> diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h\n> index 6f45be0c1c423d02..595b16acb89d98bb 100644\n> --- a/include/libcamera/internal/device_enumerator_udev.h\n> +++ b/include/libcamera/internal/device_enumerator_udev.h\n> @@ -35,6 +35,8 @@ public:\n> \tint init();\n> \tint enumerate();\n>\n> +\tstd::string lookupSysfsPath(const std::string &deviceNode);\n\noverride\n\n> +\n> private:\n> \tusing DependencyMap = std::map>;\n>\n> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> index 96689daa5dd113dc..2e43b84f62e0333d 100644\n> --- a/src/libcamera/device_enumerator_udev.cpp\n> +++ b/src/libcamera/device_enumerator_udev.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -193,6 +194,29 @@ done:\n> \treturn 0;\n> }\n>\n> +std::string DeviceEnumeratorUdev::lookupSysfsPath(const std::string &deviceNode)\n\nI might be confused, but if we construct the udev device from\n'/dev/video0' to avoid assuming '/dev/video0' is stable, what do we\ngain ?\n\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\nI think printing out why stat failed has value.\n\n << strerror(ret);\n\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\nI'm curious, why do you use aggregate initialization for strings\ninstead of the constructor ?\n\n> +\tudev_device_unref(dev);\n> +\treturn path;\n> +}\n> +\n> int DeviceEnumeratorUdev::populateMediaDevice(MediaDevice *media, DependencyMap *deps)\n> {\n> \tstd::set children;\n> --\n> 2.28.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 7685DBD87A\n\tfor ;\n\tTue, 4 Aug 2020 07:31:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04B8E61B02;\n\tTue, 4 Aug 2020 09:31:39 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13E78609B9\n\tfor ;\n\tTue, 4 Aug 2020 09:31:38 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 6A4F9200007;\n\tTue, 4 Aug 2020 07:31:37 +0000 (UTC)"],"Date":"Tue, 4 Aug 2020 09:35:18 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804073518.mvpqijumub73gf7e@uno.localdomain>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200803211733.1037019-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v6 2/9] libcamera:\n\tdevice_enumerator_udev: Add specialization for sysfs 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":11822,"web_url":"https://patchwork.libcamera.org/comment/11822/","msgid":"<20200804114019.GB6075@pendragon.ideasonboard.com>","date":"2020-08-04T11:40:19","subject":"Re: [libcamera-devel] [PATCH v6 2/9] libcamera:\n\tdevice_enumerator_udev: Add specialization for sysfs path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Aug 04, 2020 at 09:35:18AM +0200, Jacopo Mondi wrote:\n> On Mon, Aug 03, 2020 at 11:17:26PM +0200, Niklas Söderlund wrote:\n> > As udev may rename the V4L2 devices in /dev add a specialization that\n> > asks udev for the sysfs path instead of using the default method that\n> > makes the assumption that V4L2 devices are never renamed.\n> >\n> > Suggested-by: Laurent Pinchart \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > .../internal/device_enumerator_udev.h | 2 ++\n> > src/libcamera/device_enumerator_udev.cpp | 24 +++++++++++++++++++\n> > 2 files changed, 26 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h\n> > index 6f45be0c1c423d02..595b16acb89d98bb 100644\n> > --- a/include/libcamera/internal/device_enumerator_udev.h\n> > +++ b/include/libcamera/internal/device_enumerator_udev.h\n> > @@ -35,6 +35,8 @@ public:\n> > \tint init();\n> > \tint enumerate();\n> >\n> > +\tstd::string lookupSysfsPath(const std::string &deviceNode);\n> \n> override\n> \n> > +\n> > private:\n> > \tusing DependencyMap = std::map>;\n> >\n> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> > index 96689daa5dd113dc..2e43b84f62e0333d 100644\n> > --- a/src/libcamera/device_enumerator_udev.cpp\n> > +++ b/src/libcamera/device_enumerator_udev.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -193,6 +194,29 @@ done:\n> > \treturn 0;\n> > }\n> >\n> > +std::string DeviceEnumeratorUdev::lookupSysfsPath(const std::string &deviceNode)\n> \n> I might be confused, but if we construct the udev device from\n> '/dev/video0' to avoid assuming '/dev/video0' is stable, what do we\n> gain ?\n\nThis will give us the real sysfs path for the device. For instance\n/sys/devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/video4linux/video0.\nThe video0 in that string is the kernel device name, so it's not\ninfluenced by udev rules. This function will return the same string\nregardless of whether deviceNode has been renamed by udev or not. It's\nnot about stable names, it's about finding the correct device regardless\nof udev rules.\n\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> \n> I think printing out why stat failed has value.\n> \n> << strerror(ret);\n\nShould be\n\n\tret = -errno;\n\tLOG(...) << ... << strerror(-ret);\n\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> \n> I'm curious, why do you use aggregate initialization for strings\n> instead of the constructor ?\n\nI think this came from code I've provided. This isn't aggregate\ninitialization but direct list initialization ([1]). Compared to direct\ninitialization with parentheses ([2]), list initialization limits the\nallowed implicit conversions by prohibiting narrowing (e.g. converting a\nfloat to an integer). It's thus preferred in many cases to catch issues\nat compile time. There are however drawbacks, especially when using the\nauto keyword, but also in the fact that different constructors will be\nselected by default. For instance,\n\n\tstd::vector v1(10, 1);\n\nwill create a vector of 10 elements initialized to 1, while\n\n\tstd::vector v2{10, 1};\n\nwill create a vector of two elements, 10 and 1.\n\n[1] https://en.cppreference.com/w/cpp/language/list_initialization\n[2] https://en.cppreference.com/w/cpp/language/direct_initialization\n\n> > +\tudev_device_unref(dev);\n> > +\treturn path;\n> > +}\n> > +\n> > int DeviceEnumeratorUdev::populateMediaDevice(MediaDevice *media, DependencyMap *deps)\n> > {\n> > \tstd::set children;","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 8381FBD87A\n\tfor ;\n\tTue, 4 Aug 2020 11:40:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E784603D9;\n\tTue, 4 Aug 2020 13:40:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F965603C9\n\tfor ;\n\tTue, 4 Aug 2020 13:40:37 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-79-50-nat.elisa-mobile.fi\n\t[85.76.79.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A41D543;\n\tTue, 4 Aug 2020 13:40:35 +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=\"KCWR9GEV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596541236;\n\tbh=cs4bkN/5OcoWwXt18D6s8hwqKoyKz3a8RrLD4NUl3eE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KCWR9GEV4bKFUq4Rphla5KDlhSCN3tYrl6Hf0KJMJejaX6HUDNLOrBdXDrAKM5xcm\n\ttOx+hbf/tD238HjaSK3PAsue+oC0BiVZn6u/ylbJTPGeinMVoTaaLqVS24FxWGhIL5\n\t7CB6L+omg3UoitGdDJMrtOUYpAMW0hR2jmL1rxXU=","Date":"Tue, 4 Aug 2020 14:40:19 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200804114019.GB6075@pendragon.ideasonboard.com>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-3-niklas.soderlund@ragnatech.se>\n\t<20200804073518.mvpqijumub73gf7e@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804073518.mvpqijumub73gf7e@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 2/9] libcamera:\n\tdevice_enumerator_udev: Add specialization for sysfs 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\" "}}]