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

[Takashi Iwai <tiwai@suse.de>]

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