[lkml]   [2011]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist
    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.


    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:
    >> ---------------|--------------------------------
    >> 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
    >> EINTR 21
    >> EINVAL 501
    >> EIO 305
    >> EMFILE 1
    >> ENFILE 7
    >> ENOBUFS 7
    >> ENODATA 4
    >> ENODEV 270
    >> ENOENT 46
    >> 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
    >> ESHUTDOWN 27
    >> ESPIPE 3
    >> ETIME 53
    >> ETIMEDOUT 37
    >> EUSERS 2
    >> 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.


     \ /
      Last update: 2011-06-27 17:37    [W:0.029 / U:20.824 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site