| Message ID | 1356301444-10191-1-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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; }
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.