[PATCH] libcamera: udev: Catch udev notification errors

[Laurent Pinchart]

On Tue, Aug 06, 2024 at 09:19:37AM +0200, Stefan Klug wrote:
> Hi Kieran,
> 
> Thank you for the patch. 
> 
> On Mon, Aug 05, 2024 at 11:31:48PM +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
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> > This is a proposed fix for the reported CI failure caught by Systemd
> > runners. It's not clear if this failure was an injected failure, startup
> > race or otherwise - but given the documentation of this function states
> > it can return null ... it seems valid to null check before passing into
> > other APIs especially when a trace is reported directly at this point.
> > 
> > I don't presently have a way to replicate or reproduce the error either,
> > but this patch itself has passed our ci tests (which isn't too
> > unexpected as the change is quite minimal otherwise).
> > 
> > 
> >  src/libcamera/device_enumerator_udev.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > index 01c70b6daa82..c6c78651baa0 100644
> > --- a/src/libcamera/device_enumerator_udev.cpp
> > +++ b/src/libcamera/device_enumerator_udev.cpp
> > @@ -332,6 +332,12 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
> >  void DeviceEnumeratorUdev::udevNotify()
> >  {
> >  	struct udev_device *dev = udev_monitor_receive_device(monitor_);
> > +	if (!dev) {
> > +		LOG(DeviceEnumerator, Warning)
> > +			<< "Ignoring notfication received without a device";
> 
> Is that even worth a warning or only debug?

As we've never seen this product for years, I wonder why it's happening.
A warning would help noticing the issue. On the other hand, are such
errors means to happen in rare ubt valid cases ? And then, are they
meant to be ignored, or do they indicate something is fundamentally
wrong and action needs to be taken ? If no action is needed and there
are no consequences, then a debug message is more than enough.

If we want to diagnose this, we need more information. The message
should print the value of errno, and it would be nice if the original
reporter of the issues could then report the error number.

> Anyways:
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com> 
> 
> > +		return;
> > +	}
> > +
> >  	std::string_view action(udev_device_get_action(dev));
> >  	std::string_view deviceNode(udev_device_get_devnode(dev));

-- 
Regards,

Laurent Pinchart