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

[Laurent Pinchart]

Hello,

On Friday, 28 December 2018 05:31:51 EET Niklas S??derlund wrote:
> On 2018-12-24 12:40:49 +0100, Jacopo Mondi wrote:
> > 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)

nullptr (same through the whole series).

> > > +{
> > > +}
> > 
> > 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)

How about listing output variables after input variables ?

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

I think it makes sense to group variable declaration and code when they're 
related, but for variables reused through the function (such as ret) I find 
declaring them at the beginning of the function better than at the location of 
their first use.

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

You could also return an empty string

> > > +}
> > > +
> > > 
> > >  } /* 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 :)

It's an internal API so it can always change later :-)

> > If not or the design thing, all minors comment :)
> > 
> > > +
> > > 
> > >  } /* namespace libcamera */
> > >  
> > >  #endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */

-- 
Regards,

Laurent Pinchart