[RFC/PATCH] v4l2-compliance: Reject invalid ioctl error codes

[RFC/PATCH] v4l2-compliance: Reject invalid ioctl error codes

[Laurent Pinchart]

The recent uvcvideo regression that broke pulseaudio/KDE (see commit
9c016d61097cc39427a2f5025bdd97ac633d26a6 in the mainline kernel) was
caused by the uvcvideo driver returning a -ENOENT error code to
userspace by mistake.

To make sure such regressions will be caught before reaching users, test
ioctl error codes to make sure they're valid.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/v4l2-compliance/v4l2-compliance.cpp |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

A white list of valid error codes might be more appropriate. I can fix the
patch accordingly, but I'd like a general opinion first.

diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
index 1e4646f..ff1ad9b 100644
--- a/utils/v4l2-compliance/v4l2-compliance.cpp
+++ b/utils/v4l2-compliance/v4l2-compliance.cpp
@@ -112,6 +112,13 @@ int doioctl_name(struct node *node, unsigned long int request, void *parm, const
 		fail("%s returned %d instead of 0 or -1\n", name, retval);
 		return -1;
 	}
+
+	/* Reject invalid error codes */
+	switch (errno) {
+	case ENOENT:
+		fail("%s returned invalid error %d\n", name, errno);
+		break;
+	}
 	return e;
 }
 
-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH] v4l2-compliance: Reject invalid ioctl error codes

[Hans Verkuil]

On Sun December 23 2012 23:24:04 Laurent Pinchart wrote:
> The recent uvcvideo regression that broke pulseaudio/KDE (see commit
> 9c016d61097cc39427a2f5025bdd97ac633d26a6 in the mainline kernel) was
> caused by the uvcvideo driver returning a -ENOENT error code to
> userspace by mistake.
> 
> To make sure such regressions will be caught before reaching users, test
> ioctl error codes to make sure they're valid.

I don't like this change. Error codes should be checked in the test for
the actual ioctl.

Apparently it is QUERYCTRL that is returning the wrong error code in uvc, but
looking at the code in v4l2-test-controls.cpp it is already checking for ENOTTY
or EINVAL and returning a failure if it is a different error code. So why is
that not triggered in this case?

Regards,

	Hans

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/v4l2-compliance/v4l2-compliance.cpp |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> A white list of valid error codes might be more appropriate. I can fix the
> patch accordingly, but I'd like a general opinion first.
> 
> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
> index 1e4646f..ff1ad9b 100644
> --- a/utils/v4l2-compliance/v4l2-compliance.cpp
> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp
> @@ -112,6 +112,13 @@ int doioctl_name(struct node *node, unsigned long int request, void *parm, const
>  		fail("%s returned %d instead of 0 or -1\n", name, retval);
>  		return -1;
>  	}
> +
> +	/* Reject invalid error codes */
> +	switch (errno) {
> +	case ENOENT:
> +		fail("%s returned invalid error %d\n", name, errno);
> +		break;
> +	}
>  	return e;
>  }
>  
> 

Re: [RFC/PATCH] v4l2-compliance: Reject invalid ioctl error codes

[Laurent Pinchart]

Hi Hans,

On Monday 24 December 2012 10:24:48 Hans Verkuil wrote:
> On Sun December 23 2012 23:24:04 Laurent Pinchart wrote:
> > The recent uvcvideo regression that broke pulseaudio/KDE (see commit
> > 9c016d61097cc39427a2f5025bdd97ac633d26a6 in the mainline kernel) was
> > caused by the uvcvideo driver returning a -ENOENT error code to
> > userspace by mistake.
> > 
> > To make sure such regressions will be caught before reaching users, test
> > ioctl error codes to make sure they're valid.
> 
> I don't like this change. Error codes should be checked in the test for
> the actual ioctl.
> 
> Apparently it is QUERYCTRL that is returning the wrong error code in uvc,
> but looking at the code in v4l2-test-controls.cpp it is already checking
> for ENOTTY or EINVAL and returning a failure if it is a different error
> code. So why is that not triggered in this case?

I've just checked that, the missing control class issue made the control tests 
stop early before hitting the wrong return value. I guess that's a good reason 
to fix *all* compliance errors...

We can drop this patch.

-- 
Regards,

Laurent Pinchart