Re: [PATCH 1/6] uvcvideo: Set error_idx properly for extended controls API failures

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Thursday 27 September 2012 17:16:15 Laurent Pinchart wrote:
> When one of the requested controls doesn't exist the error_idx field
> must reflect that situation. For G_EXT_CTRLS and S_EXT_CTRLS, error_idx
> must be set to the control count. For TRY_EXT_CTRLS, it must be set to
> the index of the unexisting control.
> 
> This issue was found by the v4l2-compliance tool.

I'm revisiting this patch as it has been reverted in v3.8-rc1.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c |   17 ++++++++++-------
>  drivers/media/usb/uvc/uvc_v4l2.c |   19 ++++++++++++-------
>  2 files changed, 22 insertions(+), 14 deletions(-)

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index f00db30..e5817b9 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -591,8 +591,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,

[snip]

> @@ -637,8 +639,9 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> unsigned int cmd, void *arg) ret = uvc_ctrl_get(chain, ctrl);
>  			if (ret < 0) {
>  				uvc_ctrl_rollback(handle);
> -				ctrls->error_idx = i;
> -				return ret;
> +				ctrls->error_idx = ret == -ENOENT
> +						 ? ctrls->count : i;
> +				return ret == -ENOENT ? -EINVAL : ret;
>  			}
>  		}
>  		ctrls->error_idx = 0;
> @@ -661,8 +664,10 @@ static long uvc_v4l2_do_ioctl(struct file *file,
> unsigned int cmd, void *arg) ret = uvc_ctrl_set(chain, ctrl);
>  			if (ret < 0) {
>  				uvc_ctrl_rollback(handle);
> -				ctrls->error_idx = i;
> -				return ret;
> +				ctrls->error_idx = (ret == -ENOENT &&
> +						    cmd == VIDIOC_S_EXT_CTRLS)
> +						 ? ctrls->count : i;
> +				return ret == -ENOENT ? -EINVAL : ret;
>  			}
>  		}

I've reread the V4L2 specification, and the least I can say is that the text 
is pretty ambiguous. Let's clarify it.

Is there a reason to differentiate between invalid control IDs and other 
errors as far as error_idx is concerned ? It would be simpler if error_idx was 
set to the index of the first error for get and try operations, regardless of 
the error type. What do you think ?

-- 
Regards,

Laurent Pinchart