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
