[pulseaudio-discuss] [PATCH v2 2/5] oss: don't fail resume if trigger() fails

[Tanu Kaskinen]

The previous code made the SET_STATE message fail if trigger() failed.
However, trigger() was called after pa_sink/source_process_msg(), which
meant that the main thread that sent the SET_STATE thought that resuming
failed, but nothing was undone in the IO thread, so in the IO thread
things seemed as if the sink/source was successfully resumed. (I don't
use OSS myself, so I don't know what kind of practical problems this
could cause).

Unless some complex undo logic is implemented, I believe it's best to
ignore all failures in trigger(). Most error cases were already ignored,
and the only one that wasn't ignored doesn't seem too serious.

I also moved trigger() to happen before pa_sink/source_process_msg(),
which made it necessary to add new state parameters to trigger(). The
reason for this move is that I want to move the SET_STATE handler code
into a separate callback, and if things are done both before and after
pa_sink/source_process_msg(), that makes things more complicated.

The previous code checked the return value of
pa_sink/source_process_msg() before calling trigger(), but that was
unnecessary, since pa_sink/source_process_msg() never fails when
processing the SET_STATE messages.
---
 src/modules/oss/module-oss.c | 59 ++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/src/modules/oss/module-oss.c b/src/modules/oss/module-oss.c
index fb978b5ee..7d1b9f52b 100644
--- a/src/modules/oss/module-oss.c
+++ b/src/modules/oss/module-oss.c
@@ -154,20 +154,23 @@ static const char* const valid_modargs[] = {
     NULL
 };
 
-static int trigger(struct userdata *u, bool quick) {
+/* Sink and source states are passed as arguments, because this is called
+ * during state changes, and we need the new state, but thread_info.state
+ * has not yet been updated. */
+static void trigger(struct userdata *u, pa_sink_state_t sink_state, pa_source_state_t source_state, bool quick) {
     int enable_bits = 0, zero = 0;
 
     pa_assert(u);
 
     if (u->fd < 0)
-        return 0;
+        return;
 
     pa_log_debug("trigger");
 
-    if (u->source && PA_SOURCE_IS_OPENED(u->source->thread_info.state))
+    if (u->source && PA_SOURCE_IS_OPENED(source_state))
         enable_bits |= PCM_ENABLE_INPUT;
 
-    if (u->sink && PA_SINK_IS_OPENED(u->sink->thread_info.state))
+    if (u->sink && PA_SINK_IS_OPENED(sink_state))
         enable_bits |= PCM_ENABLE_OUTPUT;
 
     pa_log_debug("trigger: %i", enable_bits);
@@ -204,21 +207,20 @@ static int trigger(struct userdata *u, bool quick) {
              * register the fd as ready.
              */
 
-            if (u->source && PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
+            if (u->source && PA_SOURCE_IS_OPENED(source_state)) {
                 uint8_t *buf = pa_xnew(uint8_t, u->in_fragment_size);
 
-                if (pa_read(u->fd, buf, u->in_fragment_size, NULL) < 0) {
+                /* XXX: Shouldn't this be done only when resuming the source?
+                 * Currently this code path is executed also when resuming the
+                 * sink while the source is already running. */
+
+                if (pa_read(u->fd, buf, u->in_fragment_size, NULL) < 0)
                     pa_log("pa_read() failed: %s", pa_cstrerror(errno));
-                    pa_xfree(buf);
-                    return -1;
-                }
 
                 pa_xfree(buf);
             }
         }
     }
-
-    return 0;
 }
 
 static void mmap_fill_memblocks(struct userdata *u, unsigned n) {
@@ -641,8 +643,8 @@ fail:
 /* Called from IO context */
 static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offset, pa_memchunk *chunk) {
     struct userdata *u = PA_SINK(o)->userdata;
-    int ret;
     bool do_trigger = false, quick = true;
+    pa_sink_state_t new_state;
 
     switch (code) {
 
@@ -662,8 +664,9 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse
         }
 
         case PA_SINK_MESSAGE_SET_STATE:
+            new_state = PA_PTR_TO_UINT(data);
 
-            switch ((pa_sink_state_t) PA_PTR_TO_UINT(data)) {
+            switch (new_state) {
 
                 case PA_SINK_SUSPENDED:
                     pa_assert(PA_SINK_IS_OPENED(u->sink->thread_info.state));
@@ -709,23 +712,18 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse
             }
 
             break;
-
     }
 
-    ret = pa_sink_process_msg(o, code, data, offset, chunk);
+    if (do_trigger)
+        trigger(u, new_state, u->source ? u->source->thread_info.state : PA_SOURCE_INVALID_STATE, quick);
 
-    if (ret >= 0 && do_trigger) {
-        if (trigger(u, quick) < 0)
-            return -1;
-    }
-
-    return ret;
+    return pa_sink_process_msg(o, code, data, offset, chunk);
 }
 
 static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t offset, pa_memchunk *chunk) {
     struct userdata *u = PA_SOURCE(o)->userdata;
-    int ret;
-    int do_trigger = false, quick = true;
+    bool do_trigger = false, quick = true;
+    pa_source_state_t new_state;
 
     switch (code) {
 
@@ -744,8 +742,10 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off
         }
 
         case PA_SOURCE_MESSAGE_SET_STATE:
+            new_state = PA_PTR_TO_UINT(data);
+
+            switch (new_state) {
 
-            switch ((pa_source_state_t) PA_PTR_TO_UINT(data)) {
                 case PA_SOURCE_SUSPENDED:
                     pa_assert(PA_SOURCE_IS_OPENED(u->source->thread_info.state));
 
@@ -789,17 +789,12 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off
 
             }
             break;
-
     }
 
-    ret = pa_source_process_msg(o, code, data, offset, chunk);
-
-    if (ret >= 0 && do_trigger) {
-        if (trigger(u, quick) < 0)
-            return -1;
-    }
+    if (do_trigger)
+        trigger(u, u->sink ? u->sink->thread_info.state : PA_SINK_INVALID_STATE, new_state, quick);
 
-    return ret;
+    return pa_source_process_msg(o, code, data, offset, chunk);
 }
 
 static void sink_get_volume(pa_sink *s) {
-- 
2.16.1