| Message ID | 20190226021857.28255-8-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, great! If this needs to be checked in other places, you could consider a method for this in future. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j 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@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>{}; > + > 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; > + > ret = pipe_->configureStreams(this, config); > if (ret) > return ret; > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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 ? 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 ? > 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;
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@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
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@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;
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>{}; + 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; + ret = pipe_->configureStreams(this, config); if (ret) return ret;
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@ragnatech.se> --- src/libcamera/camera.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)