[libcamera-devel] [PATCH v5 2/7] libcamera: camera: Propagate freeBuffers() error

[Laurent Pinchart]

Hello,

On Tue, Apr 16, 2019 at 01:43:28AM +0200, Niklas Söderlund wrote:
> On 2019-04-16 01:18:54 +0200, Jacopo Mondi wrote:
> > The error return code of PipelineHandler::freeBuffers() was not
> > propagate up to applications by the Camera class. Fix this by returning
> > the incremental error accumulated by multiple calls (one per Stream)
> > to freeBuffers().
> > 
> > Do not return the error code of the call to freeBuffers() in the
> > allocateBuffers() method error path not to overwrite the original error
> > code returned from the allocation method.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/camera.cpp | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index bdf14b31d8ee..7f2dc904df16 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -671,6 +671,8 @@ int Camera::allocateBuffers()
> >   */
> >  int Camera::freeBuffers()
> >  {
> > +	int ret = 0;
> > +
> >  	if (!stateIs(CameraPrepared))
> >  		return -EACCES;
> >  
> > @@ -683,12 +685,12 @@ int Camera::freeBuffers()
> >  		 * by the V4L2 device that has allocated them.
> >  		 */
> >  		stream->bufferPool().destroyBuffers();
> > -		pipe_->freeBuffers(this, stream);
> > +		ret |= pipe_->freeBuffers(this, stream);
> 
> Or:ing in errors here seems very strange as we are dealing with the 
> standard error codes (EINVAL, EBUSY, etc). Would it not be saner to 
> store and return the (potential) first error while still running 
> freeBuffers() for all streams before returning?
> 
> How about something like this
> 
>     int ret = 0;
> 
>     for (Stream *stream : activeStreams_) {
>         int status = pipe_->freeBuffers(this, stream);
> 
>         if (!ret)
>             ret = status;
>     }
> 
>     return ret;

Or maybe merging this on top of the patch that refactors buffer
allocation ? The loop will then be gone.

> >  	}
> >  
> >  	state_ = CameraConfigured;
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  /**

-- 
Regards,

Laurent Pinchart