[libcamera-devel] [PATCH 1/9] libcamera: v4l2_device: Add method to lookup device path

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Jul 20, 2020 at 03:12:53PM +0200, Jacopo Mondi wrote:
> On Sat, Jul 18, 2020 at 03:23:16PM +0200, Niklas Söderlund wrote:
> > Add a method to lookup a V4L2 devices path in sysfs. The device path
> > describes the bus and device backing the V4L2 device and is guaranteed
> > to be unique in the system and is persistent between reboots of the
> > system.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  1 +
> >  src/libcamera/v4l2_device.cpp            | 27 ++++++++++++++++++++++++
> >  2 files changed, 28 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..2f95e45261463f34 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,32 @@ int V4L2Device::setControls(ControlList *ctrls)
> >  	return ret;
> >  }
> >
> > +/**
> > + * \brief Retrieve the device path
> 
> unique device path in sysfs
> 
> > + *
> > + * The device path describes the bus and device backing the V4L2 device and is
> > + * guaranteed to be unique in the system and is persistent between reboots of
> > + * the system.
> 
> drop the last 'the system' as it is repeated.
> 
> > + *
> > + * The path is located by utilising that every V4L2 device have an entry under
> > + * /sys/class/video4linux/ that contains a symlink the device backing it.
> 
> I'm not sure if it is required to state this, if that's part of the V4L2
> specification. I would however change 'utilising' with 'assuming', but
> maybe it's me not knowing how that word should be used.

I would indeed drop this paragraph, or move it to the commit message, or
to a normal comment in the function.

> > + *
> > + * \todo When switching to C++17 use std::filesystem:: in the implementation.
> > + *
> > + * \return The device path
> 
> The unique device path in sysfs
> 
> Do we want to use 'predictable' as systemd does with network interface
> names ?
> 
> > + */
> > +std::string V4L2Device::devicePath() const
> > +{
> > +	std::string vdev = basename(deviceNode_.c_str());
> > +	std::string sysfs = "/sys/class/video4linux/" + vdev + "/device";

I'm not sure this can always be guaranteed to be valid. It's entirely
possible to set udev rules that will rename the device node. One option
to prepare for that would be to query the device path from the device
enumerator. For the udev enumerator that would be formward to udev
(which should know about all these details), for the sysfs enumerator
the above code would be used, as a fallback.

> > +
> > +	char path[PATH_MAX];
> > +	if (!realpath(sysfs.c_str(), path))
> > +		LOG(V4L2, Fatal) << "Can not resolve path for " << deviceNode_;
> 
> I would cache the error and report why we failed (ie path non existing
> or permission issues).
> 
> > +
> > +	return path;
> > +}
> > +
> >  /**
> >   * \brief Perform an IOCTL system call on the device node
> >   * \param[in] request The IOCTL request code

-- 
Regards,

Laurent Pinchart