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

[Laurent Pinchart]

Hi Niklas,

On Tue, Jul 30, 2019 at 07:37:15AM +0200, Niklas Söderlund wrote:
> On 2019-07-30 07:09:05 +0300, Laurent Pinchart wrote:
> > 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.

>From a libcamera point of view that's true, but from an application
point of view there may be a use for closing gracefully and saving data.
For me LOG(..., Fatal) is a LOG() + ASSERT(), but if we have to keep the
same behaviour in both release and debug builds, I would prefer removing
the std::abort() call completely from LOG(..., Fatal).

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