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

[Laurent Pinchart]

Hello,

On Mon, Jul 22, 2019 at 10:54:26AM +0100, Kieran Bingham wrote:
> On 20/07/2019 14:22, Niklas Söderlund wrote:
> > On 2019-07-19 08:33:46 +0100, Kieran Bingham wrote:
> >> On 18/07/2019 14:56, Laurent Pinchart wrote:
> >>> 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.

I quite like ENOTRECOVERABLE too.

> > Is this not an academic question? As LOG(..., Fatal) will never return 
> > and terminate the application?
> 
> Currently, you are correct - but while discussing with Laurent on
> another patch somewhere, or perhaps it was IRC - I can't recall, we
> realised that we might have to make LOG(x, Fatal) *non-fatal* on release
> builds.
> 
> (Just as assertions are compiled out in release builds)
> 
> It might be that we leave this to a packaging policy as well.
> 
> Anyway, this is the only part of the code that in the event of the
> failure being detected would then go on to do a null-dereference - so I
> thought the small fix up was worth it.
> 
> Perhaps I should add a comment before the return explaining that this
> code is unreachable in debug builds, but could be enabled in future
> release builds?

I think mentioning it in the commit log is enough, there's no need to
sprinkle copies of the information through the code.

> > If we need a return statement to keep analysers happy add one in the LOG 
> > macro. The return code could be EXIT_FAILURE.
> 
> Eeeep - no - I really don't want to have a macro which returns implicitly.
> 
> I would really prefer to keep return paths explicit.

I also dislike hiding return statements in macros, even if it could help
standardising the return value. Let's also note that LOG(..., Fatal) can
be used in functions returning any type, so we can't simply return
-EWHATEVER there.

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

-- 
Regards,

Laurent Pinchart