[pulseaudio-discuss] [PATCH 2/7] don't move streams to devices that are going away

[Arun Raghavan]

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