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

[Niklas S??derlund]

Hi Jacopo,

Thanks for your feedback.

On 2018-12-24 12:40:49 +0100, Jacopo Mondi wrote:
> 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?

Same answer :-) Why split it if there is one or more method implemented 
in the cpp file? I'm happy to do it in a standardized way just want to 
raise the question.

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

No preference, I always put declaration and assignment at the end of the 
list.

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

I agree I missed the verb ;-P


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

No opinion, lets reflect the outcome in the style document.

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

I agree, that's why I think this method is cleaner.

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



-- 
Regards,
Niklas Söderlund