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

[Jacopo Mondi]

Hi Niklas,

On Sun, Dec 23, 2018 at 12:00:35AM +0100, 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>
> ---
>  src/libcamera/deviceenumerator.cpp       | 100 +++++++++++++++++++++++
>  src/libcamera/include/deviceenumerator.h |  15 ++++
>  2 files changed, 115 insertions(+)
>
> diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp
> index b2f59017e94d14aa..f4c40bf0376ab453 100644
> --- a/src/libcamera/deviceenumerator.cpp
> +++ b/src/libcamera/deviceenumerator.cpp
> @@ -6,6 +6,7 @@
>   */
>
>  #include <fcntl.h>
> +#include <libudev.h>
>  #include <sys/ioctl.h>
>  #include <unistd.h>
>
> @@ -273,4 +274,103 @@ DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const
>  	return info;
>  }
>
> +/* -----------------------------------------------------------------------------
> + * Enumerator Udev
> + */
> +
> +DeviceEnumeratorUdev::DeviceEnumeratorUdev()
> +	: udev_(NULL)
> +{
> +}

Same question as other patches. Empty constructors with only memeber
initializer list might go in the header file?

> +
> +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 = NULL;
> +	struct udev_list_entry *ents, *ent;
> +	int ret;
> +
> +	udev_enum = udev_enumerate_new(udev_);
> +	if (udev_enum == NULL)
> +		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 == NULL)
> +		goto done;
> +
> +	udev_list_entry_foreach(ent, ents) {
> +		struct udev_device *dev;
> +		const char *devnode;
> +		const char *syspath = udev_list_entry_get_name(ent);

nit: you've sorted variables in reverse-xmas-tree order in this file,
could you re-sort here to keep them consistent?

> +
> +		dev = udev_device_new_from_syspath(udev_, syspath);
> +		if (dev == NULL) {

nullptr, or better !dev

> +			LOG(Error) << "Failed to device for '" << syspath << "', skipping";

break to stay in 80 cols. We can span to 120 if it makes code more
clear, but here it is not necessary

			LOG(Error) << "Failed to device for '"
                                   << syspath << "', skipping";

Also "Failed to device": seems like you've missed the verb :)

> +			continue;
> +		}
> +
> +		devnode = udev_device_get_devnode(dev);
> +		if (devnode == NULL) {

nullptr, or better !dev

> +			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;
> +}
> +
> +int DeviceEnumeratorUdev::lookupDevnode(std::string &devnode, int major, int minor)
> +{
> +	struct udev_device *device;
> +	const char *name;
> +	dev_t devnum;

This might be discussed as well, and I pointed it out in review's of
Kieran's series as well. I probably like more declaring variables at the
beginning of the function, but it's common practice to declare them
at initialization if possible.

What do you think? Have a look at my series, where I tried to do that
consistently to make yourself an idea of how it would look like.

> +	int ret = 0;
> +
> +	devnum = makedev(major, minor);
> +	device = udev_device_new_from_devnum(udev_, 'c', devnum);
> +	if (!device)
> +		return -ENODEV;
> +
> +	name = udev_device_get_devnode(device);
> +	if (name)
> +		devnode = name;
> +	else
> +		ret = -ENODEV;
> +
> +	udev_device_unref(device);
> +
> +	return ret;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h
> index cbe17774edb7fcc5..2c7ff3f336ba127d 100644
> --- a/src/libcamera/include/deviceenumerator.h
> +++ b/src/libcamera/include/deviceenumerator.h
> @@ -78,6 +78,21 @@ private:
>  	virtual int lookupDevnode(std::string &devnode, int major, int minor) = 0;
>  };
>
> +class DeviceEnumeratorUdev: public DeviceEnumerator
> +{
> +public:
> +	DeviceEnumeratorUdev();
> +	~DeviceEnumeratorUdev();
> +
> +	int init() final;
> +	int enumerate() final;
> +
> +private:
> +	struct udev *udev_;
> +
> +	int lookupDevnode(std::string &devnode, int major, int minor) final;

This is what I was referring to in my comment to [00/12].
Here it's easy to have udev enumerate entities, and in the same class
use udev to lookup for devnode paths.

If we go for the MediaDevice doing the entities enumeration, we need a
way to provide a method for doing devnode lookup using different
backend.

> +};

The use of final applied to all methods declared virtual in the base
class makes me wonder if the whole class shouldn't be declared final.
But at the same time I wonder if we're sure this will be final for
real :)

If not or the design thing, all minors comment :)

Thanks
  j

> +
>  } /* namespace libcamera */
>
>  #endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181224/6b2b91ba/attachment.sig>