headers
Deng Zhenrong | 1 Apr 2012 05:12
Picon

[PATCH] proof of concept: use pactl log to get the buffer log

This is just a proof of concept for me to see how the overall
communication works. From this patch, I learnt a lot about the
importances of callback, the tag struct etc. It's a nice start for me to
dive into pulseaudio.  :)

However, there's still a lot to improve:
1. Add thread protection to log facilities. To make it simple at first,
   I haven't added that yet, but in real cases, different threads would
   call the log function, and if protection is not there, all would be
   messed up.
2. reduce the overhead of copying the data buffer. Now, `get_log_buffer'
   allocates a new buffer and pass it to tag struct. But this kind of
   operation can be reduced to keep the overhead at minimal.

Signed-off-by: Deng Zhenrong <dzrongg <at> gmail.com>
---
 src/map-file                    |    1 +
 src/pulse/introspect.c          |   34 +++++++++++++++++++++++
 src/pulse/introspect.h          |    3 ++
 src/pulsecore/log.c             |   58 +++++++++++++++++++++++++++++++++++++++
 src/pulsecore/log.h             |    2 +
 src/pulsecore/native-common.h   |    1 +
 src/pulsecore/pdispatch.c       |    1 +
 src/pulsecore/protocol-native.c |   26 +++++++++++++++++
 src/utils/pactl.c               |   14 +++++++++
 9 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/src/map-file b/src/map-file
index 69cf25b..812875a 100644
--- a/src/map-file
(Continue reading)

Permalink | Reply | 1 comment |
headers
Frédéric Dalleau | 2 Apr 2012 11:16
Picon

[PATCH] loopback: Fix crash on error during init

If an error during pa__init() causes a jump to fail: u->asyncmsgq is not
initialized.
---
 src/modules/module-loopback.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
index ba62453..0d65682 100644
--- a/src/modules/module-loopback.c
+++ b/src/modules/module-loopback.c
 <at>  <at>  -136,7 +136,8  <at>  <at>  static void teardown(struct userdata *u) {
     pa_assert(u);
     pa_assert_ctl_context();

-    pa_asyncmsgq_flush(u->asyncmsgq, 0);
+    if (u->asyncmsgq)
+        pa_asyncmsgq_flush(u->asyncmsgq, 0);

     u->adjust_time = 0;
     if (u->time_event) {
--

-- 
1.7.5.4
Permalink | Reply | 2 comments |
headers
Dalleau, Frederic | 2 Apr 2012 11:46
Picon
Favicon

Re: [PATCH] loopback: Fix crash on error during init

For example, this would crash pa an is fixed by the patch :

pacmd load-module module-loopback sink_input_properties=dummy

2012/4/2 Frédéric Dalleau <frederic.dalleau <at> linux.intel.com>:
> If an error during pa__init() causes a jump to fail: u->asyncmsgq is not
> initialized.

Frédéric
Permalink | Reply | 0 comments |
headers
Tanu Kaskinen | 2 Apr 2012 12:47
Favicon

Re: pa_mainloop_wakeup() broken and useless?

On Fri, 2012-03-30 at 13:33 +0300, Tanu Kaskinen wrote:
> Hi,
> 
> The documentation for pa_mainloop_wakeup() says: "Interrupt a running
> poll (for threaded systems)"
> 
> Indeed, the function is only useful for waking up the mainloop from
> other threads. But is this implementation really thread-safe?
> 
> void pa_mainloop_wakeup(pa_mainloop *m) {
>     char c = 'W';
>     pa_assert(m);
> 
>     if (m->wakeup_pipe[1] >= 0 && m->state == STATE_POLLING) {
>         pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type);
>         m->wakeup_requested++;
>     }
> }
> 
> The code reads m->wakeup_pipe[1] and m->state, which are not volatile or
> atomic. Both the variables have usually so small values that there
> probably aren't any atomicity problems, but I'm wondering about caching
> issues; are they guaranteed to be up-to-date? But even if the data is
> up-to-date in the if condition, when this code is called from a foreign
> thread, I don't think there's any guarantee that m->wakeup_pipe[1] will
> be valid or that m->state is POLLING anymore when pa_write() is called
> (the return value of pa_write() isn't checked, by the way, which was
> flagged by Coverity and which is why I'm reading this code).
> 
> Also, incrementing a boolean variable (wakeup_requested) seems wrong.
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tanu Kaskinen | 2 Apr 2012 14:01
Favicon

[PATCH 0/5] A bunch of pa_mainloop_wakeup() related fixes

Coverity warned about an unchecked pa_write() return value,
which is fixed in the last patch of this series. While
figuring out what pa_mainloop_wakeup() was supposed to do
and how it was called, I noticed some other issues related
to that function.

There's some discussion (so far monologue only) on the
mailing list:
http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-March/013164.html
http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-April/013175.html

Tanu Kaskinen (5):
  mainloop: Change wakeup_requested type from pa_bool_t to pa_atomic_t.
  mainloop: Remove useless pa_mainloop_wakeup() calls.
  mainloop: Remove redundant wakeup_pipe validity checks.
  mainloop: Write to the wakeup pipe unconditionally when waking up the
    mainloop.
  mainloop: Check pa_write() return value.

 src/pulse/mainloop.c |   46 +++++++++++++---------------------------------
 1 files changed, 13 insertions(+), 33 deletions(-)

--

-- 
1.7.8
Permalink | Reply | 9 comments |
headers
Tanu Kaskinen | 2 Apr 2012 14:01
Favicon

[PATCH 1/5] mainloop: Change wakeup_requested type from pa_bool_t to pa_atomic_t.

The variable is accessed from multiple threads, so it should
be atomic.
---
 src/pulse/mainloop.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
index 5c0345e..262e4b3 100644
--- a/src/pulse/mainloop.c
+++ b/src/pulse/mainloop.c
 <at>  <at>  -114,7 +114,7  <at>  <at>  struct pa_mainloop {
     int retval;
     pa_bool_t quit:1;

-    pa_bool_t wakeup_requested:1;
+    pa_atomic_t wakeup_requested;
     int wakeup_pipe[2];
     int wakeup_pipe_type;

 <at>  <at>  -792,7 +792,7  <at>  <at>  void pa_mainloop_wakeup(pa_mainloop *m) {

     if (m->wakeup_pipe[1] >= 0 && m->state == STATE_POLLING) {
         pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type);
-        m->wakeup_requested++;
+        pa_atomic_store(&m->wakeup_requested, TRUE);
     }
 }

 <at>  <at>  -804,10 +804,9  <at>  <at>  static void clear_wakeup(pa_mainloop *m) {
     if (m->wakeup_pipe[0] < 0)
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tanu Kaskinen | 2 Apr 2012 14:01
Favicon

[PATCH 2/5] mainloop: Remove useless pa_mainloop_wakeup() calls.

pa_mainloop_wakeup() is only useful for waking up the
mainloop from another thread. The removed calls were in
functions that can not be safely called from other threads,
so calling pa_mainloop_wakeup() didn't make sense.
---
 src/pulse/mainloop.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
index 262e4b3..53ff7af 100644
--- a/src/pulse/mainloop.c
+++ b/src/pulse/mainloop.c
 <at>  <at>  -197,8 +197,6  <at>  <at>  static pa_io_event* mainloop_io_new(
     m->rebuild_pollfds = TRUE;
     m->n_io_events ++;

-    pa_mainloop_wakeup(m);
-
     return e;
 }

 <at>  <at>  -215,8 +213,6  <at>  <at>  static void mainloop_io_enable(pa_io_event *e, pa_io_event_flags_t events) {
         e->pollfd->events = map_flags_to_libc(events);
     else
         e->mainloop->rebuild_pollfds = TRUE;
-
-    pa_mainloop_wakeup(e->mainloop);
 }

 static void mainloop_io_free(pa_io_event *e) {
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tanu Kaskinen | 2 Apr 2012 14:01
Favicon

[PATCH 3/5] mainloop: Remove redundant wakeup_pipe validity checks.

The pipe is opened when creating the mainloop and closed
when freeing the mainloop. The pipe fds can never be less
than zero.
---
 src/pulse/mainloop.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
index 53ff7af..160ba9c 100644
--- a/src/pulse/mainloop.c
+++ b/src/pulse/mainloop.c
 <at>  <at>  -615,13 +615,11  <at>  <at>  static void rebuild_pollfds(pa_mainloop *m) {
     m->n_pollfds = 0;
     p = m->pollfds;

-    if (m->wakeup_pipe[0] >= 0) {
-        m->pollfds[0].fd = m->wakeup_pipe[0];
-        m->pollfds[0].events = POLLIN;
-        m->pollfds[0].revents = 0;
-        p++;
-        m->n_pollfds++;
-    }
+    m->pollfds[0].fd = m->wakeup_pipe[0];
+    m->pollfds[0].events = POLLIN;
+    m->pollfds[0].revents = 0;
+    p++;
+    m->n_pollfds++;

     PA_LLIST_FOREACH(e, m->io_events) {
         if (e->dead) {
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tanu Kaskinen | 2 Apr 2012 14:01
Favicon

[PATCH 4/5] mainloop: Write to the wakeup pipe unconditionally when waking up the mainloop.

If the mainloop is just about to enter polling, but m->state
is not POLLING yet when some other thread calls
pa_mainloop_wakeup(), the mainloop will not be woken up.
It's safe to write to the wakeup pipe at any time, so let's
just remove the check.
---
 src/pulse/mainloop.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
index 160ba9c..8ac8f06 100644
--- a/src/pulse/mainloop.c
+++ b/src/pulse/mainloop.c
 <at>  <at>  -774,10 +774,8  <at>  <at>  void pa_mainloop_wakeup(pa_mainloop *m) {
     char c = 'W';
     pa_assert(m);

-    if (m->state == STATE_POLLING) {
-        pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type);
-        pa_atomic_store(&m->wakeup_requested, TRUE);
-    }
+    pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type);
+    pa_atomic_store(&m->wakeup_requested, TRUE);
 }

 static void clear_wakeup(pa_mainloop *m) {
--

-- 
1.7.8
Permalink | Reply | 1 comment |
headers
Tanu Kaskinen | 2 Apr 2012 14:01
Favicon

[PATCH 5/5] mainloop: Check pa_write() return value.

---
 src/pulse/mainloop.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
index 8ac8f06..fe2966d 100644
--- a/src/pulse/mainloop.c
+++ b/src/pulse/mainloop.c
 <at>  <at>  -774,7 +774,10  <at>  <at>  void pa_mainloop_wakeup(pa_mainloop *m) {
     char c = 'W';
     pa_assert(m);

-    pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type);
+    if (pa_write(m->wakeup_pipe[1], &c, sizeof(c), &m->wakeup_pipe_type) < 0)
+        /* Not much options for recovering from the error. Let's at least log something. */
+        pa_log("pa_write() failed while trying to wake up the mainloop: %s", pa_cstrerror(errno));
+
     pa_atomic_store(&m->wakeup_requested, TRUE);
 }

--

-- 
1.7.8
Permalink | Reply | 0 comments |

Gmane