Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Hans Verkuil <hverkuil@xs4all.nl>]

On 10/02/13 16:45, Sakari Ailus wrote:
> Hi Hans,
> 
> Hans Verkuil wrote:
>> On 10/02/13 16:18, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the comments!
>>>
>>> Hans Verkuil wrote:
>>>> On 10/02/13 15:45, Sakari Ailus wrote:
>>>>> Dequeueing events was is entirely possible even if none are subscribed,
>>>>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no
>>>>> events are subscribed.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> ---
>>>>>    drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++--
>>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>>>>> b/drivers/media/v4l2-core/v4l2-event.c
>>>>> index b53897e..553a800 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-event.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>>>>> @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct
>>>>> v4l2_event *event,
>>>>>            mutex_unlock(fh->vdev->lock);
>>>>>
>>>>>        do {
>>>>> -        ret = wait_event_interruptible(fh->wait,
>>>>> -                           fh->navailable != 0);
>>>>> +        bool subscribed;
>>>>
>>>> Can you add an empty line here?
>>>
>>> Sure.
>>>
>>>>> +        ret = wait_event_interruptible(
>>>>> +            fh->wait,
>>>>> +            fh->navailable != 0 ||
>>>>> +            !(subscribed = v4l2_event_has_subscribed(fh)));
>>>>>            if (ret < 0)
>>>>>                break;
>>>>> +        if (!subscribed) {
>>>>> +            ret = -EIO;
>>>>
>>>> Shouldn't this be -ENOENT?
>>>
>>> If I use -ENOENT, having no events subscribed is indistinguishable
>>> form no events pending condition. Combine that with using select(2),
>>> and you can no longer distinguish having no events subscribed from
>>> the case where you got an event but someone else (another thread or
>>> process) dequeued it.
>>
>> OK, but then your commit message is out of sync with the actual patch since
>> the commit log says ENOENT.
> 
> Right. The error code was the last thing I changed before sending the
> patch, and I ignored it was also present in the commit message. :-P
> 
>>> -EIO makes that explicit --- this also mirrors the behaviour of
>>> VIDIOC_DQBUF. (And it must be documented as well, which is missing
>>> from the patch currently.)
>>
>> I don't like using EIO for this. EIO generally is returned if a hardware
>> error or an unexpected hardware condition occurs. How about -ENOMSG? Or
>> perhaps EPIPE? (As in: "the pipe containing events is gone").
> 
> There is no pipe (or at least wasn't; it's a queue or rather is
> implemented as a fifo :)) so of the two I prefer -ENOMSG. What would
> you think of -ENODATA or -EPERM (which is used e.g. when writing
> read-only controls)?
> 

I don't like ENODATA, mostly because it is so close in meaning to ENOENT.
EPERM would work for me. It's probably a bit better than ENOMSG.

Regards,

	Hans