[libcamera-devel] [PATCH 06/12] libcamera: deviceenumerator: add DeviceEnumeratorUdev class
Niklas S??derlund
niklas.soderlund at ragnatech.se
Fri Dec 28 04:31:51 CET 2018
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
More information about the libcamera-devel
mailing list