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

[Niklas Söderlund]

Hi Kieran,

Thanks for your work.

On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 18/07/2019 14:56, Laurent Pinchart wrote:
> > 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).
> 
> -EPIPE 'sounds' appropriate, because we have a broken pipe if we don't
> have a stream pointer - as long as that doesn't get too confusing
> against other types of pipe :)
> 
> Or we could use: "ESTRPIPE 86 Streams pipe error"?
> 
> But then again for a generic 'Fatal' error code we could choose one of:
> 
>   EOWNERDEAD 130 Owner died
>   ENOTRECOVERABLE 131 State not recoverable
> 
> I quite like both of those for a 'fatal' error.

Is this not an academic question? As LOG(..., Fatal) will never return 
and terminate the application?

If we need a return statement to keep analysers happy add one in the LOG 
macro. The return code could be EXIT_FAILURE.

> 
> 
> >> +		}
> >>  
> >>  		stream->configuration_ = cfg;
> >>  		activeStreams_.insert(stream);
> > 
> 
> -- 
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund