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@xxxxxxxxxxxxxxx>
>>> ---
>>> 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.
> -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").
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
