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

[Niklas Söderlund]

Hi,

On 2019-07-30 07:09:05 +0300, Laurent Pinchart wrote:
> 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.

I'm not sure I like the idea of allowing continued execution after a 
LOG(..., Fatal) in release builds. It feels a bit like Java, whenever 
one looks at a process logs it's sprinkled with unhandled expectation 
log messages and trying to narrow down a problem is impossible so one 
just gives up and once prejudice against the language is reinforced ;-)

I'm not against changing/removing LOG(..., Fatal) so that it always 
allow continued execution after it's been called. But I think it should 
be the same behavior always, we will have enough trouble as it is to 
test all different error paths :-)

ASSERT() for me is different, it's usually at the start of functions and 
could be in hot paths and do not alter the execution order of functions 
so it make sens to disable them in release builds as it decreases the 
execution time. While I think of LOG(..., Fatal) as BUG() if it happens 
we are in deep shit and there is no point in trying to recover from it.

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

-- 
Regards,
Niklas Söderlund