[libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams belong to camera

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-02-27 18:49:53 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Feb 26, 2019 at 03:18:56AM +0100, Niklas Söderlund wrote:
> > Before calling into the pipeline handler make sure the streams provided
> > by the application actually belongs to the camera.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/camera.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 938f21fe80ef8ceb..ba8638009992170f 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -253,6 +253,10 @@ Camera::streamConfiguration(std::set<Stream *> &streams)
> >  	if (!stateIsAtleast(Free) || !streams.size())
> >  		return std::map<Stream *, StreamConfiguration>{};
> >  
> > +	for (Stream *stream : streams)
> > +		if (streams_.find(stream) == streams_.end())
> > +			return std::map<Stream *, StreamConfiguration>{};
> > +
> 
> Could we have braces for the for statement ?

Of course.

> 
> I'm beginning to wonder if the function shouldn't return an error value
> to differentiate between the differents errors, and take the std::map as
> an output parameter.
> 
> I also wonder if this check is worth it. Do you foresee this problem as
> common, or even possible without the application going through great
> efforts to make this happen ?

I'm not sure, we discussed this in Brussels and I recall we thought this 
was a good idea at the time. The rational was to guarantee that pipeline 
handlers wound not be tempted to implement this check on there own. I 
will keep this for next version but put it at the end so it can be 
dropped if we choose it's not needed.

> 
> >  	return pipe_->streamConfiguration(this, streams);
> >  }
> >  
> > @@ -290,6 +294,10 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> >  		return -EINVAL;
> >  	}
> >  
> > +	for (auto const &iter : config)
> > +		if (streams_.find(iter.first) == streams_.end())
> > +			return -EINVAL;
> > +
> 
> Braces here too please.
> 
> >  	ret = pipe_->configureStreams(this, config);
> >  	if (ret)
> >  		return ret;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund