[libcamera-devel] [PATCH 01/19] test: buffer_import: Propagate status code from buffer allocation

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 20, 2020 at 12:09:17PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 20, 2020 at 02:24:19AM +0200, Laurent Pinchart wrote:
> > The BufferSource::allocate() return value isn't propagated correctly,
> > resulting in a test failure when the test should be skipped due to a
> > missing vivid device. Fix it.
> >
> > While at it, return valid status codes from BufferSource::allocate() in
> > all error cases, with proper diagnostic messages.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  test/camera/buffer_import.cpp | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > index e7048335e031..16bfca9beeea 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -89,15 +89,23 @@ public:
> >  		ret = video_->getFormat(&format);
> >  		if (ret) {
> >  			std::cout << "Failed to get format on output device" << std::endl;
> > -			return ret;
> > +			return TestFail;
> 
> Nitpicking, but we now have 2 occurrences of
> 
>                 ret = video->doSomething()
>                 if (ret) {
> 
>                 }
> 
> Where the value of ret is actually later ignored.
> 
> >  		}
> >
> >  		format.size = config.size;
> >  		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);
> > -		if (video_->setFormat(&format))
> > +		if (video_->setFormat(&format)) {
> 
> I would unify all three calls here to this format
> 
>                 if (video_->doSomething()) {
> 
>                 }
> and remove ret

OK I'll change that.

> With or without this considered
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +			std::cout << "Failed to set format on output device" << std::endl;
> >  			return TestFail;
> > +		}
> >
> > -		return video_->exportBuffers(config.bufferCount, &buffers_);
> > +		ret = video_->exportBuffers(config.bufferCount, &buffers_);
> > +		if (ret < 0) {
> > +			std::cout << "Failed to export buffers" << std::endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> >  	}
> >
> >  	const std::vector<std::unique_ptr<FrameBuffer>> &buffers()
> > @@ -178,8 +186,8 @@ protected:
> >
> >  		BufferSource source;
> >  		int ret = source.allocate(cfg);
> > -		if (ret < 0)
> > -			return TestFail;
> > +		if (ret != TestPass)
> > +			return ret;
> >
> >  		std::vector<Request *> requests;
> >  		for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {

-- 
Regards,

Laurent Pinchart