[libcamera-devel] [PATCH v2 06/12] libcamera: device_enumerator: add DeviceEnumeratorUdev class

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-12-30 22:41:09 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Saturday, 29 December 2018 05:28:49 EET Niklas Söderlund wrote:
> > Provide a DeviceEnumeratorUdev class which is a specialization
> > of DeviceEnumerator which uses udev to enumerate information in the
> > system.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > * Changes since v1
> > - s/NULL/nullptr/
> > - s/foo == NULL/!foo/
> > - Break log messages to not exceed 80 chars.
> > - Return a std::string from lookupDevnode().
> > ---
> >  src/libcamera/device_enumerator.cpp       | 98 +++++++++++++++++++++++
> >  src/libcamera/include/device_enumerator.h | 15 ++++
> >  2 files changed, 113 insertions(+)
> > 
> > diff --git a/src/libcamera/device_enumerator.cpp
> > b/src/libcamera/device_enumerator.cpp index
> > df9e89a1afeecda1..3cafd0d3703dac99 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -266,4 +266,102 @@ DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm)
> > const return info;
> >  }
> > 
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * Enumerator Udev
> > + */
> > +
> > +DeviceEnumeratorUdev::DeviceEnumeratorUdev()
> > +	: udev_(nullptr)
> > +{
> > +}
> > +
> > +DeviceEnumeratorUdev::~DeviceEnumeratorUdev()
> > +{
> > +	if (udev_)
> > +		udev_unref(udev_);
> > +}
> > +
> > +int DeviceEnumeratorUdev::init()
> > +{
> > +	if (udev_)
> > +		return -EBUSY;
> > +
> > +	udev_ = udev_new();
> > +	if (!udev_)
> > +		return -ENODEV;
> > +
> > +	return 0;
> > +}
> > +
> > +int DeviceEnumeratorUdev::enumerate()
> > +{
> > +	struct udev_enumerate *udev_enum = nullptr;
> > +	struct udev_list_entry *ents, *ent;
> > +	int ret;
> > +
> > +	udev_enum = udev_enumerate_new(udev_);
> > +	if (!udev_enum)
> > +		return -ENOMEM;
> > +
> > +	ret = udev_enumerate_add_match_subsystem(udev_enum, "media");
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	ret = udev_enumerate_scan_devices(udev_enum);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	ents = udev_enumerate_get_list_entry(udev_enum);
> > +	if (!ents)
> > +		goto done;
> > +
> > +	udev_list_entry_foreach(ent, ents) {
> > +		struct udev_device *dev;
> > +		const char *devnode;
> > +		const char *syspath = udev_list_entry_get_name(ent);
> > +
> > +		dev = udev_device_new_from_syspath(udev_, syspath);
> > +		if (!dev) {
> > +			LOG(Error) << "Failed to get device for '" <<
> > +				   syspath << "', skipping";
> > +			continue;
> > +		}
> > +
> > +		devnode = udev_device_get_devnode(dev);
> > +		if (!devnode) {
> > +			udev_device_unref(dev);
> > +			ret = -ENODEV;
> > +			goto done;
> > +		}
> > +
> > +		addDevice(devnode);
> > +
> > +		udev_device_unref(dev);
> > +	}
> > +done:
> > +	udev_enumerate_unref(udev_enum);
> > +	return ret >= 0 ? 0 : ret;
> > +}
> > +
> > +std::string DeviceEnumeratorUdev::lookupDevnode(int major, int minor)
> > +{
> > +	struct udev_device *device;
> > +	const char *name;
> > +	dev_t devnum;
> > +	std::string devnode("");
> 
> Wouldn't std::string devnode(); be a little more efficient as it won't need to 
> parse the const char * ?

std::string devnode();

Would be considered a function declaration and is not valid, confusing I 
know. One option is

std::string devnode = std::string()

This would create a copy, but IIRC the compiler is allowed to optimize 
away the copy. In any case it's better so I'm going with this for now.

> 
> > +
> > +	devnum = makedev(major, minor);
> > +	device = udev_device_new_from_devnum(udev_, 'c', devnum);
> > +	if (!device)
> > +		return "";
> 
> Same here, return std::string() ?

Here it make sens, using this.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks!

> 
> > +
> > +	name = udev_device_get_devnode(device);
> > +	if (name)
> > +		devnode = name;
> > +
> > +	udev_device_unref(device);
> > +
> > +	return devnode;
> > +}
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/device_enumerator.h
> > b/src/libcamera/include/device_enumerator.h index
> > 1f8cef3311012308..5348e6cf583dbd15 100644
> > --- a/src/libcamera/include/device_enumerator.h
> > +++ b/src/libcamera/include/device_enumerator.h
> > @@ -75,6 +75,21 @@ private:
> >  	virtual std::string lookupDevnode(int major, int minor) = 0;
> >  };
> > 
> > +class DeviceEnumeratorUdev: public DeviceEnumerator
> > +{
> > +public:
> > +	DeviceEnumeratorUdev();
> > +	~DeviceEnumeratorUdev();
> > +
> > +	int init() final;
> > +	int enumerate() final;
> > +
> > +private:
> > +	struct udev *udev_;
> > +
> > +	std::string lookupDevnode(int major, int minor) final;
> > +};
> > +
> >  } /* namespace libcamera */
> > 
> >  #endif	/* __LIBCAMERA_DEVICE_ENUMERATOR_H__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund