[libcamera-devel,v5,1/5] libcamera: v4l2_device: Add method to lookup device path

9072 diff mbox series

Add a method to lookup a V4L2 devices path in sysfs.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
* Changes since v3
- s/the device path/the device path in sysfs/
---
 include/libcamera/internal/v4l2_device.h |  1 +
 src/libcamera/v4l2_device.cpp            | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Hi Niklas,

Thank you for the patch.

On Wed, Jul 29, 2020 at 01:50:51PM +0200, Niklas Söderlund wrote:
> Add a method to lookup a V4L2 devices path in sysfs.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> * Changes since v3
> - s/the device path/the device path in sysfs/
> ---
>  include/libcamera/internal/v4l2_device.h |  1 +
>  src/libcamera/v4l2_device.cpp            | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index bf643f2ec966bb33..3b605aab343b3b94 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -30,6 +30,7 @@ public:
>  	int setControls(ControlList *ctrls);
>  
>  	const std::string &deviceNode() const { return deviceNode_; }
> +	std::string devicePath() const;
>  
>  protected:
>  	V4L2Device(const std::string &deviceNode);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 56ea1ddda2c1425f..205797b568a869ae 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <fcntl.h>
>  #include <iomanip>
> +#include <limits.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <sys/syscall.h>
> @@ -350,6 +351,29 @@ int V4L2Device::setControls(ControlList *ctrls)
>  	return ret;
>  }
>  
> +/**
> + * \brief Retrieve the device path in sysfs
> + *
> + * The device path in sysfs describes the device backing the V4L2 device.

Is it worth expanding this to mention the physical device ?

 * This function returns the sysfs path to the physical device backing the V4L2
 * device. The path is guaranteed to be an absolute path, without any symbolic
 * link.

> + *
> + * \todo When switching to C++17 use std::filesystem:: in the implementation.
> + * \todo Query udev for this information as the path created for vdev here
> + * might not be correct.

I was going to mention this :-) Could we give it a go already ? At least
plumbing this to the DeviceEnumerator, with your implementation being
the default. You can implement it in the DeviceEnumerator base class,
and it can later be moved to the specialized classes. The function
should take the device node as an argument.

For udev, I think it can be as simple as

std::string DeviceEnumeratorUdev::lookupDevicePath(const std::string &deviceNode)
{
	struct stat st;
	int ret = stat(deviceNode.c_str(), &st);
	if (ret < 0) {
		LOG(DeviceEnumerator, Error)
			<< "Unable to stat '" << deviceNode << "'";
		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) };
	udev_device_unref(dev);
	return path;
}

Another option would be to retrieve the syspath at enumeration time and
cache it, it would be less costly at runtime, but that could be
unnecessary optimization.

> + *
> + * \return The device path in sysfs
> + */
> +std::string V4L2Device::devicePath() const
> +{
> +	std::string vdev = basename(deviceNode_.c_str());
> +	std::string sysfs = "/sys/class/video4linux/" + vdev + "/device";
> +
> +	char path[PATH_MAX];
> +	if (!realpath(sysfs.c_str(), path))

According to its manpage, realpath() also requires stdlib.h.

Quoting the manpage,

The POSIX.1-2001 standard version of this function is broken by design,
since it is impossible to determine a suitable size for the output
buffer, resolved_path.  According to POSIX.1-2001 a buffer of size
PATH_MAX suffices, but PATH_MAX need not be a defined constant, and may
have to be obtained using pathconf(3).  And asking pathconf(3) does not
really  help, since, on the one hand POSIX warns that the result of
pathconf(3) may be huge and unsuitable for mallocing memory, and on the
other hand pathconf(3) may return -1 to signify that PATH_MAX is not
bounded.  The resolved_path == NULL feature, not standardized in
POSIX.1-2001, but standardized in POSIX.1-2008, allows this design
problem to be avoided.

Should we thus do

	char *realPath = realpath(sysfs.c_str(), nullptr);
	if (!realPath) {
		LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_;
		return {};
	}

	std::string path{ realPath };
	free(realPath);
	return path;

> +		LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_;
> +
> +	return path;
> +}
> +
>  /**
>   * \brief Perform an IOCTL system call on the device node
>   * \param[in] request The IOCTL request code