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@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.
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)?
--
Cheers,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
--
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
