On Fri, 30 Jun 2017, at 02:39 AM, Tanu Kaskinen wrote: > This allows us to restore the default device properly when a > hotpluggable device (e.g. a USB sound card) is set as the default, but > unplugged temporarily. Previously we would forget that the unplugged > device was ever set as the default, because we had to set > configured_default_sink to NULL to avoid having a stale pa_sink pointer, > and also because module-default-device-restore couldn't resolve the name > of a currently-unplugged device to a pa_sink pointer. > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=89934 > --- > src/modules/dbus/iface-core.c | 4 +-- > src/modules/module-default-device-restore.c | 30 +++++++++---------- > src/modules/module-switch-on-connect.c | 8 ++--- > src/pulsecore/cli-command.c | 4 +-- > src/pulsecore/core.c | 46 > ++++++++++++++++++----------- > src/pulsecore/core.h | 13 ++++---- > src/pulsecore/protocol-native.c | 4 +-- > src/pulsecore/sink.c | 5 +--- > src/pulsecore/source.c | 5 +--- > 9 files changed, 63 insertions(+), 56 deletions(-) > > diff --git a/src/modules/dbus/iface-core.c > b/src/modules/dbus/iface-core.c > index 3f368ab46..7177455bf 100644 > --- a/src/modules/dbus/iface-core.c > +++ b/src/modules/dbus/iface-core.c > @@ -721,7 +721,7 @@ static void handle_set_fallback_sink(DBusConnection > *conn, DBusMessage *msg, DBu > return; > } > > - pa_core_set_configured_default_sink(c->core, > pa_dbusiface_device_get_sink(fallback_sink)); > + pa_core_set_configured_default_sink(c->core, > pa_dbusiface_device_get_sink(fallback_sink)->name); > > pa_dbus_send_empty_reply(conn, msg); > } > @@ -809,7 +809,7 @@ static void handle_set_fallback_source(DBusConnection > *conn, DBusMessage *msg, D > return; > } > > - pa_core_set_configured_default_source(c->core, > pa_dbusiface_device_get_source(fallback_source)); > + pa_core_set_configured_default_source(c->core, > pa_dbusiface_device_get_source(fallback_source)->name); > > pa_dbus_send_empty_reply(conn, msg); > } > diff --git a/src/modules/module-default-device-restore.c > b/src/modules/module-default-device-restore.c > index 56fee67f8..adf19ff94 100644 > --- a/src/modules/module-default-device-restore.c > +++ b/src/modules/module-default-device-restore.c > @@ -60,7 +60,6 @@ static void load(struct userdata *u) { > pa_log_info("Manually configured default sink, not > overwriting."); > else if ((f = pa_fopen_cloexec(u->sink_filename, "r"))) { > char ln[256] = ""; > - pa_sink *s; > > if (fgets(ln, sizeof(ln)-1, f)) > pa_strip_nl(ln); > @@ -68,11 +67,12 @@ static void load(struct userdata *u) { > > if (!ln[0]) > pa_log_info("No previous default sink setting, ignoring."); > - else if ((s = pa_namereg_get(u->core, ln, PA_NAMEREG_SINK))) { > - pa_core_set_configured_default_sink(u->core, s); > - pa_log_info("Restored default sink '%s'.", ln); > - } else > - pa_log_info("Saved default sink '%s' not existent, not > restoring default sink setting.", ln); > + else if (!pa_namereg_is_valid_name(ln)) > + pa_log_warn("Invalid sink name: %s", ln); > + else { > + pa_log_info("Restoring default sink '%s'.", ln); > + pa_core_set_configured_default_sink(u->core, ln); > + } > > } else if (errno != ENOENT) > pa_log("Failed to load default sink: %s", pa_cstrerror(errno)); > @@ -81,7 +81,6 @@ static void load(struct userdata *u) { > pa_log_info("Manually configured default source, not > overwriting."); > else if ((f = pa_fopen_cloexec(u->source_filename, "r"))) { > char ln[256] = ""; > - pa_source *s; > > if (fgets(ln, sizeof(ln)-1, f)) > pa_strip_nl(ln); > @@ -89,14 +88,15 @@ static void load(struct userdata *u) { > > if (!ln[0]) > pa_log_info("No previous default source setting, > ignoring."); > - else if ((s = pa_namereg_get(u->core, ln, PA_NAMEREG_SOURCE))) { > - pa_core_set_configured_default_source(u->core, s); > - pa_log_info("Restored default source '%s'.", ln); > - } else > - pa_log_info("Saved default source '%s' not existent, not > restoring default source setting.", ln); > + else if (!pa_namereg_is_valid_name(ln)) > + pa_log_warn("Invalid source name: %s", ln); > + else { > + pa_log_info("Restoring default source '%s'.", ln); > + pa_core_set_configured_default_source(u->core, ln); > + } > > } else if (errno != ENOENT) > - pa_log("Failed to load default sink: %s", > pa_cstrerror(errno)); > + pa_log("Failed to load default source: %s", > pa_cstrerror(errno)); > } > > static void save(struct userdata *u) { > @@ -107,7 +107,7 @@ static void save(struct userdata *u) { > > if (u->sink_filename) { > if ((f = pa_fopen_cloexec(u->sink_filename, "w"))) { > - fprintf(f, "%s\n", u->core->default_sink ? > u->core->default_sink->name : ""); > + fprintf(f, "%s\n", u->core->configured_default_sink ? > u->core->configured_default_sink : ""); > fclose(f); > } else > pa_log("Failed to save default sink: %s", > pa_cstrerror(errno)); > @@ -115,7 +115,7 @@ static void save(struct userdata *u) { > > if (u->source_filename) { > if ((f = pa_fopen_cloexec(u->source_filename, "w"))) { > - fprintf(f, "%s\n", u->core->default_source ? > u->core->default_source->name : ""); > + fprintf(f, "%s\n", u->core->configured_default_source ? > u->core->configured_default_source : ""); > fclose(f); > } else > pa_log("Failed to save default source: %s", > pa_cstrerror(errno)); > diff --git a/src/modules/module-switch-on-connect.c > b/src/modules/module-switch-on-connect.c > index e2da7222f..640024e95 100644 > --- a/src/modules/module-switch-on-connect.c > +++ b/src/modules/module-switch-on-connect.c > @@ -77,7 +77,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core > *c, pa_sink *sink, void* > > /* No default sink, nothing to move away, just set the new default > */ > if (!c->default_sink) { > - pa_core_set_configured_default_sink(c, sink); > + pa_core_set_configured_default_sink(c, sink->name); > return PA_HOOK_OK; > } > > @@ -91,7 +91,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core > *c, pa_sink *sink, void* > old_default_sink = c->default_sink; > > /* Actually do the switch to the new sink */ > - pa_core_set_configured_default_sink(c, sink); > + pa_core_set_configured_default_sink(c, sink->name); > > /* Now move all old inputs over */ > if (pa_idxset_size(old_default_sink->inputs) <= 0) { > @@ -143,7 +143,7 @@ static pa_hook_result_t > source_put_hook_callback(pa_core *c, pa_source *source, > > /* No default source, nothing to move away, just set the new default > */ > if (!c->default_source) { > - pa_core_set_configured_default_source(c, source); > + pa_core_set_configured_default_source(c, source->name); > return PA_HOOK_OK; > } > > @@ -157,7 +157,7 @@ static pa_hook_result_t > source_put_hook_callback(pa_core *c, pa_source *source, > old_default_source = c->default_source; > > /* Actually do the switch to the new source */ > - pa_core_set_configured_default_source(c, source); > + pa_core_set_configured_default_source(c, source->name); > > /* Now move all old outputs over */ > if (pa_idxset_size(old_default_source->outputs) <= 0) { > diff --git a/src/pulsecore/cli-command.c b/src/pulsecore/cli-command.c > index 5a632be45..01fea475b 100644 > --- a/src/pulsecore/cli-command.c > +++ b/src/pulsecore/cli-command.c > @@ -1030,7 +1030,7 @@ static int pa_cli_command_sink_default(pa_core *c, > pa_tokenizer *t, pa_strbuf *b > } > > if ((s = pa_namereg_get(c, n, PA_NAMEREG_SINK))) > - pa_core_set_configured_default_sink(c, s); > + pa_core_set_configured_default_sink(c, s->name); > else > pa_strbuf_printf(buf, "Sink %s does not exist.\n", n); > > @@ -1052,7 +1052,7 @@ static int pa_cli_command_source_default(pa_core > *c, pa_tokenizer *t, pa_strbuf > } > > if ((s = pa_namereg_get(c, n, PA_NAMEREG_SOURCE))) > - pa_core_set_configured_default_source(c, s); > + pa_core_set_configured_default_source(c, s->name); > else > pa_strbuf_printf(buf, "Source %s does not exist.\n", n); > return 0; > diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c > index 52e51db1a..e01677d5d 100644 > --- a/src/pulsecore/core.c > +++ b/src/pulsecore/core.c > @@ -214,6 +214,8 @@ static void core_free(pa_object *o) { > > pa_assert(!c->default_source); > pa_assert(!c->default_sink); > + pa_xfree(c->configured_default_source); > + pa_xfree(c->configured_default_sink); > > pa_silence_cache_done(&c->silence_cache); > pa_mempool_unref(c->mempool); > @@ -224,38 +226,46 @@ static void core_free(pa_object *o) { > pa_xfree(c); > } > > -void pa_core_set_configured_default_sink(pa_core *core, pa_sink *sink) { > - pa_sink *old_sink; > +void pa_core_set_configured_default_sink(pa_core *core, const char > *sink) { > + char *old_sink; > > pa_assert(core); > > - old_sink = core->configured_default_sink; > + old_sink = pa_xstrdup(core->configured_default_sink); > > - if (sink == old_sink) > - return; > + if (pa_safe_streq(sink, old_sink)) > + goto finish; > > - core->configured_default_sink = sink; > + pa_xfree(core->configured_default_sink); > + core->configured_default_sink = pa_xstrdup(sink); > pa_log_info("configured_default_sink: %s -> %s", > - old_sink ? old_sink->name : "(unset)", sink ? sink->name > : "(unset)"); > + old_sink ? old_sink : "(unset)", sink ? sink : > "(unset)"); > > pa_core_update_default_sink(core); > + > +finish: > + pa_xfree(old_sink); > } > > -void pa_core_set_configured_default_source(pa_core *core, pa_source > *source) { > - pa_source *old_source; > +void pa_core_set_configured_default_source(pa_core *core, const char > *source) { > + char *old_source; > > pa_assert(core); > > - old_source = core->configured_default_source; > + old_source = pa_xstrdup(core->configured_default_source); > > - if (source == old_source) > - return; > + if (pa_safe_streq(source, old_source)) > + goto finish; > > - core->configured_default_source = source; > + pa_xfree(core->configured_default_source); > + core->configured_default_source = pa_xstrdup(source); > pa_log_info("configured_default_source: %s -> %s", > - old_source ? old_source->name : "(unset)", source ? > source->name : "(unset)"); > + old_source ? old_source : "(unset)", source ? source : > "(unset)"); This is okay for now, but we should probably make this more concise in the future with something like pa_str_safe() or something which returns the string or a default value on null. Looks good, otherwise. -- Arun
