[pulseaudio-discuss] [PATCH] srbchannel: Defer reading when setting up read callback

[David Henningsson]

Calling the callback while setting it up can make things
complicated for clients, as the callback can do arbitrarily
things.

In this case, a protocol error caused the srbchannel to be
owned by both the pstream and the native connection.

Now the read callback is deferred, making sure the callback
is called from a cleaner context where errors are handled
appropriately.

Reported-by: Tanu Kaskinen <tanu.kaskinen at linux.intel.com>
Signed-off-by: David Henningsson <david.henningsson at canonical.com>
---
 src/pulsecore/srbchannel.c | 22 +++++++++++++++++++---
 src/pulsecore/srbchannel.h |  3 ---
 2 files changed, 19 insertions(+), 6 deletions(-)

Hi Tanu, can you check that this patch resolves your problem,
as it seems to require your "unfinished code" to replicate?

diff --git a/src/pulsecore/srbchannel.c b/src/pulsecore/srbchannel.c
index 3f81e25..b9b8f1e 100644
--- a/src/pulsecore/srbchannel.c
+++ b/src/pulsecore/srbchannel.c
@@ -86,6 +86,7 @@ struct pa_srbchannel {
     pa_srbchannel_cb_t callback;
 
     pa_io_event *read_event;
+    pa_defer_event *defer_event;
     pa_mainloop_api *mainloop;
 };
 
@@ -211,6 +212,17 @@ static void semread_cb(pa_mainloop_api *m, pa_io_event *e, int fd, pa_io_event_f
     srbchannel_rwloop(sr);
 }
 
+static void defer_cb(pa_mainloop_api *m, pa_defer_event *e, void *userdata) {
+    pa_srbchannel* sr = userdata;
+
+#ifdef DEBUG_SRBCHANNEL
+    pa_log("Calling rw loop from deferred event");
+#endif
+
+    m->defer_enable(e, 0);
+    srbchannel_rwloop(sr);
+}
+
 pa_srbchannel* pa_srbchannel_new(pa_mainloop_api *m, pa_mempool *p) {
     int capacity;
     int readfd;
@@ -331,9 +343,11 @@ void pa_srbchannel_set_callback(pa_srbchannel *sr, pa_srbchannel_cb_t callback,
     sr->callback = callback;
     sr->cb_userdata = userdata;
 
-    if (sr->callback)
-        /* Maybe deferred event? */
-        srbchannel_rwloop(sr);
+    if (sr->callback) {
+        if (!sr->defer_event)
+            sr->defer_event = sr->mainloop->defer_new(sr->mainloop, defer_cb, sr);
+        sr->mainloop->defer_enable(sr->defer_event, 1);
+    }
 }
 
 void pa_srbchannel_free(pa_srbchannel *sr)
@@ -343,6 +357,8 @@ void pa_srbchannel_free(pa_srbchannel *sr)
 #endif
     pa_assert(sr);
 
+    if (sr->defer_event)
+        sr->mainloop->defer_free(sr->defer_event);
     if (sr->read_event)
         sr->mainloop->io_free(sr->read_event);
 
diff --git a/src/pulsecore/srbchannel.h b/src/pulsecore/srbchannel.h
index e41cc52..c96877e 100644
--- a/src/pulsecore/srbchannel.h
+++ b/src/pulsecore/srbchannel.h
@@ -52,9 +52,6 @@ size_t pa_srbchannel_read(pa_srbchannel *sr, void *data, size_t l);
  *
  * Return false to abort all processing (e g if the srbchannel has been freed during the callback).
  * Otherwise return true.
- *
- * Note that the callback will be called immediately, to be able to process stuff that
- * might already be in the buffer.
 */
 typedef bool (*pa_srbchannel_cb_t)(pa_srbchannel *sr, void *userdata);
 void pa_srbchannel_set_callback(pa_srbchannel *sr, pa_srbchannel_cb_t callback, void *userdata);
-- 
1.9.1