[PATCH v2] libcamera: udev: Catch udev notification errors

[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 at ideasonboard.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham at 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 at 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));
> > >  

-- 
Regards,

Laurent Pinchart