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

[Kieran Bingham]

Hi Niklas,

On 20/07/2019 14:22, Niklas Söderlund wrote:
> 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?

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?

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



> 
>>
>>
>>>> +		}
>>>>  
>>>>  		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
--
Kieran