| Message ID | 20181222230041.29999-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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? > + > +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? > + > + 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 :) > + 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. > + 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. > +}; 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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__ */
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@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 > > >
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) +{ +} + +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); + + dev = udev_device_new_from_syspath(udev_, syspath); + if (dev == NULL) { + LOG(Error) << "Failed to device for '" << syspath << "', skipping"; + continue; + } + + devnode = udev_device_get_devnode(dev); + if (devnode == NULL) { + 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; + 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; +}; + } /* namespace libcamera */ #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */
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@ragnatech.se> --- src/libcamera/deviceenumerator.cpp | 100 +++++++++++++++++++++++ src/libcamera/include/deviceenumerator.h | 15 ++++ 2 files changed, 115 insertions(+)