[PATCH] CameraManager: Ensure we cleanup on failure

[Jacopo Mondi]

Hi Kieran

On Fri, Nov 08, 2024 at 11:34:15PM +0000, Kieran Bingham wrote:
> If the CameraManager fails to initialise at startup during
> CameraManager::Private::init(), then the run() function will prepare the
> thread for operations, but the thread will immediately close without
> executing any cleanup.
>
> This can leave instantiated objects such as the EventNotifier registered
> by the udev enumerator constructed in a thread which no longer exists.
> The destructor of those objects can then fire an assertion that they are
> being executed from an incorrect thread while performing their cleanup.
>
> Ensure that the failure path does not result in reporting thread
> ownership assertions by performing cleanup correctly on the
> CameraManager thread before it closes.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>

Seems sane to me!
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

> ---
> This is the result of debugging an awkward error on an unrelated failure
> path that caused the CameraManager to fail to construct:
>
> """
> camera_sensor_legacy.cpp:850 'ar0144 2-0010': The analogue crop rectangle has been defaulted to the active area size
> [1:49:26.079788375] [922]  WARN RkISP1Agc agc.cpp:46 'AeMeteringMode' parameter not found in tuning file
> [1:49:26.079829125] [922]  WARN RkISP1Agc agc.cpp:69 No metering modes read from tuning file; defaulting to matrix
> [1:49:26.080311125] [921] ERROR Camera camera_manager.cpp:350 Failed to start camera manager: No such file or directory
> Failed to start camera manager: No such file or directory
> [1:49:26.080547875] [921] ERROR Object object.cpp:252 EventNotifier can't be enabled from another thread
> [1:49:26.080582375] [921] FATAL default object.cpp:253 assertion "false" failed in assertThreadBound()
> Backtrace:
> /usr/lib/libcamera-base.so.0.3(_ZN9libcamera6Object17assertThreadBoundEPKc+0xe4) [0xffffa35fd884]
> /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifier10setEnabledEb+0x24) [0xffffa3601b34]
> /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifierD2Ev+0x50) [0xffffa3601bf0]
> /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifierD0Ev+0x18) [0xffffa3601cb8]
> /usr/lib/libcamera.so.0.3(_ZN9libcamera20DeviceEnumeratorUdevD2Ev+0x38) [0xffffa3798698]
> /usr/lib/libcamera.so.0.3(_ZN9libcamera20DeviceEnumeratorUdevD0Ev+0x18) [0xffffa3798738]
> /usr/lib/libcamera.so.0.3(+0x997cc) [0xffffa36d97cc]
> /usr/lib/libcamera.so.0.3(_ZN9libcamera13CameraManagerD1Ev+0x134) [0xffffa36d6f04]
> /usr/lib/libcamera.so.0.3(_ZN9libcamera13CameraManagerD0Ev+0x18) [0xffffa36d6f78]
> cam(+0x1ba60) [0xaaaad8d9ba60]
> cam(+0x8b64) [0xaaaad8d88b64]
> /usr/lib/libc.so.6(+0x284b4) [0xffffa30984b4]
> /usr/lib/libc.so.6(__libc_start_main+0x9c) [0xffffa309858c]
> cam(+0x8fb0) [0xaaaad8d88fb0]
> Aborted (core dumped)
> """
>
> Whilst the error "No such file or directory" reported above was out of
> tree code causing the camera manager to fail - libcamera should not then
> fail with an assertion after that while shutting down and calling
> destructors.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/camera_manager.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 6280b2b3e485..a8b36bcea2d4 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -83,8 +83,10 @@ void CameraManager::Private::run()
>  	mutex_.unlock();
>  	cv_.notify_one();
>
> -	if (ret < 0)
> +	if (ret < 0) {
> +		cleanup();
>  		return;
> +	}
>
>  	/* Now start processing events and messages. */
>  	exec();
> --
> 2.47.0
>