[haiku-commits] Re: r40787 - haiku/trunk/src/system/kernel/fs

On 2011-03-03 at 10:44:51 [+0100], Jérôme Duval <korli@xxxxxxxxxxxxxxxx> 
wrote:
> 2011/3/2 Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>:
> > korli@xxxxxxxxxxxxxxxx wrote:
> >> +++ haiku/trunk/src/system/kernel/fs/fd.cpp   2011-03-02 18:58:14 UTC
> >> (rev 40787)
> >> @@ -518,8 +518,11 @@
> >>       if (descriptor->ops->fd_ioctl)
> >>               status = descriptor->ops->fd_ioctl(descriptor, op, buffer,
> >> length);
> >>       else
> >> -             status = B_NOT_SUPPORTED;
> >> +             status = B_DEV_INVALID_IOCTL;
> >
> > Not sure this should be replaced, too. At least I think it's not so
> > nice to lose the expressiveness of our error codes because of some
> > poorly documented POSIX functions.
> 
> Well I wasn't sure too. These error codes look the same for me, some
> are a bit more specific to the hook like B_DEV_INVALID_IOCTL.
> B_NOT_SUPPORTED is more generic.
> We can well assume the same meaning for both.
> 
> >
> >> +     if (status == B_DEV_INVALID_IOCTL)
> >> +             status = ENOTTY;
> >
> > Isn't that a bit too central? Where is the difference to returning
> > ENOTTY directly this way?
> 
> Well I asked, and Ingo suggested to keep this mapping in the kernel
> because ioctl() can be called kernel side too.

Yes, though I thought that would only be done for the two ioctls you 
mentioned. Admittedly I hadn't looked at the ioctl() specification before. 
Apparently ENOTTY is the right error when the file isn't a STREAMS device 
(i.e. that's the one to be returned by "normal" file systems) ENODEV when 
the file is a STREAMS device, but generally doesn't support ioctl(), and 
EINVAL, if the particular ioctl isn't supported.

> > The POSIX definition of ioctl() is rather useless, I'm afraid, but also
> > states EINVAL for this error case. And since we don't differentiate
> > between ioctl/fcntl on that level, maybe we should move the error code
> > to libroot instead?

I don't see what you mean that we don't differentiate between ioctl() and 
fcntl(). The FS hook for fcntl() is set_flags(). The only fuzziness is where 
it comes to non-blocking I/O, since that can be set via both and I believe 
we don't keep the descriptor's flag in sync when done via ioctl().

I would suggest to consequently use the error codes POSIX specifies for the 
respective cases. That would leave B_DEV_INVALID_IOCTL unused. IOW:

* no FS ioctl() hook -> ENOTTY

* devfs, no device ioctl() hook -> ENODEV or ENOTTY (depending on character 
vs. block device?)

* in a "normal" FS ioctl() hook: ENOTTY, if not supported

* in a device ioctl() hooks: EINVAL, if not supported

CU, Ingo

Other related posts: