[libcamera-devel] [PATCH 06/12] libcamera: deviceenumerator: add DeviceEnumeratorUdev class
Niklas S??derlund
niklas.soderlund at ragnatech.se
Sat Dec 29 02:54:15 CET 2018
Hi Laurent,
Thanks for your comments.
On 2018-12-29 01:23:50 +0200, Laurent Pinchart wrote:
> 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).
Thanks.
>
> > > > +{
> > > > +}
> > >
> > > 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
I went with this approach.
>
> > > > +}
> > > > +
> > > >
> > > > } /* 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
>
>
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list