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

[Laurent Pinchart]

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 * ?

> +
> +	devnum = makedev(major, minor);
> +	device = udev_device_new_from_devnum(udev_, 'c', devnum);
> +	if (!device)
> +		return "";

Same here, return std::string() ?

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

> +
> +	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