[libcamera-devel] [PATCH 06/12] libcamera: deviceenumerator: add DeviceEnumeratorUdev class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Dec 29 00:23:50 CET 2018
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
More information about the libcamera-devel
mailing list