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

[Kieran Bingham]

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.


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

-- 
Regards
--
Kieran