[libcamera-devel] [PATCH] libcamera: camera: Fail to configure on mis-configured streams

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Jul 18, 2019 at 05:27:54AM +0100, Kieran Bingham wrote:
> Uses of LOG(x, Fatal) and ASSERT() should be capable of continuing
> without crashing.
> 
> Camera::configure() validates that each StreamConfiguration has a
> Stream* and reports a Fatal error otherwise. The code then goes on to
> dereference the Stream pointer which will be a nullptr in the event of
> the Fatal error being triggered.
> 
> In release builds, ASSERTS are compiled out, and LOG(x, Fatal) should
> attempt to continue or report the error up the call-stack.
> 
> Make Camera::configure() return an error in the event that the Stream*
> is not valid. This will cause the configure operation to fail and the
> application will be notified just as other errors in the
> Camera::configure() operation do.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/camera.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 76c737cb9381..7ad9e0e48686 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -672,9 +672,11 @@ int Camera::configure(CameraConfiguration *config)
>  	activeStreams_.clear();
>  	for (const StreamConfiguration &cfg : *config) {
>  		Stream *stream = cfg.stream();
> -		if (!stream)
> +		if (!stream) {
>  			LOG(Camera, Fatal)
>  				<< "Pipeline handler failed to update stream configuration";
> +			return -EINVAL;

Overall this makes sense, but I wonder if -EINVAL is the appropriate
error code. The issue at hand is that the pipeline handler is buggy. A
specific error code to denote errors that are not supposed to happen
ever would be best I think (and bonus points if we could use that error
code consistently through libcamera).

> +		}
>  
>  		stream->configuration_ = cfg;
>  		activeStreams_.insert(stream);

-- 
Regards,

Laurent Pinchart