Message ID | 20240807134950.2458614-1-kieran.bingham@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Kieran, Thank you for the patch. On Wed, Aug 07, 2024 at 02:49:50PM +0100, Kieran Bingham wrote: > The udev_monitor_receive_device() can return NULL on an error as > detailed in the man pages for the function. > > The udevNotify() handler in the DeviceEnumeratorUdev directly uses the > return value of udev_monitor_receive_device() in successive calls to > process the event without having first checked the udev_device. > > Ensure we identify, and handle events where the udev_device can not be > returned successfully. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=230 > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > v2: > - Report strerror based on errno from the udev_monitor_receive_device() > call. > > src/libcamera/device_enumerator_udev.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > index 01c70b6daa82..53eeb772d900 100644 > --- a/src/libcamera/device_enumerator_udev.cpp > +++ b/src/libcamera/device_enumerator_udev.cpp > @@ -332,6 +332,14 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > void DeviceEnumeratorUdev::udevNotify() > { > struct udev_device *dev = udev_monitor_receive_device(monitor_); > + if (!dev) { > + int error = errno; grepping for ' = errno' gives me 12 variables named ret, and one named err. I'd go for 'ret' here too for consitency. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Could you ask the bug reporter to test this patch and tell what error number they get ? > + LOG(DeviceEnumerator, Warning) > + << "Ignoring notfication received without a device: " > + << strerror(error); > + return; > + } > + > std::string_view action(udev_device_get_action(dev)); > std::string_view deviceNode(udev_device_get_devnode(dev)); >
Quoting Laurent Pinchart (2024-08-07 15:24:55) > Hi Kieran, > > Thank you for the patch. > > On Wed, Aug 07, 2024 at 02:49:50PM +0100, Kieran Bingham wrote: > > The udev_monitor_receive_device() can return NULL on an error as > > detailed in the man pages for the function. > > > > The udevNotify() handler in the DeviceEnumeratorUdev directly uses the > > return value of udev_monitor_receive_device() in successive calls to > > process the event without having first checked the udev_device. > > > > Ensure we identify, and handle events where the udev_device can not be > > returned successfully. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=230 > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > v2: > > - Report strerror based on errno from the udev_monitor_receive_device() > > call. > > > > src/libcamera/device_enumerator_udev.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > index 01c70b6daa82..53eeb772d900 100644 > > --- a/src/libcamera/device_enumerator_udev.cpp > > +++ b/src/libcamera/device_enumerator_udev.cpp > > @@ -332,6 +332,14 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > void DeviceEnumeratorUdev::udevNotify() > > { > > struct udev_device *dev = udev_monitor_receive_device(monitor_); > > + if (!dev) { > > + int error = errno; > > grepping for ' = errno' gives me 12 variables named ret, and one named > err. I'd go for 'ret' here too for consitency. I explicitly didn't use 'ret' because it's not a ret value... And 'err' was just shorthand. I'll rename if you wish, but please confirm. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Could you ask the bug reporter to test this patch and tell what error > number they get ? Yes, but I don't know how they'll test it, as it's part of a separate CI system, so I don't think we'll get a response (until it filters into a release, that then gets utilised in that CI system). -- Kieran > > > + LOG(DeviceEnumerator, Warning) > > + << "Ignoring notfication received without a device: " > > + << strerror(error); > > + return; > > + } > > + > > std::string_view action(udev_device_get_action(dev)); > > std::string_view deviceNode(udev_device_get_devnode(dev)); > > > > -- > Regards, > > Laurent Pinchart
On Wed, Aug 07, 2024 at 03:33:20PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-08-07 15:24:55) > > On Wed, Aug 07, 2024 at 02:49:50PM +0100, Kieran Bingham wrote: > > > The udev_monitor_receive_device() can return NULL on an error as > > > detailed in the man pages for the function. > > > > > > The udevNotify() handler in the DeviceEnumeratorUdev directly uses the > > > return value of udev_monitor_receive_device() in successive calls to > > > process the event without having first checked the udev_device. > > > > > > Ensure we identify, and handle events where the udev_device can not be > > > returned successfully. > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=230 > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > --- > > > v2: > > > - Report strerror based on errno from the udev_monitor_receive_device() > > > call. > > > > > > src/libcamera/device_enumerator_udev.cpp | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > > index 01c70b6daa82..53eeb772d900 100644 > > > --- a/src/libcamera/device_enumerator_udev.cpp > > > +++ b/src/libcamera/device_enumerator_udev.cpp > > > @@ -332,6 +332,14 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > > void DeviceEnumeratorUdev::udevNotify() > > > { > > > struct udev_device *dev = udev_monitor_receive_device(monitor_); > > > + if (!dev) { > > > + int error = errno; > > > > grepping for ' = errno' gives me 12 variables named ret, and one named > > err. I'd go for 'ret' here too for consitency. > > I explicitly didn't use 'ret' because it's not a ret value... > > And 'err' was just shorthand. I'll rename if you wish, but please > confirm. I'd prefer 'ret' but I'm also fine with 'err'. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Could you ask the bug reporter to test this patch and tell what error > > number they get ? > > Yes, but I don't know how they'll test it, as it's part of a separate CI > system, so I don't think we'll get a response (until it filters into a > release, that then gets utilised in that CI system). Can't they push a test branch to that CI system, or run the failing tests locally ? > > > + LOG(DeviceEnumerator, Warning) > > > + << "Ignoring notfication received without a device: " > > > + << strerror(error); > > > + return; > > > + } > > > + > > > std::string_view action(udev_device_get_action(dev)); > > > std::string_view deviceNode(udev_device_get_devnode(dev)); > > >
Quoting Laurent Pinchart (2024-08-07 16:01:59) > On Wed, Aug 07, 2024 at 03:33:20PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2024-08-07 15:24:55) > > > On Wed, Aug 07, 2024 at 02:49:50PM +0100, Kieran Bingham wrote: > > > > The udev_monitor_receive_device() can return NULL on an error as > > > > detailed in the man pages for the function. > > > > > > > > The udevNotify() handler in the DeviceEnumeratorUdev directly uses the > > > > return value of udev_monitor_receive_device() in successive calls to > > > > process the event without having first checked the udev_device. > > > > > > > > Ensure we identify, and handle events where the udev_device can not be > > > > returned successfully. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=230 > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > --- > > > > v2: > > > > - Report strerror based on errno from the udev_monitor_receive_device() > > > > call. > > > > > > > > src/libcamera/device_enumerator_udev.cpp | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > > > index 01c70b6daa82..53eeb772d900 100644 > > > > --- a/src/libcamera/device_enumerator_udev.cpp > > > > +++ b/src/libcamera/device_enumerator_udev.cpp > > > > @@ -332,6 +332,14 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > > > void DeviceEnumeratorUdev::udevNotify() > > > > { > > > > struct udev_device *dev = udev_monitor_receive_device(monitor_); > > > > + if (!dev) { > > > > + int error = errno; > > > > > > grepping for ' = errno' gives me 12 variables named ret, and one named > > > err. I'd go for 'ret' here too for consitency. > > > > I explicitly didn't use 'ret' because it's not a ret value... > > > > And 'err' was just shorthand. I'll rename if you wish, but please > > confirm. > > I'd prefer 'ret' but I'm also fine with 'err'. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Could you ask the bug reporter to test this patch and tell what error > > > number they get ? > > > > Yes, but I don't know how they'll test it, as it's part of a separate CI > > system, so I don't think we'll get a response (until it filters into a > > release, that then gets utilised in that CI system). > > Can't they push a test branch to that CI system, or run the failing > tests locally ? You tell me ;-) But I would suspect that as they were testing "something else entirely" [0] it would require updating various CI docker images to make a different version of libcamera be built inside that test container (which I would anticipate would have been a binary package installation) - so that they could run their secondary test which was probably a race condition as 7 out of 8 other runs [1][2] on different runners worked successfully. I've asked [3] - I'm just saying that I'm not expecting any 'quick' answer. I think this is like asking them to test a systemd patch in our libcamera CI... [0] Update syscall tables and linux headers #33930 (systemd) https://github.com/systemd/systemd/pull/33930 [1] https://github.com/systemd/systemd/actions/runs/10233929013 [2] https://github.com/systemd/systemd/actions/runs/10233929013/job/28312766461 [3] https://github.com/systemd/systemd/pull/33930#issuecomment-2273739561 -- Kieran > > > > > + LOG(DeviceEnumerator, Warning) > > > > + << "Ignoring notfication received without a device: " > > > > + << strerror(error); > > > > + return; > > > > + } > > > > + > > > > std::string_view action(udev_device_get_action(dev)); > > > > std::string_view deviceNode(udev_device_get_devnode(dev)); > > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 01c70b6daa82..53eeb772d900 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -332,6 +332,14 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) void DeviceEnumeratorUdev::udevNotify() { struct udev_device *dev = udev_monitor_receive_device(monitor_); + if (!dev) { + int error = errno; + LOG(DeviceEnumerator, Warning) + << "Ignoring notfication received without a device: " + << strerror(error); + return; + } + std::string_view action(udev_device_get_action(dev)); std::string_view deviceNode(udev_device_get_devnode(dev));