On 22 March 2016 at 19:11, Tanu Kaskinen <tanuk at iki.fi> wrote:
> Before a device is unlinked, the unlink hook is fired, and it's
> possible that a routing module tries to move streams to the unlinked
> device in that hook, because it doesn't know that the device is being
> unlinked. Of course, the unlinking is obvious when the code is in an
> unlink hook callback, but it's possible that some other module does
> something in the unlink hook that in turn triggers some other hook,
> and it's this second hook where the routing module may get confused.
> This patch adds an "unlink_requested" flag that is set before the
> unlink hook is fired, and moving streams to a device with that flag
> set is prevented.
>
> This patch is motivated by seeing module-device-manager moving a
> stream to a sink that was being unlinked. It was a complex case where
> an alsa card was changing its profile, while an echo-cancel sink was
> connected to the old alsa sink. module-always-sink loaded a null sink
> in the middle of the profile change, and after a stream had been
> rescued to the null sink, module-device-manager decided to move it
> back to the old alsa sink that was being unlinked. That move made no
> sense, so I came up with this patch.
> ---
I think this is a good solution to the problem. Still need to test
this in a few use cases, but it makes sense to me too.
I also think that it might be nicer to have this be a part of the
state enum rather than a separate bool. What do you think? I'm okay to
make that change quickly if you don't want to, fwiw.
-- Arun
> src/pulsecore/sink-input.c | 3 +++
> src/pulsecore/sink.c | 7 ++++---
> src/pulsecore/sink.h | 6 ++++++
> src/pulsecore/source-output.c | 3 +++
> src/pulsecore/source.c | 5 +++++
> src/pulsecore/source.h | 6 ++++++
> 6 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 8ec63b5..843297f 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -1538,6 +1538,9 @@ bool pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) {
> if (dest == i->sink)
> return true;
>
> + if (dest->unlink_requested)
> + return false;
> +
> if (!pa_sink_input_may_move(i))
> return false;
>
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 0b44fc7..3f1ef72 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -676,9 +676,10 @@ void pa_sink_unlink(pa_sink* s) {
> * reversing pa_sink_put(). It also undoes the registrations
> * already done in pa_sink_new()! */
>
> - /* All operations here shall be idempotent, i.e. pa_sink_unlink()
> - * may be called multiple times on the same sink without bad
> - * effects. */
> + if (s->unlink_requested)
> + return;
> +
> + s->unlink_requested = true;
>
> linked = PA_SINK_IS_LINKED(s->state);
>
> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index 5df109e..b64a666 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -63,6 +63,12 @@ struct pa_sink {
> pa_core *core;
>
> pa_sink_state_t state;
> +
> + /* Set in the beginning of pa_sink_unlink() before setting the sink state
> + * to UNLINKED. The purpose is to prevent moving streams to a sink that is
> + * about to be removed. */
> + bool unlink_requested;
> +
> pa_sink_flags_t flags;
> pa_suspend_cause_t suspend_cause;
>
> diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
> index 9217ad4..6d54ae8 100644
> --- a/src/pulsecore/source-output.c
> +++ b/src/pulsecore/source-output.c
> @@ -1187,6 +1187,9 @@ bool pa_source_output_may_move_to(pa_source_output *o, pa_source *dest) {
> if (dest == o->source)
> return true;
>
> + if (dest->unlink_requested)
> + return false;
> +
> if (!pa_source_output_may_move(o))
> return false;
>
> diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
> index f4b96ab..98374ae 100644
> --- a/src/pulsecore/source.c
> +++ b/src/pulsecore/source.c
> @@ -618,6 +618,11 @@ void pa_source_unlink(pa_source *s) {
> /* See pa_sink_unlink() for a couple of comments how this function
> * works. */
>
> + if (s->unlink_requested)
> + return;
> +
> + s->unlink_requested = true;
> +
> linked = PA_SOURCE_IS_LINKED(s->state);
>
> if (linked)
> diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
> index 19fb41b..91e8674 100644
> --- a/src/pulsecore/source.h
> +++ b/src/pulsecore/source.h
> @@ -64,6 +64,12 @@ struct pa_source {
> pa_core *core;
>
> pa_source_state_t state;
> +
> + /* Set in the beginning of pa_source_unlink() before setting the source
> + * state to UNLINKED. The purpose is to prevent moving streams to a source
> + * that is about to be removed. */
> + bool unlink_requested;
> +
> pa_source_flags_t flags;
> pa_suspend_cause_t suspend_cause;
>
> --
> 2.7.0
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
