[alsa-devel] Confusion about whether snd_pcm_poll_descriptors_revents()'s revents field is a single integer or an array
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#a3...
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=74d1d1a...
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
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#a3...
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=74d1d1a...
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
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#a3...
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=74d1d1a...
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
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#a3...
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=74d1d1a...
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;
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.
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
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#a3...
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=74d1d1a...
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=fir...
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
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#a3...
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=74d1d1a...
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
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#a3...
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=74d1d1a...
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.
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#a3...
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=74d1d1a...
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
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
participants (3)
-
Jaroslav Kysela -
Lennart Poettering -
Takashi Iwai