Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Lennart Poettering]

Heya!

If we look into the pcm example how snd_pcm_poll_descriptors_revents()
is used then we can see that the revents parameter apparently is
supposed to be a single integer. (which makes a lot of sense to me)

http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33

However, snd_pcm_wait_nocheck() calls the same function and assumes it
is a complete array!

http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368

So what's it now? It makes more sense to me if it would be a single
fd. 

The problem becomnes visible if someone writes a plugin that wants
wakeups from more than one fd because either snd_pcm_wait() using
applications start to act weirdly because the revents are not fully
initialized or snd_pcm_poll_descriptors_revents() using applications
get invalid memory accesses.

This has become visible in the Bluetooth plugin.

I guess nobody has written a plugin that has more than one fd to sleep
on yet, right?

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 2 Feb 2009 03:50:22 +0100,
Lennart Poettering wrote:
> 
> Heya!
> 
> If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> is used then we can see that the revents parameter apparently is
> supposed to be a single integer. (which makes a lot of sense to me)
> 
> http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> 
> However, snd_pcm_wait_nocheck() calls the same function and assumes it
> is a complete array!
> 
> http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> 
> So what's it now? It makes more sense to me if it would be a single
> fd. 

No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
each poll_fd in the given array.

> The problem becomnes visible if someone writes a plugin that wants
> wakeups from more than one fd because either snd_pcm_wait() using
> applications start to act weirdly because the revents are not fully
> initialized or snd_pcm_poll_descriptors_revents() using applications
> get invalid memory accesses.
> 
> This has become visible in the Bluetooth plugin.
> 
> I guess nobody has written a plugin that has more than one fd to sleep
> on yet, right?

Right, it hasn't been a big problem just because it's a pretty rare
case.


Takashi

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 02 Feb 2009 08:02:29 +0100,
I wrote:
> 
> At Mon, 2 Feb 2009 03:50:22 +0100,
> Lennart Poettering wrote:
> > 
> > Heya!
> > 
> > If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> > is used then we can see that the revents parameter apparently is
> > supposed to be a single integer. (which makes a lot of sense to me)
> > 
> > http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> > 
> > However, snd_pcm_wait_nocheck() calls the same function and assumes it
> > is a complete array!
> > 
> > http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> > 
> > So what's it now? It makes more sense to me if it would be a single
> > fd. 
> 
> No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
> each poll_fd in the given array.

Hmm..  just looking through the whole tree again, this thing seems
mixed up weirdly right now.
At least, the latest pcm_hw.c assumes the single revent.  Meanwhile,
the old pcm_multi.c assumed the multiple revents.

As you notice the work "revents", the function itself supposes the
multiple events.   However, obviously, handling the single revent is
much easier for applications. 

So I believe we should go to the current pcm_hw.c implementation way,
i.e. demangling to a single revent.  But, yes, more investigations and
fixes are needed.


Takashi

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Lennart Poettering]

On Mon, 02.02.09 08:02, Takashi Iwai (tiwai@suse.de) wrote:

> > If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> > is used then we can see that the revents parameter apparently is
> > supposed to be a single integer. (which makes a lot of sense to me)
> > 
> > http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> > 
> > However, snd_pcm_wait_nocheck() calls the same function and assumes it
> > is a complete array!
> > 
> > http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> > 
> > So what's it now? It makes more sense to me if it would be a single
> > fd. 
> 
> No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
> each poll_fd in the given array.

But does that really make sense? I mean
snd_pcm_poll_descriptors_revents() is supposed to be called by
applications that integrate ALSA into some kind of event loop. What
are they supposed to do with those multiple entries -- except simply
ORing them together and treating them as one? The application has no
information what those seperate fds mean, so why export that
information to the app?

>From an application's point of view, how is revents[0] = POLLIN,
revents[1] = 0 in any way different than revents[0] = 0, revents[1] =
POLLIN?

Also, a quick search with google code search reveals that everyone
trusted the doxygen docs and treated revents as a single integer:

http://www.google.de/codesearch?num=30&hl=en&safe=off&client=firefox-a&rls=org.mozilla%3Aen-US%3Aofficial&hs=V74&q=snd_pcm_poll_descriptors_revents&btnG=Search

For example, the following systems treat it as a single integer:

PortAudio
mplayer
allegro
wine
clalsadrv
arts
alsa-oss
disorder
alsaplayer
mumble
PulseAudio

and that list goes on and on and on. In fact I couldn't find a single
package treating it is array. Really declaring this now an array will
hence result in breakage in lots and lots of applications.

Maybe the implementation should now actually follow the documentation
and make it really a single integer, given that this makes more sense
to the user anyway?

Also, what's the point of having the revents parameter anyway if it's
just about demangling the pollfd array? I mean, then you could simply
do this in place.  It's not just the documentation and the example
that suggest that this is a single integer, it's also the simple
signature of the function that suggests so.

So, please, make this a single integer. Really going back to making it
an array sounds like an awful solution to me.

BTW, pfds in snd_pcm_poll_descriptors_revents could use a 'const'.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 2 Feb 2009 15:49:13 +0100,
Lennart Poettering wrote:
> 
> On Mon, 02.02.09 08:02, Takashi Iwai (tiwai@suse.de) wrote:
> 
> > > If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> > > is used then we can see that the revents parameter apparently is
> > > supposed to be a single integer. (which makes a lot of sense to me)
> > > 
> > > http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> > > 
> > > However, snd_pcm_wait_nocheck() calls the same function and assumes it
> > > is a complete array!
> > > 
> > > http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> > > 
> > > So what's it now? It makes more sense to me if it would be a single
> > > fd. 
> > 
> > No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
> > each poll_fd in the given array.
> 
> But does that really make sense? I mean
> snd_pcm_poll_descriptors_revents() is supposed to be called by
> applications that integrate ALSA into some kind of event loop. What
> are they supposed to do with those multiple entries -- except simply
> ORing them together and treating them as one? The application has no
> information what those seperate fds mean, so why export that
> information to the app?

Originally the multiple pfds were introduced to handle multi plugin,
which bundles multiple instances.  So, the original version of it was
pretty straightforward -- just simply calls poll to each and update
each pfd necessarily.

After some time, multi plugin itself was changed to expose only a
single pfd, so there is no meaning for that.  That's why it never got
a realistic problem.  Meanwhile, hw pcm introduced the multiple pfds
again (for cases with timer), and this could be a problem again right
now.

It's not about the question whether it makes sense.  It *was* designed
so originally although it's fairly useless.  Due to the uselessness,
one side got fixed, but another side remained.


Takashi

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 02 Feb 2009 15:34:57 +0100,
I wrote:
> 
> At Mon, 02 Feb 2009 08:02:29 +0100,
> I wrote:
> > 
> > At Mon, 2 Feb 2009 03:50:22 +0100,
> > Lennart Poettering wrote:
> > > 
> > > Heya!
> > > 
> > > If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> > > is used then we can see that the revents parameter apparently is
> > > supposed to be a single integer. (which makes a lot of sense to me)
> > > 
> > > http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> > > 
> > > However, snd_pcm_wait_nocheck() calls the same function and assumes it
> > > is a complete array!
> > > 
> > > http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> > > 
> > > So what's it now? It makes more sense to me if it would be a single
> > > fd. 
> > 
> > No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
> > each poll_fd in the given array.
> 
> Hmm..  just looking through the whole tree again, this thing seems
> mixed up weirdly right now.
> At least, the latest pcm_hw.c assumes the single revent.  Meanwhile,
> the old pcm_multi.c assumed the multiple revents.
> 
> As you notice the work "revents", the function itself supposes the
> multiple events.   However, obviously, handling the single revent is
> much easier for applications. 
> 
> So I believe we should go to the current pcm_hw.c implementation way,
> i.e. demangling to a single revent.  But, yes, more investigations and
> fixes are needed.

And the patch is below.
If no one has objection, I'll apply it later.


thanks,

Takashi

==
>From fdd79369b24d2e9405d4ec5f1bdfc09dda127ed4 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Mon, 2 Feb 2009 15:43:48 +0100
Subject: [PATCH] Fix handling of revents in snd_pcm_poll_descriptors_revents()

The revents parameter is ambiguously defined whether it's a pointer
to a single event or an arary.  While the recent pcm_hw.c assumes it
being a single event, the old multi plugin supposed it as an array.
Similarly snd_pcm_wait_nocheck() checks the revents as an arary.

This patch defines the behavior of revents more strictly.  It's a
pointer of a single event.

Fixed snd_pcm_wait_nocheck() to follow that.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm.c |   42 +++++++++++++++++++-----------------------
 1 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 74d1d1a..0203720 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -1430,7 +1430,7 @@ int snd_pcm_poll_descriptors(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int s
  * \param pcm PCM handle
  * \param pfds array of poll descriptors
  * \param nfds count of poll descriptors
- * \param revents returned events
+ * \param revents pointer to the returned (single) event
  * \return zero if success, otherwise a negative error code
  *
  * This function does "demangling" of the revents mask returned from
@@ -1440,6 +1440,9 @@ int snd_pcm_poll_descriptors(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int s
  * syscall returned that some events are waiting, this function might
  * return empty set of events. In this case, application should
  * do next event waiting using poll() or select().
+ *
+ * Note: Even if multiple poll descriptors are used (i.e. pfds > 1),
+ * this function returns only a single event.
  */
 int snd_pcm_poll_descriptors_revents(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int nfds, unsigned short *revents)
 {
@@ -2338,8 +2341,8 @@ int snd_pcm_wait(snd_pcm_t *pcm, int timeout)
 int snd_pcm_wait_nocheck(snd_pcm_t *pcm, int timeout)
 {
 	struct pollfd *pfd;
-	unsigned short *revents;
-	int i, npfds, pollio, err, err_poll;
+	unsigned short revents = 0;
+	int i, npfds, err, err_poll;
 	
 	npfds = snd_pcm_poll_descriptors_count(pcm);
 	if (npfds <= 0 || npfds >= 16) {
@@ -2347,7 +2350,6 @@ int snd_pcm_wait_nocheck(snd_pcm_t *pcm, int timeout)
 		return -EIO;
 	}
 	pfd = alloca(sizeof(*pfd) * npfds);
-	revents = alloca(sizeof(*revents) * npfds);
 	err = snd_pcm_poll_descriptors(pcm, pfd, npfds);
 	if (err < 0)
 		return err;
@@ -2356,7 +2358,6 @@ int snd_pcm_wait_nocheck(snd_pcm_t *pcm, int timeout)
 		return -EIO;
 	}
 	do {
-		pollio = 0;
 		err_poll = poll(pfd, npfds, timeout);
 		if (err_poll < 0) {
 		        if (errno == EINTR)
@@ -2365,28 +2366,23 @@ int snd_pcm_wait_nocheck(snd_pcm_t *pcm, int timeout)
                 }
 		if (! err_poll)
 			break;
-		err = snd_pcm_poll_descriptors_revents(pcm, pfd, npfds, revents);
+		err = snd_pcm_poll_descriptors_revents(pcm, pfd, npfds, &revents);
 		if (err < 0)
 			return err;
-		for (i = 0; i < npfds; i++) {
-			if (revents[i] & (POLLERR | POLLNVAL)) {
-				/* check more precisely */
-				switch (snd_pcm_state(pcm)) {
-				case SND_PCM_STATE_XRUN:
-					return -EPIPE;
-				case SND_PCM_STATE_SUSPENDED:
-					return -ESTRPIPE;
-				case SND_PCM_STATE_DISCONNECTED:
-					return -ENODEV;
-				default:
-					return -EIO;
-				}
+		if (revents & (POLLERR | POLLNVAL)) {
+			/* check more precisely */
+			switch (snd_pcm_state(pcm)) {
+			case SND_PCM_STATE_XRUN:
+				return -EPIPE;
+			case SND_PCM_STATE_SUSPENDED:
+				return -ESTRPIPE;
+			case SND_PCM_STATE_DISCONNECTED:
+				return -ENODEV;
+			default:
+				return -EIO;
 			}
-			if ((revents[i] & (POLLIN | POLLOUT)) == 0)
-				continue;
-			pollio++;
 		}
-	} while (! pollio);
+	} while (!(revents & (POLLIN | POLLOUT)));
 #if 0 /* very useful code to test poll related problems */
 	{
 		snd_pcm_sframes_t avail_update;
-- 
1.6.1.2

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Jaroslav Kysela]

On Mon, 2 Feb 2009, Takashi Iwai wrote:

> At Mon, 2 Feb 2009 15:49:13 +0100,
> Lennart Poettering wrote:
> > 
> > On Mon, 02.02.09 08:02, Takashi Iwai (tiwai@suse.de) wrote:
> > 
> > > > If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> > > > is used then we can see that the revents parameter apparently is
> > > > supposed to be a single integer. (which makes a lot of sense to me)
> > > > 
> > > > http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> > > > 
> > > > However, snd_pcm_wait_nocheck() calls the same function and assumes it
> > > > is a complete array!
> > > > 
> > > > http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> > > > 
> > > > So what's it now? It makes more sense to me if it would be a single
> > > > fd. 
> > > 
> > > No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
> > > each poll_fd in the given array.
> > 
> > But does that really make sense? I mean
> > snd_pcm_poll_descriptors_revents() is supposed to be called by
> > applications that integrate ALSA into some kind of event loop. What
> > are they supposed to do with those multiple entries -- except simply
> > ORing them together and treating them as one? The application has no
> > information what those seperate fds mean, so why export that
> > information to the app?
> 
> Originally the multiple pfds were introduced to handle multi plugin,
> which bundles multiple instances.  So, the original version of it was
> pretty straightforward -- just simply calls poll to each and update
> each pfd necessarily.

But snd_pcm_poll_descriptors_revents() was designed to return single 
value originally. Multiple fds were never used in pcm_multi for 
revents, too. Also, snd_pcm_wait() was OK (in the sense revents 
callback) before:

commit d5b98234478680c4afdf475988b8952605f66ff8
Author: Takashi Iwai <tiwai@suse.de>
Date:   Wed May 18 13:28:06 2005 +0000

    Fix snd_pcm_wait() for multiple pollfd's

    Fixed snd_pcm_wait() to handle multiple pollfd's.


					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 2 Feb 2009 15:49:13 +0100,
Lennart Poettering wrote:
> 
> BTW, pfds in snd_pcm_poll_descriptors_revents could use a 'const'.

Right.  However, it's so spread over, and adding the const prefix
causes many compile errors.  This also influences on the exported API
such as the external plugins, thus fixing this means more possible
build problems.  So, I'd leave it as is unless it must be changed.
(Yeah I'm a kind of conservative about API change :)


thanks,

Takashi

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Jaroslav Kysela]

On Mon, 2 Feb 2009, Takashi Iwai wrote:

> ==
> >From fdd79369b24d2e9405d4ec5f1bdfc09dda127ed4 Mon Sep 17 00:00:00 2001
> From: Takashi Iwai <tiwai@suse.de>
> Date: Mon, 2 Feb 2009 15:43:48 +0100
> Subject: [PATCH] Fix handling of revents in snd_pcm_poll_descriptors_revents()
> 
> The revents parameter is ambiguously defined whether it's a pointer
> to a single event or an arary.  While the recent pcm_hw.c assumes it
> being a single event, the old multi plugin supposed it as an array.
> Similarly snd_pcm_wait_nocheck() checks the revents as an arary.

Please, remove notice about the multi plugin. It is not true.

> This patch defines the behavior of revents more strictly.  It's a
> pointer of a single event.
>
> 
> Fixed snd_pcm_wait_nocheck() to follow that.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Otherwise acked..

					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 2 Feb 2009 16:04:10 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Mon, 2 Feb 2009, Takashi Iwai wrote:
> 
> > ==
> > >From fdd79369b24d2e9405d4ec5f1bdfc09dda127ed4 Mon Sep 17 00:00:00 2001
> > From: Takashi Iwai <tiwai@suse.de>
> > Date: Mon, 2 Feb 2009 15:43:48 +0100
> > Subject: [PATCH] Fix handling of revents in snd_pcm_poll_descriptors_revents()
> > 
> > The revents parameter is ambiguously defined whether it's a pointer
> > to a single event or an arary.  While the recent pcm_hw.c assumes it
> > being a single event, the old multi plugin supposed it as an array.
> > Similarly snd_pcm_wait_nocheck() checks the revents as an arary.
> 
> Please, remove notice about the multi plugin. It is not true.
> 
> > This patch defines the behavior of revents more strictly.  It's a
> > pointer of a single event.
> >
> > 
> > Fixed snd_pcm_wait_nocheck() to follow that.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Otherwise acked..

OK, done.


Takashi

Re: Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array

[Takashi Iwai]

At Mon, 2 Feb 2009 16:02:38 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Mon, 2 Feb 2009, Takashi Iwai wrote:
> 
> > At Mon, 2 Feb 2009 15:49:13 +0100,
> > Lennart Poettering wrote:
> > > 
> > > On Mon, 02.02.09 08:02, Takashi Iwai (tiwai@suse.de) wrote:
> > > 
> > > > > If we look into the pcm example how snd_pcm_poll_descriptors_revents()
> > > > > is used then we can see that the revents parameter apparently is
> > > > > supposed to be a single integer. (which makes a lot of sense to me)
> > > > > 
> > > > > http://www.alsa-project.org/alsa-doc/alsa-lib/_2test_2pcm_8c-example.html#a33
> > > > > 
> > > > > However, snd_pcm_wait_nocheck() calls the same function and assumes it
> > > > > is a complete array!
> > > > > 
> > > > > http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=74d1d1a4bd6083cd461b6d793c0ae41cca912f16;hb=HEAD#l2368
> > > > > 
> > > > > So what's it now? It makes more sense to me if it would be a single
> > > > > fd. 
> > > > 
> > > > No, it's a bug in test/pcm.c.  As documented, the API converts (fixes)
> > > > each poll_fd in the given array.
> > > 
> > > But does that really make sense? I mean
> > > snd_pcm_poll_descriptors_revents() is supposed to be called by
> > > applications that integrate ALSA into some kind of event loop. What
> > > are they supposed to do with those multiple entries -- except simply
> > > ORing them together and treating them as one? The application has no
> > > information what those seperate fds mean, so why export that
> > > information to the app?
> > 
> > Originally the multiple pfds were introduced to handle multi plugin,
> > which bundles multiple instances.  So, the original version of it was
> > pretty straightforward -- just simply calls poll to each and update
> > each pfd necessarily.
> 
> But snd_pcm_poll_descriptors_revents() was designed to return single 
> value originally.

Hrm, then the definition was very ambiguous...


Takashi