[libcamera-devel] [PATCH 7/8] libcamera: camera: ensure streams belong to camera
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 28 18:19:13 CET 2019
Hi Niklas,
On Thu, Feb 28, 2019 at 02:55:50AM +0100, Niklas Söderlund wrote:
> On 2019-02-27 18:49:53 +0200, Laurent Pinchart wrote:
> > 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.
Sorry, I should have replied to this e-mail before 4/4 of v2. I fully
agree that pipeline handlers should not implement this on their own, but
I still wonder if application can still get this wrong in a plausible
way.
> > > 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
More information about the libcamera-devel
mailing list