[{"id":1550,"web_url":"https://patchwork.libcamera.org/comment/1550/","msgid":"<20190502225636.GC1593@pendragon.ideasonboard.com>","date":"2019-05-02T22:56:36","subject":"Re: [libcamera-devel] [PATCH] libcamera: device_enumerator: add\n\tDeviceEnumeratorSysfs class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, May 02, 2019 at 05:26:51PM -0400, Paul Elder wrote:\n> A udev-based device enumerator is not sufficient, since libudev is an\n> optional dependency, or udev might fail. In these cases, we should fall\n> back to using sysfs to enumerate devices.\n> \n> Add a DeviceEnumeratorSysfs class which is a specialization of\n> DeviceEnumerator that uses sysfs to enumerate media devices on the\n> system.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Documentation/Doxyfile.in | 2 +\n> src/libcamera/device_enumerator.cpp | 6 +-\n> src/libcamera/device_enumerator_sysfs.cpp | 109 ++++++++++++++++++\n> .../include/device_enumerator_sysfs.h | 31 +++++\n> src/libcamera/meson.build | 2 +\n> 5 files changed, 148 insertions(+), 2 deletions(-)\n> create mode 100644 src/libcamera/device_enumerator_sysfs.cpp\n> create mode 100644 src/libcamera/include/device_enumerator_sysfs.h\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 950ad4f..5dd4445 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -835,6 +835,8 @@ RECURSIVE = YES\n> \n> EXCLUDE = ../src/libcamera/device_enumerator_udev.cpp \\\n> \t\t\t ../src/libcamera/include/device_enumerator_udev.h \\\n> +\t\t\t ../src/libcamera/device_enumerator_sysfs.cpp \\\n> +\t\t\t ../src/libcamera/include/device_enumerator_sysfs.h \\\n> \t\t\t ../src/libcamera/pipeline/\n\nCould you please keep those entries ordered alphabetically ?\n\n> \n> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or\n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index f6878b3..5c44eb1 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"device_enumerator.h\"\n> #include \"device_enumerator_udev.h\"\n> +#include \"device_enumerator_sysfs.h\"\n\nHere too :-)\n\n> \n> #include \n> \n> @@ -153,8 +154,9 @@ std::unique_ptr DeviceEnumerator::create()\n> \t * Either udev is not available or udev initialization failed. Fall back\n> \t * on the sysfs enumerator.\n> \t */\n> -\n> -\t/** \\todo Add a sysfs-based enumerator. */\n> +\tenumerator = utils::make_unique();\n> +\tif (!enumerator->init())\n> +\t\treturn enumerator;\n> \n> \treturn nullptr;\n> }\n> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp\n> new file mode 100644\n> index 0000000..6dd04e8\n> --- /dev/null\n> +++ b/src/libcamera/device_enumerator_sysfs.cpp\n> @@ -0,0 +1,109 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018-2019, Google Inc.\n\nAs this is a new file, 2019 is sufficient.\n\n> + *\n> + * device_enumerator_sysfs.cpp - udev-based device enumerator\n\nsysfs/based ?\n\n> + */\n> +\n> +#include \"device_enumerator_sysfs.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(DeviceEnumerator)\n> +\n> +DeviceEnumeratorSysfs::DeviceEnumeratorSysfs()\n> +{\n> +}\n> +\n> +DeviceEnumeratorSysfs::~DeviceEnumeratorSysfs()\n> +{\n> +}\n> +\n\nUnless I'm mistaken you don't need to define empty constructors and\ndestructors (except for the destructor at the base of the hierarchy of a\nvirtual class), the compiler will handle this for you.\n\n> +int DeviceEnumeratorSysfs::init()\n> +{\n> +\treturn 0;\n> +}\n> +\n> +int DeviceEnumeratorSysfs::enumerate()\n> +{\n> +\tstruct dirent *ent;\n> +\tstruct stat devstat;\n> +\tchar *end;\n\nYou can declare devstat and end within the loop.\n\n> +\tDIR *dir;\n> +\n> +\tstatic const char *sysfs_dirs[] = {\n> +\t\t\"/sys/media/devices\",\n> +\t\t\"/sys/block/media/devices\",\n> +\t\t\"/sys/bus/media/devices\",\n> +\t\t\"/sys/class/media/devices\",\n\nThe first and second entries look really suspicious. Do you need them ?\nDo you need the last entry ?\n\n> +\t};\n> +\n> +\tfor (const char *dirname : sysfs_dirs) {\n> +\t\tdir = opendir(dirname);\n> +\t\tif (dir)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\twhile ((ent = readdir(dir)) != nullptr) {\n> +\t\tunsigned int idx;\n> +\t\tstd::string devnode = std::string();\n\nNo need for initialisers, the variable will be constructed with the\ndefault constructor of std::string in any case.\n\n> +\n> +\t\tif (strncmp(ent->d_name, \"media\", 5))\n> +\t\t\tcontinue;\n> +\n> +\t\tidx = strtoul(ent->d_name + 5, &end, 10);\n> +\t\tif (*end != '\\0')\n> +\t\t\tcontinue;\n> +\n> +\t\tdevnode = \"/dev/media\" + std::to_string(idx);\n> +\n> +\t\t/* Verify that the device node exists. */\n> +\t\tif (stat(devnode.c_str(), &devstat) < 0)\n> +\t\t\tcontinue;\n\nHow about logging a warning message here ?\n\n> +\n> +\t\taddDevice(devnode);\n> +\t}\n> +\n> +\tclosedir(dir);\n> +\n> +\treturn 0;\n> +}\n> +\n> +std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor)\n> +{\n> +\tstd::string deviceNode = std::string();\n> +\tstd::string line = std::string();\n\nNo need for initialisers here either.\n\n> +\tstd::ifstream uevent_file;\n\nWe use camelCase for variables, this should be named ueventFile.\n\n> +\tsize_t c_cnt;\n\nAnd this could just be named count.\n\n> +\n> +\tuevent_file.open(\"/sys/dev/char/\" + std::to_string(major) + \":\" +\n> +\t\t\t std::to_string(minor) + \"/uevent\");\n> +\tif (!uevent_file)\n> +\t\treturn std::string();\n> +\n> +\twhile (uevent_file >> line) {\n> +\t\tif (line.find(\"DEVNAME\") != std::string::npos) {\n> +\t\t\tc_cnt = line.find(\"=\");\n\nI think we should require the line to start with \"DEVNAME=\" exactly.\nMaybe\n\n\t\tif (line.find(\"DEVNAME=\") == 0) {\n\t\t\tdeviceNode = \"dev\" + line.substr(strlen(\"DEVNAME=\"));\n\t\t\tbreak;\n\t\t}\n\n> +\t\t\tdeviceNode = \"/dev/\" + line.substr(c_cnt + 1);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tuevent_file.close();\n> +\n> +\treturn deviceNode;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h\n> new file mode 100644\n> index 0000000..f32d6ae\n> --- /dev/null\n> +++ b/src/libcamera/include/device_enumerator_sysfs.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018-2019, Google Inc.\n\nJust 2019 here too.\n\n> + *\n> + * device_enumerator_sysfs.h - sysfs-based device enumerator\n> + */\n> +#ifndef __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__\n> +#define __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__\n> +\n> +#include \n> +\n> +#include \"device_enumerator.h\"\n> +\n> +namespace libcamera {\n> +\n> +class DeviceEnumeratorSysfs : public DeviceEnumerator\n> +{\n> +public:\n> +\tDeviceEnumeratorSysfs();\n> +\t~DeviceEnumeratorSysfs();\n> +\n> +\tint init() final;\n> +\tint enumerate() final;\n\nYou can declare the whole class final instead of individual members with\n\nclass DeviceEnumeratorSysfs final : public DeviceEnumerator\n\n> +\n> +private:\n> +\tstd::string lookupDeviceNode(int major, int minor) final;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 2b67823..ca9ed7a 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -4,6 +4,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'device_enumerator.cpp',\n> + 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n> 'event_dispatcher_poll.cpp',\n> 'event_notifier.cpp',\n> @@ -27,6 +28,7 @@ libcamera_headers = files([\n> 'include/camera_sensor.h',\n> 'include/device_enumerator.h',\n> 'include/device_enumerator_udev.h',\n> + 'include/device_enumerator_sysfs.h',\n\nAlphabetical order here too please.\n\n> 'include/event_dispatcher_poll.h',\n> 'include/formats.h',\n> 'include/log.h',","headers":{"Return-Path":"","Received":["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 2764660E5F\n\tfor ;\n\tFri, 3 May 2019 00:56:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B4F22D1;\n\tFri, 3 May 2019 00:56:50 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1556837810;\n\tbh=1+Gknn9Gu/Kj0d4yrUJEbHf3QImQRrm7rGLQ6GLvXug=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qujr7TlYIg0xlshSASrOGAuNEl+GeusXSKGdB1hNbcLtvQX30EpfVf6HDJrulUl3J\n\tJvapfxDKvkfBePdGLmqTugL6kXyWgBLEAlR11X0Xr8SiAU2BumpMko3605Aq2WWKfC\n\tpw8YGtSqodTnj+HfNZdtdP6J4Y7hLGaJ1Njv0BLQ=","Date":"Fri, 3 May 2019 01:56:36 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190502225636.GC1593@pendragon.ideasonboard.com>","References":"<20190502212651.4279-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190502212651.4279-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH] libcamera: device_enumerator: add\n\tDeviceEnumeratorSysfs class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 02 May 2019 22:56:52 -0000"}}]