[pulseaudio-discuss] [PATCH] pstream: fix revoke callback setting

[Arun Raghavan]

On Wed, 25 May 2016, at 04:57 PM, Tanu Kaskinen wrote:
> While investigating bug 95352, I noticed that
> pa_pstream_set_revoke_callback() and pa_pstream_set_release_callback()
> were identical - both set the release callback.
> pa_pstream_set_revoke_callback() was obviously broken - it was setting
> the wrong callback.
> 
> The only place where set_revoke_callback() is called is in
> protocol-native.c. The code there looks like this:
> 
>     pa_pstream_set_revoke_callback(c->pstream, pstream_revoke_callback,
>     c);
>     pa_pstream_set_release_callback(c->pstream, pstream_release_callback,
>     c);
> 
> Since set_release_callback() is called last, the release callback gets
> set correctly. The only problem is that the revoke callback stays
> unset. What are the consequences of that? The code that calls the
> revoke callback looks like this:
> 
>     if (p->revoke_callback)
>         p->revoke_callback(p, block_id, p->revoke_callback_userdata);
>     else
>         pa_pstream_send_revoke(p, block_id);
> 
> So the intended callback is replaced with a pa_pstream_send_revoke()
> call. What does the intended callback, that doesn't get called, do?
> 
>     if (!(q = pa_thread_mq_get()))
>         pa_pstream_send_revoke(p, block_id);
>     else
>         pa_asyncmsgq_post(q->outq, PA_MSGOBJECT(userdata),
>         CONNECTION_MESSAGE_REVOKE, PA_UINT_TO_PTR(block_id), 0, NULL,
>         NULL);
> 
> So the native protocol's revoke callback is anyway going to call
> pa_pstream_send_revoke() when called from the main thread. If the
> revoking is done from an IO thread, an asynchronous message is sent to
> the main thread instead, and the message handler will then call
> pa_pstream_send_revoke().
> 
> In conclusion, the only effect of this bug was that
> pa_pstream_send_revoke() was sometimes being called from an IO thread
> when it should have been called from the main thread. I don't know if
> this caused the crash in bug 95352. Probably not.
> ---
>  src/pulsecore/pstream.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> index 98a8382..d5200d7 100644
> --- a/src/pulsecore/pstream.c
> +++ b/src/pulsecore/pstream.c
> @@ -994,8 +994,8 @@ void pa_pstream_set_revoke_callback(pa_pstream *p,
> pa_pstream_block_id_cb_t cb,
>      pa_assert(p);
>      pa_assert(PA_REFCNT_VALUE(p) > 0);
>  
> -    p->release_callback = cb;
> -    p->release_callback_userdata = userdata;
> +    p->revoke_callback = cb;
> +    p->revoke_callback_userdata = userdata;
>  }
>  
>  bool pa_pstream_is_pending(pa_pstream *p) {
> -- 

Looks good for next. Thanks for the detailed commit message!

-- Arun