| Message ID | 20220317131726.1483971-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hello Kieran, On 3/17/22 18:47, Kieran Bingham via libcamera-devel wrote: > Requests are created by a Camera, and can only be queued > to that specific Camera. Enforce this during the public API > to prevent mis-use by incorrect applications. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Makes sense Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/camera.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index bb856d606f4a..dd6552e83eee 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -23,6 +23,7 @@ > #include "libcamera/internal/camera_controls.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/pipeline_handler.h" > +#include "libcamera/internal/request.h" > > /** > * \file libcamera/camera.h > @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) > if (ret < 0) > return ret; > > + /* Requests can only be queued to the camera that created them.*/ > + if (request->_d()->camera() != this) { > + LOG(Camera, Error) << "Request was not created by this camera"; > + return -EINVAL; > + } > + > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle
Hi Kieran, Thank you for the patch. On Thu, Mar 17, 2022 at 01:17:26PM +0000, Kieran Bingham via libcamera-devel wrote: > Requests are created by a Camera, and can only be queued > to that specific Camera. Enforce this during the public API > to prevent mis-use by incorrect applications. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index bb856d606f4a..dd6552e83eee 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -23,6 +23,7 @@ > #include "libcamera/internal/camera_controls.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/pipeline_handler.h" > +#include "libcamera/internal/request.h" > > /** > * \file libcamera/camera.h > @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) > if (ret < 0) > return ret; > > + /* Requests can only be queued to the camera that created them.*/ > + if (request->_d()->camera() != this) { > + LOG(Camera, Error) << "Request was not created by this camera"; > + return -EINVAL; > + } This could even be worth an ASSERT(), up to you. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle
Quoting Laurent Pinchart (2022-03-29 22:58:05) > Hi Kieran, > > Thank you for the patch. > > On Thu, Mar 17, 2022 at 01:17:26PM +0000, Kieran Bingham via libcamera-devel wrote: > > Requests are created by a Camera, and can only be queued > > to that specific Camera. Enforce this during the public API > > to prevent mis-use by incorrect applications. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/camera.cpp | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index bb856d606f4a..dd6552e83eee 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -23,6 +23,7 @@ > > #include "libcamera/internal/camera_controls.h" > > #include "libcamera/internal/formats.h" > > #include "libcamera/internal/pipeline_handler.h" > > +#include "libcamera/internal/request.h" > > > > /** > > * \file libcamera/camera.h > > @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) > > if (ret < 0) > > return ret; > > > > + /* Requests can only be queued to the camera that created them.*/ > > + if (request->_d()->camera() != this) { > > + LOG(Camera, Error) << "Request was not created by this camera"; > > + return -EINVAL; > > + } > > This could even be worth an ASSERT(), up to you. If this was the other direction, such as a libcamera request getting completed on the wrong camera, I would agree, that would be a fatal / assert - but this check is on the incoming public API - where I think it's better to just reject an invalid input. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks > > > + > > /* > > * The camera state may change until the end of the function. No locking > > * is however needed as PipelineHandler::queueRequest() will handle > > -- > Regards, > > Laurent Pinchart
Quoting Kieran Bingham (2022-03-17 13:17:26) > Requests are created by a Camera, and can only be queued > to that specific Camera. Enforce this during the public API > to prevent mis-use by incorrect applications. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index bb856d606f4a..dd6552e83eee 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -23,6 +23,7 @@ > #include "libcamera/internal/camera_controls.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/pipeline_handler.h" > +#include "libcamera/internal/request.h" > > /** > * \file libcamera/camera.h > @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) > if (ret < 0) > return ret; > > + /* Requests can only be queued to the camera that created them.*/ > + if (request->_d()->camera() != this) { > + LOG(Camera, Error) << "Request was not created by this camera"; > + return -EINVAL; I have two tags so I'm ready to merge this but ... I'm tempted to make this a distinct return code: EXDEV 18 Invalid cross-device link Adding + * \retval -EXDEV The request does not belong to this camera to the doxygen accordingly. Any comments, either a +1 or a -1 on whether I should or should not post a v2 with this? -- Kieran > + } > + > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle > -- > 2.32.0 >
Hi Kieran, On 4/11/22 01:54, Kieran Bingham via libcamera-devel wrote: > Quoting Kieran Bingham (2022-03-17 13:17:26) >> Requests are created by a Camera, and can only be queued >> to that specific Camera. Enforce this during the public API >> to prevent mis-use by incorrect applications. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/camera.cpp | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index bb856d606f4a..dd6552e83eee 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -23,6 +23,7 @@ >> #include "libcamera/internal/camera_controls.h" >> #include "libcamera/internal/formats.h" >> #include "libcamera/internal/pipeline_handler.h" >> +#include "libcamera/internal/request.h" >> >> /** >> * \file libcamera/camera.h >> @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) >> if (ret < 0) >> return ret; >> >> + /* Requests can only be queued to the camera that created them.*/ >> + if (request->_d()->camera() != this) { >> + LOG(Camera, Error) << "Request was not created by this camera"; >> + return -EINVAL; > I have two tags so I'm ready to merge this but ... I'm tempted to make > this a distinct return code: > > EXDEV 18 Invalid cross-device link > > Adding > + * \retval -EXDEV The request does not belong to this camera > > to the doxygen accordingly. > > Any comments, either a +1 or a -1 on whether I should or should not post > a v2 with this? -1 (i.e. shouldn't need a v2 on the list). I am okay returning with EXDEV as the error code. > > -- > Kieran > > >> + } >> + >> /* >> * The camera state may change until the end of the function. No locking >> * is however needed as PipelineHandler::queueRequest() will handle >> -- >> 2.32.0 >>
On Mon, Apr 11, 2022 at 03:02:07PM +0530, Umang Jain via libcamera-devel wrote: > Hi Kieran, > > On 4/11/22 01:54, Kieran Bingham via libcamera-devel wrote: > > Quoting Kieran Bingham (2022-03-17 13:17:26) > >> Requests are created by a Camera, and can only be queued > >> to that specific Camera. Enforce this during the public API > >> to prevent mis-use by incorrect applications. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/camera.cpp | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >> index bb856d606f4a..dd6552e83eee 100644 > >> --- a/src/libcamera/camera.cpp > >> +++ b/src/libcamera/camera.cpp > >> @@ -23,6 +23,7 @@ > >> #include "libcamera/internal/camera_controls.h" > >> #include "libcamera/internal/formats.h" > >> #include "libcamera/internal/pipeline_handler.h" > >> +#include "libcamera/internal/request.h" > >> > >> /** > >> * \file libcamera/camera.h > >> @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) > >> if (ret < 0) > >> return ret; > >> > >> + /* Requests can only be queued to the camera that created them.*/ > >> + if (request->_d()->camera() != this) { > >> + LOG(Camera, Error) << "Request was not created by this camera"; > >> + return -EINVAL; > > I have two tags so I'm ready to merge this but ... I'm tempted to make > > this a distinct return code: > > > > EXDEV 18 Invalid cross-device link > > > > Adding > > + * \retval -EXDEV The request does not belong to this camera > > > > to the doxygen accordingly. > > > > Any comments, either a +1 or a -1 on whether I should or should not post > > a v2 with this? > > > -1 (i.e. shouldn't need a v2 on the list). > > I am okay returning with EXDEV as the error code. Agreed, -EXDEV looks good to me too. > >> + } > >> + > >> /* > >> * The camera state may change until the end of the function. No locking > >> * is however needed as PipelineHandler::queueRequest() will handle
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index bb856d606f4a..dd6552e83eee 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -23,6 +23,7 @@ #include "libcamera/internal/camera_controls.h" #include "libcamera/internal/formats.h" #include "libcamera/internal/pipeline_handler.h" +#include "libcamera/internal/request.h" /** * \file libcamera/camera.h @@ -1119,6 +1120,12 @@ int Camera::queueRequest(Request *request) if (ret < 0) return ret; + /* Requests can only be queued to the camera that created them.*/ + if (request->_d()->camera() != this) { + LOG(Camera, Error) << "Request was not created by this camera"; + return -EINVAL; + } + /* * The camera state may change until the end of the function. No locking * is however needed as PipelineHandler::queueRequest() will handle
Requests are created by a Camera, and can only be queued to that specific Camera. Enforce this during the public API to prevent mis-use by incorrect applications. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera.cpp | 7 +++++++ 1 file changed, 7 insertions(+)