[libcamera-devel] [PATCH v6 2/9] libcamera: device_enumerator_udev: Add specialization for sysfs path

[Jacopo Mondi]

Hi Niklas,

On Mon, Aug 03, 2020 at 11:17:26PM +0200, Niklas Söderlund wrote:
> As udev may rename the V4L2 devices in /dev add a specialization that
> asks udev for the sysfs path instead of using the default method that
> makes the assumption that V4L2 devices are never renamed.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  .../internal/device_enumerator_udev.h         |  2 ++
>  src/libcamera/device_enumerator_udev.cpp      | 24 +++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h
> index 6f45be0c1c423d02..595b16acb89d98bb 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -35,6 +35,8 @@ public:
>  	int init();
>  	int enumerate();
>
> +	std::string lookupSysfsPath(const std::string &deviceNode);

override

> +
>  private:
>  	using DependencyMap = std::map<dev_t, std::list<MediaEntity *>>;
>
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 96689daa5dd113dc..2e43b84f62e0333d 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -14,6 +14,7 @@
>  #include <map>
>  #include <string.h>
>  #include <sys/ioctl.h>
> +#include <sys/stat.h>
>  #include <sys/sysmacros.h>
>  #include <unistd.h>
>
> @@ -193,6 +194,29 @@ done:
>  	return 0;
>  }
>
> +std::string DeviceEnumeratorUdev::lookupSysfsPath(const std::string &deviceNode)

I might be confused, but if we construct the udev device from
'/dev/video0' to avoid assuming '/dev/video0' is stable, what do we
gain ?

> +{
> +	struct stat st;
> +	int ret = stat(deviceNode.c_str(), &st);
> +	if (ret < 0) {
> +		LOG(DeviceEnumerator, Error)
> +			<< "Unable to stat '" << deviceNode << "'";

I think printing out why stat failed has value.

                        << strerror(ret);

> +		return {};
> +	}
> +
> +	struct udev_device *dev = udev_device_new_from_devnum(udev_, 'c',
> +							      st.st_rdev);
> +	if (!dev) {
> +		LOG(DeviceEnumerator, Error)
> +			<< "Unable to make device from devnum " << st.st_rdev;
> +		return {};
> +	}
> +
> +	std::string path{ udev_device_get_syspath(dev) };

I'm curious, why do you use aggregate initialization for strings
instead of the constructor ?

> +	udev_device_unref(dev);
> +	return path;
> +}
> +
>  int DeviceEnumeratorUdev::populateMediaDevice(MediaDevice *media, DependencyMap *deps)
>  {
>  	std::set<dev_t> children;
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel