[v2] libcamera: udev: Catch udev notification errors
diff mbox series

Message ID 20240807134950.2458614-1-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • [v2] libcamera: udev: Catch udev notification errors
Related show

Commit Message

Kieran Bingham Aug. 7, 2024, 1:49 p.m. UTC 
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(+)

Comments

Laurent Pinchart Aug. 7, 2024, 2:24 p.m. UTC | #1 
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));
>
Kieran Bingham Aug. 7, 2024, 2:33 p.m. UTC | #2 
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
Laurent Pinchart Aug. 7, 2024, 3:01 p.m. UTC | #3 
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));
> > >
Kieran Bingham Aug. 7, 2024, 3:32 p.m. UTC | #4 
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

Patch
diff mbox series 

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));