[libcamera-devel] [PATCH v4 08/12] libcamera: camera: Refuse empty requests

[Jacopo Mondi]

Hi Laurent,

On Mon, Apr 15, 2019 at 05:19:29PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Apr 15, 2019 at 02:40:36PM +0200, Jacopo Mondi wrote:
> > On Sun, Apr 14, 2019 at 10:08:44PM +0200, Niklas Söderlund wrote:
> > > On 2019-04-09 21:25:44 +0200, Jacopo Mondi wrote:
> > > > Requests that do not contain any Stream should not be forwarded to
> > > > the pipeline handlers. Return -EINVAL if the request does not contain
> > > > any Stream to capture from.
>
> When we'll add support for controls this will not necessarily hold true
> anymore (although I'm not sure what the use cases would be). In any case
> I don't think it's a reason to block this patch.
>
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/libcamera/camera.cpp | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index cb392ca3b7e7..bd73ff69c3da 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -728,6 +728,7 @@ Request *Camera::createRequest()
> > > >   * \return 0 on success or a negative error code otherwise
> > > >   * \retval -ENODEV The camera has been disconnected from the system
> > > >   * \retval -EACCES The camera is not running so requests can't be queued
> > > > + * \retval -EINVAL The request does not contain any stream to capture from
>
> In the future I'm sure we'll have other types of invalid requests,
> should we already just state that the request is invalid without
> detailing why ? It may make sense to add a new paragraph to the
> documentation of this function to explain what a valid request is (or
> move that to the documentation of Request::valid() if you decide to go
> that route).
>
> > > >   */
> > > >  int Camera::queueRequest(Request *request)
> > > >  {
> > > > @@ -743,6 +744,9 @@ int Camera::queueRequest(Request *request)
> > > >  		return ret;
> > > >  	}
> > > >
> > > > +	if (request->empty())
>
> I think you should log a debug message.
>
> > > > +		return -EINVAL;
> > > > +
> > >
> > > Should this be moved inside Request::prepare() ?
> >
> > I considered doing that, but this sees to me like something that
> > should live in a 'validate()' method more than in 'prepare()'.
> >
> > As long as this is only validation we have I think it could live
> > here, to be later moved to a 'validate()' method if we see any need
> > for that. What do you think?
>
> Maybe you could rename Request::empty() to Request::valid() ?
>

This would be very convienent, but I'm not sure a request should be
self-validating honestly... Although a Request::valid() function might
make sense to perform the basic checks, I think there should be a
validation performed by the Camera class too (and posibly a hook to
pipeline handler too? as some control might be valid for one
implementation but not for the other? That's for later thuough...)

I'll change empty() to valid() and perform a first validity check,
such as making sure the request has at least one Stream where to
capture from...

Thanks
  j

> > > >  	return pipe_->queueRequest(this, request);
> > > >  }
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190415/8ad69191/attachment-0001.sig>