| Message ID | 4E075C45.3010200@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Sunday 26 June 2011 18:20:21 Mauro Carvalho Chehab wrote: > > The V4L2 core probably should return -ENOIOCTLCMD when an IOCTL isn't implemented, but as long as vfs_ioctl() would stay as it is, the user space would still get -EINVAL. Or is vfs_ioctl() about to change? > > > > fs/ioctl.c: > > ----8<----------- > > static long vfs_ioctl(struct file *filp, unsigned int cmd, > > unsigned long arg) > > { > > int error = -ENOTTY; > > > > if (!filp->f_op || !filp->f_op->unlocked_ioctl) > > goto out; > > > > error = filp->f_op->unlocked_ioctl(filp, cmd, arg); > > if (error == -ENOIOCTLCMD) > > error = -EINVAL; > > out: > > return error; > > } > > ----8<----------- One of the differences between the old ->ioctl() and the ->unlocked_ioctl() function is that unlocked_ioctl could point to the same function as ->compat_ioctl(), so we have to catch functions returning -ENOIOCTLCMD. > Good catch! > > At the recent git history, the return for -ENOIOCTLCMD were modified > by this changeset: > > commit b19dd42faf413b4705d4adb38521e82d73fa4249 > Author: Arnd Bergmann <arnd@arndb.de> > Date: Sun Jul 4 00:15:10 2010 +0200 > > bkl: Remove locked .ioctl file operation > ... > @@ -39,21 +38,12 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, > { > int error = -ENOTTY; > > - if (!filp->f_op) > + if (!filp->f_op || !filp->f_op->unlocked_ioctl) > goto out; > > - if (filp->f_op->unlocked_ioctl) { > - error = filp->f_op->unlocked_ioctl(filp, cmd, arg); > - if (error == -ENOIOCTLCMD) > - error = -EINVAL; > - goto out; > - } else if (filp->f_op->ioctl) { > - lock_kernel(); > - error = filp->f_op->ioctl(filp->f_path.dentry->d_inode, > - filp, cmd, arg); > - unlock_kernel(); > ... > > Before Arnd's patch, locked ioctl's were returning -ENOIOCTLCMD, and > unlocked ones were returning -EINVAL. Now, the return of -ENOIOCTLCMD > doesn't go to userspace anymore. IMO, that's wrong and can cause > regressions, as some subsystems like DVB were returning -ENOIOCTLCMD > to userspace. ENOIOCTLCMD should never be returned to user space, see the comment in include/linux/errno.h: /* * These should never be seen by user programs. To return one of ERESTART* * codes, signal_pending() MUST be set. Note that ptrace can observe these * at syscall exit tracing, but they will never be left for the debugged user * process to see. */ There was a lot of debate whether undefined ioctls on non-ttys should return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to -EINVAL at some point in the pre-git era, IIRC. Inside of v4l2, I believe this is handled by video_usercopy(), which turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe where this is not done correctly and we do return ENOIOCTLCMD to vfs_ioctl? > The right fix would be to remove this from fs: > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 1d9b9fc..802fbbd 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -41,8 +41,6 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, > goto out; > > error = filp->f_op->unlocked_ioctl(filp, cmd, arg); > - if (error == -ENOIOCTLCMD) > - error = -EINVAL; > out: > return error; > } > > However, the replacement from -EINVAL to -ENOIOCTLCMD is there since 2.6.12 for > unlocked_ioctl: > > $ git blame b19dd42f^1 fs/ioctl.c > ... > ^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700 46) error = filp->f_op->unlocked_ioctl(filp, cmd, arg); > ^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700 47) if (error == -ENOIOCTLCMD) > ^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700 48) error = -EINVAL; > > Linus, > > what would be the expected behaviour? Note that 1da177e is the initial commit to git, Linus did not write that code, although he might have an opinion. Arnd -- 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
Em 26-06-2011 14:13, Arnd Bergmann escreveu: > On Sunday 26 June 2011 18:20:21 Mauro Carvalho Chehab wrote: > >>> The V4L2 core probably should return -ENOIOCTLCMD when an IOCTL isn't implemented, but as long as vfs_ioctl() would stay as it is, the user space would still get -EINVAL. Or is vfs_ioctl() about to change? >>> >>> fs/ioctl.c: >>> ----8<----------- >>> static long vfs_ioctl(struct file *filp, unsigned int cmd, >>> unsigned long arg) >>> { >>> int error = -ENOTTY; >>> >>> if (!filp->f_op || !filp->f_op->unlocked_ioctl) >>> goto out; >>> >>> error = filp->f_op->unlocked_ioctl(filp, cmd, arg); >>> if (error == -ENOIOCTLCMD) >>> error = -EINVAL; >>> out: >>> return error; >>> } >>> ----8<----------- > > One of the differences between the old ->ioctl() and the ->unlocked_ioctl() > function is that unlocked_ioctl could point to the same function as > ->compat_ioctl(), so we have to catch functions returning -ENOIOCTLCMD. > >> Good catch! >> >> At the recent git history, the return for -ENOIOCTLCMD were modified >> by this changeset: >> >> commit b19dd42faf413b4705d4adb38521e82d73fa4249 >> Author: Arnd Bergmann <arnd@arndb.de> >> Date: Sun Jul 4 00:15:10 2010 +0200 >> >> bkl: Remove locked .ioctl file operation >> ... >> @@ -39,21 +38,12 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, >> { >> int error = -ENOTTY; >> >> - if (!filp->f_op) >> + if (!filp->f_op || !filp->f_op->unlocked_ioctl) >> goto out; >> >> - if (filp->f_op->unlocked_ioctl) { >> - error = filp->f_op->unlocked_ioctl(filp, cmd, arg); >> - if (error == -ENOIOCTLCMD) >> - error = -EINVAL; >> - goto out; >> - } else if (filp->f_op->ioctl) { >> - lock_kernel(); >> - error = filp->f_op->ioctl(filp->f_path.dentry->d_inode, >> - filp, cmd, arg); >> - unlock_kernel(); >> ... >> >> Before Arnd's patch, locked ioctl's were returning -ENOIOCTLCMD, and >> unlocked ones were returning -EINVAL. Now, the return of -ENOIOCTLCMD >> doesn't go to userspace anymore. IMO, that's wrong and can cause >> regressions, as some subsystems like DVB were returning -ENOIOCTLCMD >> to userspace. > > ENOIOCTLCMD should never be returned to user space, see the comment > in include/linux/errno.h: > > /* > * These should never be seen by user programs. To return one of ERESTART* > * codes, signal_pending() MUST be set. Note that ptrace can observe these > * at syscall exit tracing, but they will never be left for the debugged user > * process to see. > */ > Ah, ok. > There was a lot of debate whether undefined ioctls on non-ttys should > return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to > -EINVAL at some point in the pre-git era, IIRC. > > Inside of v4l2, I believe this is handled by video_usercopy(), which > turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe > where this is not done correctly and we do return ENOIOCTLCMD to > vfs_ioctl? Well, currently, it is returning -EINVAL maybe due to the mass-conversions you've mentioned. The point is that -EINVAL has too many meanings at V4L. It currently can be either that an ioctl is not supported, or that one of the parameters had an invalid parameter. If the userspace can't distinguish between an unimplemented ioctl and an invalid parameter, it can't decide if it needs to fall back to some different methods of handling a V4L device. Maybe the answer would be to return -ENOTTY when an ioctl is not implemented. >> Linus, >> >> what would be the expected behaviour? > > Note that 1da177e is the initial commit to git, Linus did not write that > code, although he might have an opinion. Yes, I know. I'm expecting his comments on it. Mauro. -- 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
On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: > > There was a lot of debate whether undefined ioctls on non-ttys should > > return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to > > -EINVAL at some point in the pre-git era, IIRC. > > > > Inside of v4l2, I believe this is handled by video_usercopy(), which > > turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe > > where this is not done correctly and we do return ENOIOCTLCMD to > > vfs_ioctl? > > Well, currently, it is returning -EINVAL maybe due to the mass-conversions > you've mentioned. I mean what do you return *to* vfs_ioctl from v4l? The conversions must have been long before we introduced compat_ioctl and ENOIOCTLCMD. As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into EINVAL, so changing the vfs functions would not have any effect. > The point is that -EINVAL has too many meanings at V4L. It currently can be > either that an ioctl is not supported, or that one of the parameters had > an invalid parameter. If the userspace can't distinguish between an unimplemented > ioctl and an invalid parameter, it can't decide if it needs to fall back to > some different methods of handling a V4L device. > > Maybe the answer would be to return -ENOTTY when an ioctl is not implemented. That is what a lot of subsystems do these days. But wouldn't that change your ABI? Arnd -- 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
Em 26-06-2011 15:20, Arnd Bergmann escreveu: > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: >>> There was a lot of debate whether undefined ioctls on non-ttys should >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to >>> -EINVAL at some point in the pre-git era, IIRC. >>> >>> Inside of v4l2, I believe this is handled by video_usercopy(), which >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe >>> where this is not done correctly and we do return ENOIOCTLCMD to >>> vfs_ioctl? >> >> Well, currently, it is returning -EINVAL maybe due to the mass-conversions >> you've mentioned. > > I mean what do you return *to* vfs_ioctl from v4l? The conversions must > have been long before we introduced compat_ioctl and ENOIOCTLCMD. > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into > EINVAL, so changing the vfs functions would not have any effect. Yes. This discussion was originated by a RFC patch proposing to change video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. >> The point is that -EINVAL has too many meanings at V4L. It currently can be >> either that an ioctl is not supported, or that one of the parameters had >> an invalid parameter. If the userspace can't distinguish between an unimplemented >> ioctl and an invalid parameter, it can't decide if it needs to fall back to >> some different methods of handling a V4L device. >> >> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented. > > That is what a lot of subsystems do these days. But wouldn't that change > your ABI? Yes. The patch in question is also changing the DocBook spec for the ABI. We'll likely need to drop some notes about that at the features-to-be-removed.txt. I don't think that applications are relying at -EINVAL in order to detect if an ioctl is not supported, but before merging such patch, we need to double-check. Mauro. -- 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
On Sunday 26 June 2011 20:51:37 Mauro Carvalho Chehab wrote: > > > > I mean what do you return to vfs_ioctl from v4l? The conversions must > > have been long before we introduced compat_ioctl and ENOIOCTLCMD. > > > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into > > EINVAL, so changing the vfs functions would not have any effect. > > Yes. This discussion was originated by a RFC patch proposing to change > video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. Ok, I see. So returning -ENOIOCTLCMD is not an option IMHO, but if you are confident that it doesn't break anything, returning -ENOTTY would be possible and doesn't require any core changes. Arnd -- 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
On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote: > Em 26-06-2011 15:20, Arnd Bergmann escreveu: > > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: > >>> There was a lot of debate whether undefined ioctls on non-ttys should > >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to > >>> -EINVAL at some point in the pre-git era, IIRC. > >>> > >>> Inside of v4l2, I believe this is handled by video_usercopy(), which > >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe > >>> where this is not done correctly and we do return ENOIOCTLCMD to > >>> vfs_ioctl? > >> > >> Well, currently, it is returning -EINVAL maybe due to the mass-conversions > >> you've mentioned. > > > > I mean what do you return *to* vfs_ioctl from v4l? The conversions must > > have been long before we introduced compat_ioctl and ENOIOCTLCMD. > > > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into > > EINVAL, so changing the vfs functions would not have any effect. > > Yes. This discussion was originated by a RFC patch proposing to change > video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. > > >> The point is that -EINVAL has too many meanings at V4L. It currently can be > >> either that an ioctl is not supported, or that one of the parameters had > >> an invalid parameter. If the userspace can't distinguish between an unimplemented > >> ioctl and an invalid parameter, it can't decide if it needs to fall back to > >> some different methods of handling a V4L device. > >> > >> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented. > > > > That is what a lot of subsystems do these days. But wouldn't that change > > your ABI? > > Yes. The patch in question is also changing the DocBook spec for the ABI. We'll > likely need to drop some notes about that at the features-to-be-removed.txt. > > I don't think that applications are relying at -EINVAL in order to detect if > an ioctl is not supported, but before merging such patch, we need to double-check. I really don't think we can change this behavior. It's been part of the spec since forever and it is not just open source apps that can rely on this, but also closed source. Making an ABI change like this can really mess up applications. We should instead review the spec and ensure that applications can discover what is and what isn't supported through e.g. capabilities. Regards, Hans -- 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 Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote: > On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote: > > Em 26-06-2011 15:20, Arnd Bergmann escreveu: > > > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: > > >>> There was a lot of debate whether undefined ioctls on non-ttys should > > >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to > > >>> -EINVAL at some point in the pre-git era, IIRC. > > >>> > > >>> Inside of v4l2, I believe this is handled by video_usercopy(), which > > >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe > > >>> where this is not done correctly and we do return ENOIOCTLCMD to > > >>> vfs_ioctl? > > >> > > >> Well, currently, it is returning -EINVAL maybe due to the mass-conversions > > >> you've mentioned. > > > > > > I mean what do you return *to* vfs_ioctl from v4l? The conversions must > > > have been long before we introduced compat_ioctl and ENOIOCTLCMD. > > > > > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into > > > EINVAL, so changing the vfs functions would not have any effect. > > > > Yes. This discussion was originated by a RFC patch proposing to change > > video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. > > > > >> The point is that -EINVAL has too many meanings at V4L. It currently can be > > >> either that an ioctl is not supported, or that one of the parameters had > > >> an invalid parameter. If the userspace can't distinguish between an unimplemented > > >> ioctl and an invalid parameter, it can't decide if it needs to fall back to > > >> some different methods of handling a V4L device. > > >> > > >> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented. > > > > > > That is what a lot of subsystems do these days. But wouldn't that change > > > your ABI? > > > > Yes. The patch in question is also changing the DocBook spec for the ABI. We'll > > likely need to drop some notes about that at the features-to-be-removed.txt. > > > > I don't think that applications are relying at -EINVAL in order to detect if > > an ioctl is not supported, but before merging such patch, we need to double-check. > > I really don't think we can change this behavior. It's been part of the spec since > forever and it is not just open source apps that can rely on this, but also closed > source. Making an ABI change like this can really mess up applications. > > We should instead review the spec and ensure that applications can discover what > is and what isn't supported through e.g. capabilities. As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY to tell that an ioctl doesn't exist; there are others. And many switched from EINVAL they used in the past. From that point it would be good to do it on V4L2 as well. Although I have to reckon that the V4L2 API does serve use cases of quite different natures than these --- I can't think of an equivalent e.g. to that astronomy application using V4L1 in the scope of these: Examples: - Networking - KVM - SCSI/libata-scsi Currently EINVAL is used to signal from a phletora of conditions in V4L2, usually bad, in a way or another, parameters to an ioctl. The more low level APIs we add (for cameras, for example), the less guessing of parameters can be done in general. I think it would be important to distinguish the two cases and we don't have enumeration capability (do we?) to tell which IOCTLs the application should be expect to be able to use. Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return ENOTTY *right now* when the IOCTL handler is not defined. Have we heard about this up to now? :-) As you mention, switching to ENOTTY in general would change the ABI which would potentially break applications. Can this be handled in a way or another? My understanding is that not many applications would rely on EINVAL telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its attempt to figure out what kind of image sizes the device supports. Fixing this would be a very small change. In short, I think it would be beneficial to switch to ENOTTY in the long run even if it causes some momentary pain. Kind regards,
> Hi Hans, > > On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote: >> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote: >> > Em 26-06-2011 15:20, Arnd Bergmann escreveu: >> > > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: >> > >>> There was a lot of debate whether undefined ioctls on non-ttys >> should >> > >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY >> to >> > >>> -EINVAL at some point in the pre-git era, IIRC. >> > >>> >> > >>> Inside of v4l2, I believe this is handled by video_usercopy(), >> which >> > >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you >> observe >> > >>> where this is not done correctly and we do return ENOIOCTLCMD to >> > >>> vfs_ioctl? >> > >> >> > >> Well, currently, it is returning -EINVAL maybe due to the >> mass-conversions >> > >> you've mentioned. >> > > >> > > I mean what do you return *to* vfs_ioctl from v4l? The conversions >> must >> > > have been long before we introduced compat_ioctl and ENOIOCTLCMD. >> > > >> > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD >> into >> > > EINVAL, so changing the vfs functions would not have any effect. >> > >> > Yes. This discussion was originated by a RFC patch proposing to >> change >> > video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. >> > >> > >> The point is that -EINVAL has too many meanings at V4L. It >> currently can be >> > >> either that an ioctl is not supported, or that one of the >> parameters had >> > >> an invalid parameter. If the userspace can't distinguish between an >> unimplemented >> > >> ioctl and an invalid parameter, it can't decide if it needs to fall >> back to >> > >> some different methods of handling a V4L device. >> > >> >> > >> Maybe the answer would be to return -ENOTTY when an ioctl is not >> implemented. >> > > >> > > That is what a lot of subsystems do these days. But wouldn't that >> change >> > > your ABI? >> > >> > Yes. The patch in question is also changing the DocBook spec for the >> ABI. We'll >> > likely need to drop some notes about that at the >> features-to-be-removed.txt. >> > >> > I don't think that applications are relying at -EINVAL in order to >> detect if >> > an ioctl is not supported, but before merging such patch, we need to >> double-check. >> >> I really don't think we can change this behavior. It's been part of the >> spec since >> forever and it is not just open source apps that can rely on this, but >> also closed >> source. Making an ABI change like this can really mess up applications. >> >> We should instead review the spec and ensure that applications can >> discover what >> is and what isn't supported through e.g. capabilities. > > As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY > to tell that an ioctl doesn't exist; there are others. And many switched > from EINVAL they used in the past. From that point it would be good to do > it > on V4L2 as well. Although I have to reckon that the V4L2 API does serve > use > cases of quite different natures than these --- I can't think of an > equivalent e.g. to that astronomy application using V4L1 in the scope of > these: > > Examples: > - Networking > - KVM > - SCSI/libata-scsi > > Currently EINVAL is used to signal from a phletora of conditions in V4L2, > usually bad, in a way or another, parameters to an ioctl. The more low > level > APIs we add (for cameras, for example), the less guessing of parameters > can > be done in general. I think it would be important to distinguish the two > cases and we don't have enumeration capability (do we?) to tell which > IOCTLs > the application should be expect to be able to use. While we don't have an enum capability, in many cases you can deduce whether a particular ioctl should be supported or not. Usually based on capabilities, sometimes because certain ioctls allow 'NOP' operations that allow you to test for their presence. Of course, drivers are not always consistent here, but that's a separate problem. > Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return > ENOTTY *right now* when the IOCTL handler is not defined. Have we heard > about this up to now? :-) No, but that's because all drivers have an ioctl handler :-) So you never see ENOTTY. > As you mention, switching to ENOTTY in general would change the ABI which > would potentially break applications. Can this be handled in a way or > another? My understanding is that not many applications would rely on > EINVAL > telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its > attempt to figure out what kind of image sizes the device supports. Fixing > this would be a very small change. > > In short, I think it would be beneficial to switch to ENOTTY in the long > run even if it causes some momentary pain. I would like that as well, but the V4L2 Specification explicitly mentions EINVAL as the error code if an ioctl is not supported. It has done so since it was created. You cannot just change that. And closed source programs may very well rely on this. I don't think changing such an important return value is acceptable. A better approach would be to allow applications to deduce whether ioctls are (should be) present or not based on capabilities, etc. And document that in the spec and ensure that drivers do this right. The v4l2-compliance tool is already checking that where possible. Regards, Hans -- 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
Em 27-06-2011 09:17, Hans Verkuil escreveu: >> Hi Hans, >> >> On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote: >>> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote: >>>> Em 26-06-2011 15:20, Arnd Bergmann escreveu: >>>>> On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: >>>>>>> There was a lot of debate whether undefined ioctls on non-ttys >>> should >>>>>>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY >>> to >>>>>>> -EINVAL at some point in the pre-git era, IIRC. >>>>>>> >>>>>>> Inside of v4l2, I believe this is handled by video_usercopy(), >>> which >>>>>>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you >>> observe >>>>>>> where this is not done correctly and we do return ENOIOCTLCMD to >>>>>>> vfs_ioctl? >>>>>> >>>>>> Well, currently, it is returning -EINVAL maybe due to the >>> mass-conversions >>>>>> you've mentioned. >>>>> >>>>> I mean what do you return *to* vfs_ioctl from v4l? The conversions >>> must >>>>> have been long before we introduced compat_ioctl and ENOIOCTLCMD. >>>>> >>>>> As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD >>> into >>>>> EINVAL, so changing the vfs functions would not have any effect. >>>> >>>> Yes. This discussion was originated by a RFC patch proposing to >>> change >>>> video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. >>>> >>>>>> The point is that -EINVAL has too many meanings at V4L. It >>> currently can be >>>>>> either that an ioctl is not supported, or that one of the >>> parameters had >>>>>> an invalid parameter. If the userspace can't distinguish between an >>> unimplemented >>>>>> ioctl and an invalid parameter, it can't decide if it needs to fall >>> back to >>>>>> some different methods of handling a V4L device. >>>>>> >>>>>> Maybe the answer would be to return -ENOTTY when an ioctl is not >>> implemented. >>>>> >>>>> That is what a lot of subsystems do these days. But wouldn't that >>> change >>>>> your ABI? >>>> >>>> Yes. The patch in question is also changing the DocBook spec for the >>> ABI. We'll >>>> likely need to drop some notes about that at the >>> features-to-be-removed.txt. >>>> >>>> I don't think that applications are relying at -EINVAL in order to >>> detect if >>>> an ioctl is not supported, but before merging such patch, we need to >>> double-check. >>> >>> I really don't think we can change this behavior. It's been part of the >>> spec since >>> forever and it is not just open source apps that can rely on this, but >>> also closed >>> source. Making an ABI change like this can really mess up applications. >>> >>> We should instead review the spec and ensure that applications can >>> discover what >>> is and what isn't supported through e.g. capabilities. >> >> As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY >> to tell that an ioctl doesn't exist; there are others. And many switched >> from EINVAL they used in the past. From that point it would be good to do >> it >> on V4L2 as well. Although I have to reckon that the V4L2 API does serve >> use >> cases of quite different natures than these --- I can't think of an >> equivalent e.g. to that astronomy application using V4L1 in the scope of >> these: >> >> Examples: >> - Networking >> - KVM >> - SCSI/libata-scsi >> >> Currently EINVAL is used to signal from a phletora of conditions in V4L2, >> usually bad, in a way or another, parameters to an ioctl. The more low >> level >> APIs we add (for cameras, for example), the less guessing of parameters >> can >> be done in general. I think it would be important to distinguish the two >> cases and we don't have enumeration capability (do we?) to tell which >> IOCTLs >> the application should be expect to be able to use. > > While we don't have an enum capability, in many cases you can deduce > whether a particular ioctl should be supported or not. Usually based on > capabilities, sometimes because certain ioctls allow 'NOP' operations that > allow you to test for their presence. > > Of course, drivers are not always consistent here, but that's a separate > problem. Any "hint" code that would try to do some NOP operations may fail. One of the reasons is that such hint is not documented. Yet, I don't officially support such "hint" methods at the API. >> Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return >> ENOTTY *right now* when the IOCTL handler is not defined. Have we heard >> about this up to now? :-) > > No, but that's because all drivers have an ioctl handler :-) So you never > see ENOTTY. Well, a V4L1 call now returns -ENOTTY, with the current behaviour. Btw, there are two drivers returning -ENOTTY, when the device got disconnected (or firmware were not uploaded). The truth is that the current API specs for return code is bogus. > >> As you mention, switching to ENOTTY in general would change the ABI which >> would potentially break applications. Can this be handled in a way or >> another? My understanding is that not many applications would rely on >> EINVAL >> telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its >> attempt to figure out what kind of image sizes the device supports. Fixing >> this would be a very small change. >> >> In short, I think it would be beneficial to switch to ENOTTY in the long >> run even if it causes some momentary pain. > > I would like that as well, but the V4L2 Specification explicitly mentions > EINVAL as the error code if an ioctl is not supported. It has done so > since it was created. You cannot just change that. And closed source > programs may very well rely on this. The V4L2 spec needs to be fixed with respect to error codes. Driver authors are much more creative than DocBook authors ;) There are a lot of return codes used by the drivers whose API spec doesn't mention (and, on this subject, the same applies to the DVB API). What I've seen is that: - Sometimes, a core return code is returned. One of the important examples is the ENOSPC error returned when the usb core refuses to stream when the USB bus reached 80% of the available bandwidth. There's a patch floating around that would allow to override the 80% hard limit, via sysfs. So, if properly documented, an userspace application could give a hint that the user needs to either use a different bus or try to change the hard limit; - For every ioctl, it presents its own "private" list of error codes. If someone wants to add a new code (for example, standardizing ENOTTY or ENOSPC), all affected ioctl's will need to be touched. This is hard to maintain; - Drivers are not compliant with error codes. The right thing to do is to create a separate chapter for error codes, based on errno(3) man page, where we document all error codes that should be used by the drivers. Then, at the ioctl pages, link to the common chapter and, only when needed, document special cases where an error code for that specific ioctl has some special meaning. I ran a script here to check how many different error codes are used inside drivers/media: $ find drivers/media -type f -name '*.[ch]' >files $ grep define `find . -name errno*.h`|perl -ne 'print "$1\n" if (/\#define\s+(E[^\s]+)/)'|sort|uniq >errors $ for i in `cat errors`; do COUNT=$(git grep -c $i `cat files`|wc -l); if [ "$COUNT" != "0" ]; then echo $i $COUNT; fi; done The result is that we're using 53 different types of errors, but the API specs documents only 17 of them. Those are the currently used errors at drivers/media: ERROR CODE |NUMBER OF *.c/*.h FILES USING IT ---------------|-------------------------------- E2BIG 1 EACCES 8 EAGAIN 66 EBADF 1 EBADFD 1 EBADR 2 EBADRQC 2 EBUSY 149 ECHILD 1 ECONNRESET 25 EDEADLK 1 EDOM 1 EEXIST 3 EFAULT 230 EFBIG 1 EILSEQ 8 EINIT 2 EINPROGRESS 6 EINTR 21 EINVAL 501 EIO 305 EMFILE 1 ENFILE 7 ENOBUFS 7 ENODATA 4 ENODEV 270 ENOENT 46 ENOIOCTLCMD 31 ENOMEM 359 ENOSPC 13 ENOSR 7 ENOSYS 15 ENOTSUP 3 ENOTSUPP 3 ENOTTY 5 ENXIO 26 EOPNOTSUPP 19 EOVERFLOW 14 EPERM 47 EPIPE 12 EPROTO 11 ERANGE 25 EREMOTE 80 EREMOTEIO 80 ERESTART 32 ERESTARTSYS 32 ESHUTDOWN 27 ESPIPE 3 ETIME 53 ETIMEDOUT 37 EUSERS 2 EWOULDBLOCK 14 EXDEV 1 I suspect that we'll need to both fix some drivers, and the API, as I bet that the same error conditions are reported differently on different drivers. > I don't think changing such an important return value is acceptable. As I said, the current API is bogus with respect to error codes. Of course, we need to do take care to avoid userspace applications breakage, but we can't use the excuse that it is there for a long time as a reason for not fixing it. > A better approach would be to allow applications to deduce whether ioctls > are (should be) present or not based on capabilities, etc. And document > that in the spec and ensure that drivers do this right. > > The v4l2-compliance tool is already checking that where possible. > > Regards, > > Hans > Thanks, Mauro -- 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
On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote: > Em 27-06-2011 09:17, Hans Verkuil escreveu: > >> Hi Hans, > >> > >> On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote: > >>> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote: > >>>> Em 26-06-2011 15:20, Arnd Bergmann escreveu: > >>>>> On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote: > >>>>>>> There was a lot of debate whether undefined ioctls on non-ttys > >>> should > >>>>>>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY > >>> to > >>>>>>> -EINVAL at some point in the pre-git era, IIRC. > >>>>>>> > >>>>>>> Inside of v4l2, I believe this is handled by video_usercopy(), > >>> which > >>>>>>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you > >>> observe > >>>>>>> where this is not done correctly and we do return ENOIOCTLCMD to > >>>>>>> vfs_ioctl? > >>>>>> > >>>>>> Well, currently, it is returning -EINVAL maybe due to the > >>> mass-conversions > >>>>>> you've mentioned. > >>>>> > >>>>> I mean what do you return *to* vfs_ioctl from v4l? The conversions > >>> must > >>>>> have been long before we introduced compat_ioctl and ENOIOCTLCMD. > >>>>> > >>>>> As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD > >>> into > >>>>> EINVAL, so changing the vfs functions would not have any effect. > >>>> > >>>> Yes. This discussion was originated by a RFC patch proposing to > >>> change > >>>> video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL. > >>>> > >>>>>> The point is that -EINVAL has too many meanings at V4L. It > >>> currently can be > >>>>>> either that an ioctl is not supported, or that one of the > >>> parameters had > >>>>>> an invalid parameter. If the userspace can't distinguish between an > >>> unimplemented > >>>>>> ioctl and an invalid parameter, it can't decide if it needs to fall > >>> back to > >>>>>> some different methods of handling a V4L device. > >>>>>> > >>>>>> Maybe the answer would be to return -ENOTTY when an ioctl is not > >>> implemented. > >>>>> > >>>>> That is what a lot of subsystems do these days. But wouldn't that > >>> change > >>>>> your ABI? > >>>> > >>>> Yes. The patch in question is also changing the DocBook spec for the > >>> ABI. We'll > >>>> likely need to drop some notes about that at the > >>> features-to-be-removed.txt. > >>>> > >>>> I don't think that applications are relying at -EINVAL in order to > >>> detect if > >>>> an ioctl is not supported, but before merging such patch, we need to > >>> double-check. > >>> > >>> I really don't think we can change this behavior. It's been part of the > >>> spec since > >>> forever and it is not just open source apps that can rely on this, but > >>> also closed > >>> source. Making an ABI change like this can really mess up applications. > >>> > >>> We should instead review the spec and ensure that applications can > >>> discover what > >>> is and what isn't supported through e.g. capabilities. > >> > >> As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY > >> to tell that an ioctl doesn't exist; there are others. And many switched > >> from EINVAL they used in the past. From that point it would be good to do > >> it > >> on V4L2 as well. Although I have to reckon that the V4L2 API does serve > >> use > >> cases of quite different natures than these --- I can't think of an > >> equivalent e.g. to that astronomy application using V4L1 in the scope of > >> these: > >> > >> Examples: > >> - Networking > >> - KVM > >> - SCSI/libata-scsi > >> > >> Currently EINVAL is used to signal from a phletora of conditions in V4L2, > >> usually bad, in a way or another, parameters to an ioctl. The more low > >> level > >> APIs we add (for cameras, for example), the less guessing of parameters > >> can > >> be done in general. I think it would be important to distinguish the two > >> cases and we don't have enumeration capability (do we?) to tell which > >> IOCTLs > >> the application should be expect to be able to use. > > > > While we don't have an enum capability, in many cases you can deduce > > whether a particular ioctl should be supported or not. Usually based on > > capabilities, sometimes because certain ioctls allow 'NOP' operations that > > allow you to test for their presence. > > > > Of course, drivers are not always consistent here, but that's a separate > > problem. > > Any "hint" code that would try to do some NOP operations may fail. One of the > reasons is that such hint is not documented. Yet, I don't officially support > such "hint" methods at the API. The point is that the spec can easily be improved to make such 'NOP' operations explicit, or to require that if a capability is present, then the corresponding ioctl(s) must also be present. Things like that are easy to verify as well with v4l2-compliance. > >> Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return > >> ENOTTY *right now* when the IOCTL handler is not defined. Have we heard > >> about this up to now? :-) > > > > No, but that's because all drivers have an ioctl handler :-) So you never > > see ENOTTY. > > Well, a V4L1 call now returns -ENOTTY, with the current behaviour. How? I don't see that. All drivers have an ioctl handler, so all ioctls will go through that handler, which returns -EINVAL for V4L1 ioctls. > Btw, there are two drivers returning -ENOTTY, when the device got disconnected > (or firmware were not uploaded). > > The truth is that the current API specs for return code is bogus. Bogus in what way? It's been documented very clearly for years. We may not like that design decision (I certainly don't like it), but someone clearly thought about it at the time. You know that I am usually more than willing to make/accept ABI changes, especially in ambiguous circumstances (and we have a lot of those). But this particular one is actually for once consistently implemented in all drivers and apps can (and probably do) rely on them. And that includes also closed source applications, and there is no way we can change those. Neither is this something that only affects some niche products, this is subsystem-wide. > > > > >> As you mention, switching to ENOTTY in general would change the ABI which > >> would potentially break applications. Can this be handled in a way or > >> another? My understanding is that not many applications would rely on > >> EINVAL > >> telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its > >> attempt to figure out what kind of image sizes the device supports. Fixing > >> this would be a very small change. > >> > >> In short, I think it would be beneficial to switch to ENOTTY in the long > >> run even if it causes some momentary pain. > > > > I would like that as well, but the V4L2 Specification explicitly mentions > > EINVAL as the error code if an ioctl is not supported. It has done so > > since it was created. You cannot just change that. And closed source > > programs may very well rely on this. > > The V4L2 spec needs to be fixed with respect to error codes. Driver authors > are much more creative than DocBook authors ;) There are a lot of return > codes used by the drivers whose API spec doesn't mention (and, on this subject, > the same applies to the DVB API). What I've seen is that: > - Sometimes, a core return code is returned. One of the important examples is > the ENOSPC error returned when the usb core refuses to stream when the USB > bus reached 80% of the available bandwidth. There's a patch floating around that > would allow to override the 80% hard limit, via sysfs. So, if properly documented, > an userspace application could give a hint that the user needs to either use a > different bus or try to change the hard limit; > - For every ioctl, it presents its own "private" list of error codes. If someone wants > to add a new code (for example, standardizing ENOTTY or ENOSPC), all affected > ioctl's will need to be touched. This is hard to maintain; > - Drivers are not compliant with error codes. > > The right thing to do is to create a separate chapter for error codes, based on errno(3) > man page, where we document all error codes that should be used by the drivers. Then, > at the ioctl pages, link to the common chapter and, only when needed, document special > cases where an error code for that specific ioctl has some special meaning. Great, I've no problem with that. But this particular error code you want to change is actually implemented *consistently* in all drivers. There is no confusion, no ambiguity, and it is according to the spec. > I ran a script here to check how many different error codes are used inside drivers/media: > > $ find drivers/media -type f -name '*.[ch]' >files > $ grep define `find . -name errno*.h`|perl -ne 'print "$1\n" if (/\#define\s+(E[^\s]+)/)'|sort|uniq >errors > $ for i in `cat errors`; do COUNT=$(git grep -c $i `cat files`|wc -l); if [ "$COUNT" != "0" ]; then echo $i $COUNT; fi; done > > The result is that we're using 53 different types of errors, but the API specs documents > only 17 of them. Those are the currently used errors at drivers/media: > > ERROR CODE |NUMBER OF *.c/*.h FILES USING IT > ---------------|-------------------------------- > E2BIG 1 > EACCES 8 > EAGAIN 66 > EBADF 1 > EBADFD 1 > EBADR 2 > EBADRQC 2 > EBUSY 149 > ECHILD 1 > ECONNRESET 25 > EDEADLK 1 > EDOM 1 > EEXIST 3 > EFAULT 230 > EFBIG 1 > EILSEQ 8 > EINIT 2 > EINPROGRESS 6 > EINTR 21 > EINVAL 501 > EIO 305 > EMFILE 1 > ENFILE 7 > ENOBUFS 7 > ENODATA 4 > ENODEV 270 > ENOENT 46 > ENOIOCTLCMD 31 > ENOMEM 359 > ENOSPC 13 > ENOSR 7 > ENOSYS 15 > ENOTSUP 3 > ENOTSUPP 3 > ENOTTY 5 > ENXIO 26 > EOPNOTSUPP 19 > EOVERFLOW 14 > EPERM 47 > EPIPE 12 > EPROTO 11 > ERANGE 25 > EREMOTE 80 > EREMOTEIO 80 > ERESTART 32 > ERESTARTSYS 32 > ESHUTDOWN 27 > ESPIPE 3 > ETIME 53 > ETIMEDOUT 37 > EUSERS 2 > EWOULDBLOCK 14 > EXDEV 1 > > I suspect that we'll need to both fix some drivers, and the API, as I bet that > the same error conditions are reported differently on different drivers. > > > I don't think changing such an important return value is acceptable. > > As I said, the current API is bogus with respect to error codes. Of course, > we need to do take care to avoid userspace applications breakage, but we can't > use the excuse that it is there for a long time as a reason for not fixing it. The fact that many drivers use error codes creatively doesn't give us an excuse to just change the one error code that is actually used everywhere according to the spec! That's faulty logic. Regards, Hans -- 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
On Mon, 2011-06-27 at 10:54 -0300, Mauro Carvalho Chehab wrote: > The right thing to do is to create a separate chapter for error codes, based on errno(3) > man page [snip] IMO, the IEEE Std 1003.1 is a better source for errno definitions than the errno(3) manpage: http://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_03.html Regards, Andy -- 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
Em 27-06-2011 12:12, Andy Walls escreveu: > On Mon, 2011-06-27 at 10:54 -0300, Mauro Carvalho Chehab wrote: > >> The right thing to do is to create a separate chapter for error codes, based on errno(3) >> man page [snip] > > IMO, the IEEE Std 1003.1 is a better source for errno definitions than > the errno(3) manpage: > > http://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_03.html True, but it doesn't cover all error types. For sure we need to use the POSIX standard when working at the DocBook. Cheers, Mauro -- 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
Em 27-06-2011 11:56, Hans Verkuil escreveu: > On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote: >> Em 27-06-2011 09:17, Hans Verkuil escreveu: >>> While we don't have an enum capability, in many cases you can deduce >>> whether a particular ioctl should be supported or not. Usually based on >>> capabilities, sometimes because certain ioctls allow 'NOP' operations that >>> allow you to test for their presence. >>> >>> Of course, drivers are not always consistent here, but that's a separate >>> problem. >> >> Any "hint" code that would try to do some NOP operations may fail. One of the >> reasons is that such hint is not documented. Yet, I don't officially support >> such "hint" methods at the API. > > The point is that the spec can easily be improved to make such 'NOP' operations > explicit, or to require that if a capability is present, then the corresponding > ioctl(s) must also be present. Things like that are easy to verify as well with > v4l2-compliance. We currently have more than 64 ioctl's. Adding a capability bit for each doesn't seem the right thing to do. Ok, some could be grouped, but, even so, there are drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S. So, I think we don't have enough available bits for doing that. >> Btw, there are two drivers returning -ENOTTY, when the device got disconnected >> (or firmware were not uploaded). >> >> The truth is that the current API specs for return code is bogus. > > Bogus in what way? It's been documented very clearly for years. We may not like > that design decision (I certainly don't like it), but someone clearly thought > about it at the time. Bogus in the sense that drivers don't follow them, as they're returning undocumented values. Any application strictly following it will have troubles. >> The right thing to do is to create a separate chapter for error codes, based on errno(3) >> man page, where we document all error codes that should be used by the drivers. Then, >> at the ioctl pages, link to the common chapter and, only when needed, document special >> cases where an error code for that specific ioctl has some special meaning. > > Great, I've no problem with that. But this particular error code you want to change > is actually implemented *consistently* in all drivers. There is no confusion, no > ambiguity, and it is according to the spec. As I said, from userspace perspective, it is not consistent to assume that EINVAL means not implemented. For sure at VIDIOC_S_foo, this is not consistent. Even on some GET types of ioctl, like for example [1][2], there are other reasons for an EINVAL return. [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-cropcap.html [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-audio.html The only way to make it consistent is to use different return codes for "invalid parameters" and for "unsupported ioctl". >> I ran a script here to check how many different error codes are used inside drivers/media: >> >> $ find drivers/media -type f -name '*.[ch]' >files >> $ grep define `find . -name errno*.h`|perl -ne 'print "$1\n" if (/\#define\s+(E[^\s]+)/)'|sort|uniq >errors >> $ for i in `cat errors`; do COUNT=$(git grep -c $i `cat files`|wc -l); if [ "$COUNT" != "0" ]; then echo $i $COUNT; fi; done >> >> The result is that we're using 53 different types of errors, but the API specs documents >> only 17 of them. Those are the currently used errors at drivers/media: >> >> ERROR CODE |NUMBER OF *.c/*.h FILES USING IT >> ---------------|-------------------------------- >> E2BIG 1 >> EACCES 8 >> EAGAIN 66 >> EBADF 1 >> EBADFD 1 >> EBADR 2 >> EBADRQC 2 >> EBUSY 149 >> ECHILD 1 >> ECONNRESET 25 >> EDEADLK 1 >> EDOM 1 >> EEXIST 3 >> EFAULT 230 >> EFBIG 1 >> EILSEQ 8 >> EINIT 2 >> EINPROGRESS 6 >> EINTR 21 >> EINVAL 501 >> EIO 305 >> EMFILE 1 >> ENFILE 7 >> ENOBUFS 7 >> ENODATA 4 >> ENODEV 270 >> ENOENT 46 >> ENOIOCTLCMD 31 >> ENOMEM 359 >> ENOSPC 13 >> ENOSR 7 >> ENOSYS 15 >> ENOTSUP 3 >> ENOTSUPP 3 >> ENOTTY 5 >> ENXIO 26 >> EOPNOTSUPP 19 >> EOVERFLOW 14 >> EPERM 47 >> EPIPE 12 >> EPROTO 11 >> ERANGE 25 >> EREMOTE 80 >> EREMOTEIO 80 >> ERESTART 32 >> ERESTARTSYS 32 >> ESHUTDOWN 27 >> ESPIPE 3 >> ETIME 53 >> ETIMEDOUT 37 >> EUSERS 2 >> EWOULDBLOCK 14 >> EXDEV 1 >> >> I suspect that we'll need to both fix some drivers, and the API, as I bet that >> the same error conditions are reported differently on different drivers. >> >>> I don't think changing such an important return value is acceptable. >> >> As I said, the current API is bogus with respect to error codes. Of course, >> we need to do take care to avoid userspace applications breakage, but we can't >> use the excuse that it is there for a long time as a reason for not fixing it. > > The fact that many drivers use error codes creatively doesn't give us an excuse > to just change the one error code that is actually used everywhere according to > the spec! That's faulty logic. The fix that it is needed is to provide a consistent way for an userspace application to know for sure when an ioctl is not supported. It can be done on a simple way of just returning a different error code for it, or with complex mechanisms like adding a per-ioctl flag and some hint logics based on NOP. The V4L2 is complex enough for us to add more complexity with hints and cap flags. Thanks, Mauro -- 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
On Monday 27 June 2011, Mauro Carvalho Chehab wrote: > > The point is that the spec can easily be improved to make such 'NOP' operations > > explicit, or to require that if a capability is present, then the corresponding > > ioctl(s) must also be present. Things like that are easy to verify as well with > > v4l2-compliance. > > We currently have more than 64 ioctl's. Adding a capability bit for each doesn't > seem the right thing to do. Ok, some could be grouped, but, even so, there are > drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S. > So, I think we don't have enough available bits for doing that. It shouldn't be too hard to do an ioctl command that returns a le_bitmask with the ioctl command number as an index (0 to 91, currently), and the bit set for each command that has the corresponding v4l2_ioctl_ops member filled for the device. That would be an obvious way to query the operations, but I don't know if it's useful. Arnd -- 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
Em 27-06-2011 13:14, Arnd Bergmann escreveu: > On Monday 27 June 2011, Mauro Carvalho Chehab wrote: >>> The point is that the spec can easily be improved to make such 'NOP' operations >>> explicit, or to require that if a capability is present, then the corresponding >>> ioctl(s) must also be present. Things like that are easy to verify as well with >>> v4l2-compliance. >> >> We currently have more than 64 ioctl's. Adding a capability bit for each doesn't >> seem the right thing to do. Ok, some could be grouped, but, even so, there are >> drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S. >> So, I think we don't have enough available bits for doing that. > > It shouldn't be too hard to do an ioctl command that returns a le_bitmask with the > ioctl command number as an index (0 to 91, currently), and the bit set for each > command that has the corresponding v4l2_ioctl_ops member filled for the device. > That would be an obvious way to query the operations, but I don't know if it's > useful. Tricks like that could be done, but that would not be consistent with other subsystems that use -ENOTTY for such purpose. Also, to avoid having magic numbers, this will end by adding a somewhat complex logic at userspace (or having some sort of macro exported together with videodev2.h), in order to associate a V4L2 ioctl with the corresponding le_bitmask bit. Also, 91 ioctls seems a complex-enough API to me. We should avoid adding more stuff there without very good reasons. In any case, we should also double check how this is done by the other API's used at the media subsystem, and try to do the same there (lirc, Media Controller and DVB APIs). My intention is to split the error handling changes from the original series that remove the linux/version.h, and spend some time preparing a new RFC about that, trying to cover all the API's defined under drivers/media. One idea could be to postpone a decision about it, and discuss this topic further during the media workshop at the KS/2011. Thanks, Mauro. -- 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
On Monday, June 27, 2011 17:33:58 Mauro Carvalho Chehab wrote: > Em 27-06-2011 11:56, Hans Verkuil escreveu: > > On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote: > >> Em 27-06-2011 09:17, Hans Verkuil escreveu: > >>> While we don't have an enum capability, in many cases you can deduce > >>> whether a particular ioctl should be supported or not. Usually based on > >>> capabilities, sometimes because certain ioctls allow 'NOP' operations that > >>> allow you to test for their presence. > >>> > >>> Of course, drivers are not always consistent here, but that's a separate > >>> problem. > >> > >> Any "hint" code that would try to do some NOP operations may fail. One of the > >> reasons is that such hint is not documented. Yet, I don't officially support > >> such "hint" methods at the API. > > > > The point is that the spec can easily be improved to make such 'NOP' operations > > explicit, or to require that if a capability is present, then the corresponding > > ioctl(s) must also be present. Things like that are easy to verify as well with > > v4l2-compliance. > > We currently have more than 64 ioctl's. Adding a capability bit for each doesn't > seem the right thing to do. Ok, some could be grouped, but, even so, there are > drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S. > So, I think we don't have enough available bits for doing that. No, that's not what I meant. Whether or not ioctls are implemented can in many cases be deduced from the QUERYCAP capabilities: e.g. if V4L2_CAP_STREAMING is set, then the buffer I/O ioctls have to be there. For other ioctls the test whether they are implemented is also often straightforward: e.g. if VIDIOC_G_INPUT returns -EINVAL, then that can only mean that it isn't implemented, and neither are ENUM_INPUT and S_INPUT. In cases where only the G variant is implemented, there we need to tighten the spec and require that these ioctls are properly implemented: you either implement all of the ENUM/G/TRY/S ioctls or none. Currently ENUM_FRAMESIZES/INTERVALS is one set of ioctls where this is very ambiguous. For most of the others it is pretty straightforward. > >> Btw, there are two drivers returning -ENOTTY, when the device got disconnected > >> (or firmware were not uploaded). > >> > >> The truth is that the current API specs for return code is bogus. > > > > Bogus in what way? It's been documented very clearly for years. We may not like > > that design decision (I certainly don't like it), but someone clearly thought > > about it at the time. > > Bogus in the sense that drivers don't follow them, as they're returning undocumented > values. Any application strictly following it will have troubles. I suspect that in most cases the drivers are fairly reasonable, but the spec wasn't updated with the new error codes. > >> The right thing to do is to create a separate chapter for error codes, based on errno(3) > >> man page, where we document all error codes that should be used by the drivers. Then, > >> at the ioctl pages, link to the common chapter and, only when needed, document special > >> cases where an error code for that specific ioctl has some special meaning. > > > > Great, I've no problem with that. But this particular error code you want to change > > is actually implemented *consistently* in all drivers. There is no confusion, no > > ambiguity, and it is according to the spec. > > As I said, from userspace perspective, it is not consistent to assume that EINVAL means > not implemented. For sure at VIDIOC_S_foo, this is not consistent. Even on some GET types > of ioctl, like for example [1][2], there are other reasons for an EINVAL return. > > [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-cropcap.html > [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-audio.html > > The only way to make it consistent is to use different return codes for "invalid parameters" > and for "unsupported ioctl". No, what we do is perfectly consistent: i.e. we always return EINVAL when an ioctl is not supported. That's what 'consistent' means. Whether that is the *right* error code is something else. But right now our API and our documentation is perfectly consistent and has been for years. <snip> > > The fact that many drivers use error codes creatively doesn't give us an excuse > > to just change the one error code that is actually used everywhere according to > > the spec! That's faulty logic. > > The fix that it is needed is to provide a consistent way for an userspace application > to know for sure when an ioctl is not supported. It can be done on a simple way of > just returning a different error code for it, or with complex mechanisms like adding > a per-ioctl flag and some hint logics based on NOP. It's a 'fix' that I fear may break applications because drivers suddenly change their behavior. You can't just ignore that. You also can't analyze applications, since closed source apps may also use it, and we obviously have no control over those. > The V4L2 is complex enough for us to add more complexity with hints and cap flags. Actually, all it needs for the most part is that current implicit rules are made explicit: if a certain querycap flag is set, then a corresponding set of ioctls must be implemented. If I can call the GET ioctl, then the ENUM and SET (and TRY) must also be implemented. Sensible rules at any time, they just need to be made explicit. If you want to change it to ENOTTY, then go right ahead. But you can explain it to our customers when their app suddenly breaks for some hardware. I don't get it. This is really not a problem. We rarely, if ever, get complaints about it, and there is a ton of other much more important stuff that needs to be done. Regards, Hans -- 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
Em 27-06-2011 14:07, Hans Verkuil escreveu: > On Monday, June 27, 2011 17:33:58 Mauro Carvalho Chehab wrote: >> Em 27-06-2011 11:56, Hans Verkuil escreveu: >>> On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote: >>>> Em 27-06-2011 09:17, Hans Verkuil escreveu: >>>>> While we don't have an enum capability, in many cases you can deduce >>>>> whether a particular ioctl should be supported or not. Usually based on >>>>> capabilities, sometimes because certain ioctls allow 'NOP' operations that >>>>> allow you to test for their presence. >>>>> >>>>> Of course, drivers are not always consistent here, but that's a separate >>>>> problem. >>>> >>>> Any "hint" code that would try to do some NOP operations may fail. One of the >>>> reasons is that such hint is not documented. Yet, I don't officially support >>>> such "hint" methods at the API. >>> >>> The point is that the spec can easily be improved to make such 'NOP' operations >>> explicit, or to require that if a capability is present, then the corresponding >>> ioctl(s) must also be present. Things like that are easy to verify as well with >>> v4l2-compliance. >> >> We currently have more than 64 ioctl's. Adding a capability bit for each doesn't >> seem the right thing to do. Ok, some could be grouped, but, even so, there are >> drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S. >> So, I think we don't have enough available bits for doing that. > > No, that's not what I meant. > > Whether or not ioctls are implemented can in many cases be deduced from the > QUERYCAP capabilities: e.g. if V4L2_CAP_STREAMING is set, then the buffer I/O > ioctls have to be there. No. only the MMAP-based and OVERLAY-based ioctls could be deduced from the flags there, plus hw freq seek. Anything else will be just a guess. > For other ioctls the test whether they are implemented > is also often straightforward: e.g. if VIDIOC_G_INPUT returns -EINVAL, then > that can only mean that it isn't implemented, and neither are ENUM_INPUT and > S_INPUT. At the best, it is a hint coding. > In cases where only the G variant is implemented, there we need to tighten > the spec and require that these ioctls are properly implemented: you either > implement all of the ENUM/G/TRY/S ioctls or none. The V4L1 compat layer required that some VIDIOC_G variants to be implemented for some things to work. This code were moved to userspace, but the requirement is probably still there. In other words, drivers had/have no option but implementing some VIDIOC_G even when VIDIOC_S is not supported. So, changing the V4L2 spec to require that all 3 to be implemented will also cause compatibility issues that will be harder to map than the check for a proper check of the EINVAL return code. > Currently ENUM_FRAMESIZES/INTERVALS is one set of ioctls where this is very > ambiguous. For most of the others it is pretty straightforward. >>>> Btw, there are two drivers returning -ENOTTY, when the device got disconnected >>>> (or firmware were not uploaded). >>>> >>>> The truth is that the current API specs for return code is bogus. >>> >>> Bogus in what way? It's been documented very clearly for years. We may not like >>> that design decision (I certainly don't like it), but someone clearly thought >>> about it at the time. >> >> Bogus in the sense that drivers don't follow them, as they're returning undocumented >> values. Any application strictly following it will have troubles. > > I suspect that in most cases the drivers are fairly reasonable, but the spec > wasn't updated with the new error codes. If driver foo returns ENOTTY because the device got removed and driver bar returns another error, they're not consistent. Developers tried to get the errors that they considered to be the more applicable to that situation, but, as different developers took different decisions, the end result is that several error codes will need to be fixed^Wchanged, in order to allow userspace to properly deal/report with such error conditions. >>>> The right thing to do is to create a separate chapter for error codes, based on errno(3) >>>> man page, where we document all error codes that should be used by the drivers. Then, >>>> at the ioctl pages, link to the common chapter and, only when needed, document special >>>> cases where an error code for that specific ioctl has some special meaning. >>> >>> Great, I've no problem with that. But this particular error code you want to change >>> is actually implemented *consistently* in all drivers. There is no confusion, no >>> ambiguity, and it is according to the spec. >> >> As I said, from userspace perspective, it is not consistent to assume that EINVAL means >> not implemented. For sure at VIDIOC_S_foo, this is not consistent. Even on some GET types >> of ioctl, like for example [1][2], there are other reasons for an EINVAL return. >> >> [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-cropcap.html >> [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-audio.html >> >> The only way to make it consistent is to use different return codes for "invalid parameters" >> and for "unsupported ioctl". > > No, what we do is perfectly consistent: i.e. we always return EINVAL when an > ioctl is not supported. That's what 'consistent' means. Whether that is the > *right* error code is something else. > > But right now our API and our documentation is perfectly consistent and has been > for years. > > <snip> > >>> The fact that many drivers use error codes creatively doesn't give us an excuse >>> to just change the one error code that is actually used everywhere according to >>> the spec! That's faulty logic. >> >> The fix that it is needed is to provide a consistent way for an userspace application >> to know for sure when an ioctl is not supported. It can be done on a simple way of >> just returning a different error code for it, or with complex mechanisms like adding >> a per-ioctl flag and some hint logics based on NOP. > > It's a 'fix' that I fear may break applications because drivers suddenly change > their behavior. You can't just ignore that. You also can't analyze applications, > since closed source apps may also use it, and we obviously have no control over > those. That's the price that a closed source app pays: we can't do much to detect or prevent breaking a closed source application. A simple driver patch may hurt those applications, and only the app developer can fix. On the other hand, closed-source applications have a team of developers paid to keep their application working. So, if properly announced, they can change their code in advance in order to check for the QUERYCAP version and handle the new behaviour accordingly. I think that we should take a look at the existing open source applications and see how they handle -EINVAL, sending patches to them fixing the behaviour, if they're broken by such change. >> The V4L2 is complex enough for us to add more complexity with hints and cap flags. > > Actually, all it needs for the most part is that current implicit rules are made > explicit: if a certain querycap flag is set, then a corresponding set of ioctls > must be implemented. If I can call the GET ioctl, then the ENUM and SET (and > TRY) must also be implemented. Sensible rules at any time, they just need to be > made explicit. > > If you want to change it to ENOTTY, then go right ahead. But you can explain > it to our customers when their app suddenly breaks for some hardware. > > I don't get it. This is really not a problem. We rarely, if ever, get complaints > about it, and there is a ton of other much more important stuff that needs to > be done. > > Regards, > > Hans -- 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
On Mon, Jun 27, 2011 at 10:07 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > No, what we do is perfectly consistent: i.e. we always return EINVAL when an > ioctl is not supported. That's what 'consistent' means. Whether that is the > *right* error code is something else. You don't even understand the problem. The problem is two-fold: (a) it's the _wrong_ error code (this part you seem to get) but also (b) you ALSO return -EINVAL for ioctl's you support! How har dis (b) to understand? The fact is, -EINVAL does not mean "I don't support that ioctl". It _should_ mean "I _do_ support that ioctl, but you passed me bogus arguments for that ioctl that I cannot handle". And right now, for the v4l crowd, -EINVAL means "random error code that _could_ be because the ioctl doesn't exist, but it could also be because the ioctl _does_ exist but didn't like it's value". See the problem? The correct error code for "I don't understand this ioctl" is ENOTTY. The naming may be odd, but you should think of that error value as a "unrecognized ioctl number, you're feeding me random numbers that I don't understand and I assume for historical reasons that you tried to do some tty operation on me". I don't understand why the v4l people have such a hard time getting this. This has been going on for years. I would suggest that somebody just switch around the EINVAL in __video_do_ioctl() to -ENOTTY (and change the ENOIOCTLCMD translation to also make it ENOTTY instead of EINVAL) and just try to see if anything breaks. Maybe things actually break, and we'd have to undo it for compatibility reasons (and perhaps add a comment about the program that is so flaky that it needs a EINVAL return from the ioctl), but I really do not understand people like you who seem to argue against doing the right thing without even _trying_ it. Saying that your API is "consistent" is clearly bullshit. Your API is _wrong_. It's not consistent. Returning EINVAL can mean two different things, there's no way to tell which case it was - that's not "consistent" by any stretch of the imagination. Linus -- 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
On Monday, June 27, 2011 22:48:48 Linus Torvalds wrote: > On Mon, Jun 27, 2011 at 10:07 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > No, what we do is perfectly consistent: i.e. we always return EINVAL when an > > ioctl is not supported. That's what 'consistent' means. Whether that is the > > *right* error code is something else. > > You don't even understand the problem. > > The problem is two-fold: > (a) it's the _wrong_ error code (this part you seem to get) > but also > (b) you ALSO return -EINVAL for ioctl's you support! > > How har dis (b) to understand? The fact is, -EINVAL does not mean "I > don't support that ioctl". It _should_ mean "I _do_ support that > ioctl, but you passed me bogus arguments for that ioctl that I cannot > handle". > > And right now, for the v4l crowd, -EINVAL means "random error code > that _could_ be because the ioctl doesn't exist, but it could also be > because the ioctl _does_ exist but didn't like it's value". > > See the problem? Duh. Of course I see the problem. I've seen the problem ever since I started working on V4L back in 2004. Of course, since vfs_ioctl does the same thing this is clearly isn't some isolated V4L thing. Pot. Kettle. > The correct error code for "I don't understand this ioctl" is ENOTTY. > The naming may be odd, but you should think of that error value as a > "unrecognized ioctl number, you're feeding me random numbers that I > don't understand and I assume for historical reasons that you tried to > do some tty operation on me". > > I don't understand why the v4l people have such a hard time getting > this. This has been going on for years. > > I would suggest that somebody just switch around the EINVAL in > __video_do_ioctl() to -ENOTTY (and change the ENOIOCTLCMD translation > to also make it ENOTTY instead of EINVAL) and just try to see if > anything breaks. > > Maybe things actually break, and we'd have to undo it for > compatibility reasons (and perhaps add a comment about the program > that is so flaky that it needs a EINVAL return from the ioctl), but I > really do not understand people like you who seem to argue against > doing the right thing without even _trying_ it. > > Saying that your API is "consistent" is clearly bullshit. Your API is > _wrong_. It's not consistent. Returning EINVAL can mean two different > things, there's no way to tell which case it was - that's not > "consistent" by any stretch of the imagination. I call it consistently wrong :-) Listen, this error code is wrong. But it unfortunately has been documented like that for years. It's part of the V4L userspace API. I have no clue who may rely on this. I have no clue what might break if we change it. And the only way to know that is by actually releasing a kernel with that change. It was my understanding that we shouldn't break the userspace API. This breaks the userspace API. If everyone else says it's fine to break the userspace API this time, then who am I to object? Regards, Hans > > Linus > -- 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
On Mon, Jun 27, 2011 at 11:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Of course, since vfs_ioctl does the same thing this is clearly isn't some > isolated V4L thing. Pot. Kettle. Oh, absolutely. V4L is not at all the only confused user around. The EINVAL thing goes way back, and is a disaster. It predates Linux itself, as far as I can tell. You'll fin dlots of man-pages that have this line in it: EINVAL Request or argp is not valid. and it shows up in POSIX etc. And sadly, it generally shows up _before_ the line that says ENOTTY The specified request does not apply to the kind of object that the descriptor d references. so a lot of people get to the EINVAL, and never even notice the ENOTTY. And the above is from the current Linux man-pages - they are better than most. Googling for posix and ioctl, the first hit is some streams-specific man-page that is much worse, and makes you really think that EINVAL would be the right error for a bad ioctl. The reason is probably that ENOTTY has always been a "wft?" kind of error code. It doesn't make sense as a name for some filesystem or driver writer. Any sane person goes "Of _course_ it's not a tty, why would I say that?" At least glibc (and hopefully other C libraries) use a _string_ that makes much more sense: strerror(ENOTTY) is "Inappropriate ioctl for device" The particular translation of ENOIOCTLCMD to EINVAL was always a mistake. I'll happily try to change it for 3.1, but I'll need to do it early in the merge window to check for any issues. (In fact, the _correct_ thing to do would probably be to just do #define ENOIOCTLCMD ENOTTY and get rid of any translation - just giving ENOTTY a more appropriate name and less chance for confusion) > It was my understanding that we shouldn't break the userspace API. This breaks > the userspace API. If everyone else says it's fine to break the userspace API > this time, then who am I to object? We are indeed never supposed to break userspace. But that does not mean that we cannot fix bugs. It only means exactly what it says: we cannot break user space. If we fix a bug, and it turns out that user space actually depends on that bug, then we need to revert the fix. But it never means "you can never make any changes at all to interfaces". It only means "you cannot make any changes that break applications". There may be applications out there that really break when they get ENOTTY instead of EINVAL. But most cases that check for errors from ioctl's tend to just say "did this succeed or not" rather than "did this return EINVAL". That's *doubly* true since the error code has been ambiguous, so checking for the exact error code has always been pretty pointless. So the common use of the error code tends to be for things like strerror(), and then it would be a real improvement to show "Inappropriate ioctl for device" when the device doesn't support that ioctl, wouldn't it? But yes, let's try to fix it, and if it turns out that that breaks something, we must simply revert and probably add a comment to the source code ("EINVAL is wrong, it should be ENOTTY, but xyzzy only works with EINVAL"). Linus -- 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
> (In fact, the _correct_ thing to do would probably be to just do > > #define ENOIOCTLCMD ENOTTY > > and get rid of any translation - just giving ENOTTY a more appropriate > name and less chance for confusion) Some code uses the two to separate 'the driver specific helper code doesn't handle this' and 'does handle this'. In that situation you take away the ability of a driver to override a midlayer ioctl with -ENOTTY to say "I don't support this even if most people do" > There may be applications out there that really break when they get > ENOTTY instead of EINVAL. But most cases that check for errors from > ioctl's tend to just say "did this succeed or not" rather than "did > this return EINVAL". That's *doubly* true since the error code has > been ambiguous, so checking for the exact error code has always been > pretty pointless. Chances are if anything is busted its busted the other way on Linux and expects -ENOTTY. Certainly the large number I've been fixing over time haven't shown up any problems. -- 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
On Tuesday 28 June 2011, Alan Cox wrote: > > (In fact, the correct thing to do would probably be to just do > > > > #define ENOIOCTLCMD ENOTTY > > > > and get rid of any translation - just giving ENOTTY a more appropriate > > name and less chance for confusion) > > Some code uses the two to separate 'the driver specific helper code > doesn't handle this' and 'does handle this'. In that situation you take > away the ability of a driver to override a midlayer ioctl with -ENOTTY to > say "I don't support this even if most people do" Right. Similarly, in compat_sys_ioctl returning -ENOIOCTLCMD from fops->compat_ioctl means "the driver has provided no compatibility handler for this command, need to check the global translation table", while -ENOTTY returned from ->compat_ioctl means "this command won't work on this device, don't bother looking at the table and don't print an annoying message". Arnd -- 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
Em 28-06-2011 13:05, Linus Torvalds escreveu: > On Mon, Jun 27, 2011 at 11:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> It was my understanding that we shouldn't break the userspace API. This breaks >> the userspace API. If everyone else says it's fine to break the userspace API >> this time, then who am I to object? > > We are indeed never supposed to break userspace. > > But that does not mean that we cannot fix bugs. It only means exactly > what it says: we cannot break user space. > > If we fix a bug, and it turns out that user space actually depends on > that bug, then we need to revert the fix. But it never means "you can > never make any changes at all to interfaces". It only means "you > cannot make any changes that break applications". > > There may be applications out there that really break when they get > ENOTTY instead of EINVAL. But most cases that check for errors from > ioctl's tend to just say "did this succeed or not" rather than "did > this return EINVAL". That's *doubly* true since the error code has > been ambiguous, so checking for the exact error code has always been > pretty pointless. > > So the common use of the error code tends to be for things like > strerror(), and then it would be a real improvement to show > "Inappropriate ioctl for device" when the device doesn't support that > ioctl, wouldn't it? > > But yes, let's try to fix it, and if it turns out that that breaks > something, we must simply revert and probably add a comment to the > source code ("EINVAL is wrong, it should be ENOTTY, but xyzzy only > works with EINVAL"). I've applied the fix locally, using ENOTTY, and tested with 3 drivers that have several non-implemented ioctl's: vivi, uvcvideo and gspca (they basically don't implement tuner ioctl's. The uvcvideo doesn't implement the *STD ioctls, and the gspca driver doesn't implement the ENUM_FRAME* ioctl's). I tested with camorama (using libv4l1 conversion code), vlc, qv4l2, xawtv3 and mplayer. I also tested two closed source applications (Skype and flash - at twitcam). With all drivers, the applications worked as expected. There is just a minor glitch with xawtv3, as it currently doesn't print an error log if VIDIOC_G_STD returns -EINVAL when debug is disabled. Yet, the error is not fatal and happens only once, during device probing. Xawtv works fine, and suppressing the noise is an easy fix. As we're releasing version 1.101 of it with alsa streaming support, I'll latter add a code there fixing it. Complex TV applications like mythtv won't work if the devices don't implement tuner and control ioctl's. I don't believe that it would fail if we change the return code for unimplemented ioctls, because if an ioctl that mythtv needs is not implemented, it will fail anyway. Unfortunately, installing mythtv is a very complex task, and requires hours/days of work. I can't affort to install it for testing ATM, but I'm sure others have it installed, and can provide us some feedback. So, I'll prepare the patches for replacing EINVAL to ENOTTY and add it at next. This'll give us some time to get feedback about eventual breakages on other tools that I didn't test. Thanks, Mauro -- 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
diff --git a/fs/ioctl.c b/fs/ioctl.c index 1d9b9fc..802fbbd 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -41,8 +41,6 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, goto out; error = filp->f_op->unlocked_ioctl(filp, cmd, arg); - if (error == -ENOIOCTLCMD) - error = -EINVAL; out: return error; }