[pulseaudio-discuss] [PATCH v2 04/18] sink, source: Simplify pa_sink/source_new() error handling
Tanu Kaskinen
tanu.kaskinen at linux.intel.com
Fri Jun 28 00:47:58 PDT 2013
Now the only exit from the function is either via "goto fail" or
normal success. I changed pa_hook_fire() return value checking into
assertions, because it's simpler (and I verified that none of the
hooks can ever fail in the current code base). I also changed all
pa_return_null_if_fail() calls into assertions.
Further simplification could be achieved if adding the sinks and
sources to the idxsets in pa_core were moved to _put() and if
unregistering the sink and source names was moved to _free(). Then
_new() wouldn't have to call _unlink() and _unlink() would have fewer
things to worry about.
The pa_sink/pa_source field initialization order was changed somewhat,
because _unlink() and _unref() rely on certain fields to be
initialized, so they had to be initialized before the first "goto
fail" line in _new().
---
src/pulsecore/sink.c | 89 ++++++++++++++++++++++++--------------------------
src/pulsecore/source.c | 66 ++++++++++++++++++-------------------
2 files changed, 73 insertions(+), 82 deletions(-)
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index fae4daf..15237c5 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -185,33 +185,29 @@ pa_sink* pa_sink_new(
pa_assert_ctl_context();
s = pa_msgobject_new(pa_sink);
+ s->state = PA_SINK_INIT;
+ s->parent.parent.free = sink_free;
+ s->parent.process_msg = pa_sink_process_msg;
+ s->core = core;
+ s->inputs = pa_idxset_new(NULL, NULL);
if (!(name = pa_namereg_register(core, data->name, PA_NAMEREG_SINK, s, data->namereg_fail))) {
- pa_log_debug("Failed to register name %s.", data->name);
- pa_xfree(s);
- return NULL;
+ pa_log("Failed to register name %s.", data->name);
+ goto fail;
}
+ s->name = pa_xstrdup(name);
pa_sink_new_data_set_name(data, name);
- if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_SINK_NEW], data) < 0) {
- pa_xfree(s);
- pa_namereg_unregister(core, name);
- return NULL;
- }
-
- /* FIXME, need to free s here on failure */
-
- pa_return_null_if_fail(!data->driver || pa_utf8_valid(data->driver));
- pa_return_null_if_fail(data->name && pa_utf8_valid(data->name) && data->name[0]);
+ pa_assert_se(pa_hook_fire(&core->hooks[PA_CORE_HOOK_SINK_NEW], data) >= 0);
- pa_return_null_if_fail(data->sample_spec_is_set && pa_sample_spec_valid(&data->sample_spec));
+ pa_assert(data->sample_spec_is_set && pa_sample_spec_valid(&data->sample_spec));
if (!data->channel_map_is_set)
- pa_return_null_if_fail(pa_channel_map_init_auto(&data->channel_map, data->sample_spec.channels, PA_CHANNEL_MAP_DEFAULT));
+ pa_assert_se(pa_channel_map_init_auto(&data->channel_map, data->sample_spec.channels, PA_CHANNEL_MAP_DEFAULT));
- pa_return_null_if_fail(pa_channel_map_valid(&data->channel_map));
- pa_return_null_if_fail(data->channel_map.channels == data->sample_spec.channels);
+ pa_assert(pa_channel_map_valid(&data->channel_map));
+ pa_assert(data->channel_map.channels == data->sample_spec.channels);
/* FIXME: There should probably be a general function for checking whether
* the sink volume is allowed to be set, like there is for sink inputs. */
@@ -222,8 +218,8 @@ pa_sink* pa_sink_new(
data->save_volume = FALSE;
}
- pa_return_null_if_fail(pa_cvolume_valid(&data->volume));
- pa_return_null_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec));
+ pa_assert(pa_cvolume_valid(&data->volume));
+ pa_assert(pa_cvolume_compatible(&data->volume, &data->sample_spec));
if (!data->muted_is_set)
data->muted = FALSE;
@@ -235,22 +231,12 @@ pa_sink* pa_sink_new(
pa_device_init_icon(data->proplist, TRUE);
pa_device_init_intended_roles(data->proplist);
- if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_SINK_FIXATE], data) < 0) {
- pa_xfree(s);
- pa_namereg_unregister(core, name);
- return NULL;
- }
+ pa_assert_se(pa_hook_fire(&core->hooks[PA_CORE_HOOK_SINK_FIXATE], data) >= 0);
- s->parent.parent.free = sink_free;
- s->parent.process_msg = pa_sink_process_msg;
-
- s->core = core;
- s->state = PA_SINK_INIT;
s->flags = flags;
s->priority = 0;
s->suspend_cause = data->suspend_cause;
pa_sink_set_mixer_dirty(s, FALSE);
- s->name = pa_xstrdup(name);
s->proplist = pa_proplist_copy(data->proplist);
s->driver = pa_xstrdup(pa_path_get_filename(data->driver));
s->module = data->module;
@@ -272,7 +258,6 @@ pa_sink* pa_sink_new(
s->alternate_sample_rate = 0;
}
- s->inputs = pa_idxset_new(NULL, NULL);
s->n_corked = 0;
s->input_to_master = NULL;
@@ -352,15 +337,6 @@ pa_sink* pa_sink_new(
if (s->card)
pa_assert_se(pa_idxset_put(s->card->sinks, s, NULL) >= 0);
- pt = pa_proplist_to_string_sep(s->proplist, "\n ");
- pa_log_info("Created sink %u \"%s\" with sample spec %s and channel map %s\n %s",
- s->index,
- s->name,
- pa_sample_spec_snprint(st, sizeof(st), &s->sample_spec),
- pa_channel_map_snprint(cm, sizeof(cm), &s->channel_map),
- pt);
- pa_xfree(pt);
-
pa_source_new_data_init(&source_data);
pa_source_new_data_set_sample_spec(&source_data, &s->sample_spec);
pa_source_new_data_set_channel_map(&source_data, &s->channel_map);
@@ -381,9 +357,8 @@ pa_sink* pa_sink_new(
pa_source_new_data_done(&source_data);
if (!s->monitor_source) {
- pa_sink_unlink(s);
- pa_sink_unref(s);
- return NULL;
+ pa_log("Failed to create a monitor source for sink %s.", s->name);
+ goto fail;
}
s->monitor_source->monitor_of = s;
@@ -392,7 +367,22 @@ pa_sink* pa_sink_new(
pa_source_set_fixed_latency(s->monitor_source, s->thread_info.fixed_latency);
pa_source_set_max_rewind(s->monitor_source, s->thread_info.max_rewind);
+ pt = pa_proplist_to_string_sep(s->proplist, "\n ");
+ pa_log_info("Created sink %u \"%s\" with sample spec %s and channel map %s\n %s",
+ s->index,
+ s->name,
+ pa_sample_spec_snprint(st, sizeof(st), &s->sample_spec),
+ pa_channel_map_snprint(cm, sizeof(cm), &s->channel_map),
+ pt);
+ pa_xfree(pt);
+
return s;
+
+fail:
+ pa_sink_unlink(s);
+ pa_sink_unref(s);
+
+ return NULL;
}
/* Called from main context */
@@ -682,8 +672,9 @@ void pa_sink_unlink(pa_sink* s) {
if (linked)
pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_UNLINK], s);
- if (s->state != PA_SINK_UNLINKED)
+ if (s->state != PA_SINK_UNLINKED && s->name)
pa_namereg_unregister(s->core, s->name);
+
pa_idxset_remove_by_data(s->core->sinks, s, NULL);
if (s->card)
@@ -722,15 +713,19 @@ static void sink_free(pa_object *o) {
if (PA_SINK_IS_LINKED(s->state))
pa_sink_unlink(s);
- pa_log_info("Freeing sink %u \"%s\"", s->index, s->name);
+ if (s->state != PA_SINK_INIT)
+ pa_log_info("Freeing sink %u \"%s\"", s->index, s->name);
if (s->monitor_source) {
pa_source_unref(s->monitor_source);
s->monitor_source = NULL;
}
- pa_idxset_free(s->inputs, NULL);
- pa_hashmap_free(s->thread_info.inputs, (pa_free_cb_t) pa_sink_input_unref);
+ if (s->inputs)
+ pa_idxset_free(s->inputs, NULL);
+
+ if (s->thread_info.inputs)
+ pa_hashmap_free(s->thread_info.inputs, (pa_free_cb_t) pa_sink_input_unref);
if (s->silence.memblock)
pa_memblock_unref(s->silence.memblock);
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 18d0311..d49b4a4 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -172,33 +172,29 @@ pa_source* pa_source_new(
pa_assert_ctl_context();
s = pa_msgobject_new(pa_source);
+ s->state = PA_SOURCE_INIT;
+ s->parent.parent.free = source_free;
+ s->parent.process_msg = pa_source_process_msg;
+ s->core = core;
+ s->outputs = pa_idxset_new(NULL, NULL);
if (!(name = pa_namereg_register(core, data->name, PA_NAMEREG_SOURCE, s, data->namereg_fail))) {
- pa_log_debug("Failed to register name %s.", data->name);
- pa_xfree(s);
- return NULL;
+ pa_log("Failed to register name %s.", data->name);
+ goto fail;
}
+ s->name = pa_xstrdup(name);
pa_source_new_data_set_name(data, name);
- if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_SOURCE_NEW], data) < 0) {
- pa_xfree(s);
- pa_namereg_unregister(core, name);
- return NULL;
- }
-
- /* FIXME, need to free s here on failure */
-
- pa_return_null_if_fail(!data->driver || pa_utf8_valid(data->driver));
- pa_return_null_if_fail(data->name && pa_utf8_valid(data->name) && data->name[0]);
+ pa_assert_se(pa_hook_fire(&core->hooks[PA_CORE_HOOK_SOURCE_NEW], data) >= 0);
- pa_return_null_if_fail(data->sample_spec_is_set && pa_sample_spec_valid(&data->sample_spec));
+ pa_assert(data->sample_spec_is_set && pa_sample_spec_valid(&data->sample_spec));
if (!data->channel_map_is_set)
- pa_return_null_if_fail(pa_channel_map_init_auto(&data->channel_map, data->sample_spec.channels, PA_CHANNEL_MAP_DEFAULT));
+ pa_assert_se(pa_channel_map_init_auto(&data->channel_map, data->sample_spec.channels, PA_CHANNEL_MAP_DEFAULT));
- pa_return_null_if_fail(pa_channel_map_valid(&data->channel_map));
- pa_return_null_if_fail(data->channel_map.channels == data->sample_spec.channels);
+ pa_assert(pa_channel_map_valid(&data->channel_map));
+ pa_assert(data->channel_map.channels == data->sample_spec.channels);
/* FIXME: There should probably be a general function for checking whether
* the source volume is allowed to be set, like there is for source outputs. */
@@ -209,8 +205,8 @@ pa_source* pa_source_new(
data->save_volume = FALSE;
}
- pa_return_null_if_fail(pa_cvolume_valid(&data->volume));
- pa_return_null_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec));
+ pa_assert(pa_cvolume_valid(&data->volume));
+ pa_assert(pa_cvolume_compatible(&data->volume, &data->sample_spec));
if (!data->muted_is_set)
data->muted = FALSE;
@@ -222,22 +218,12 @@ pa_source* pa_source_new(
pa_device_init_icon(data->proplist, FALSE);
pa_device_init_intended_roles(data->proplist);
- if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_SOURCE_FIXATE], data) < 0) {
- pa_xfree(s);
- pa_namereg_unregister(core, name);
- return NULL;
- }
+ pa_assert_se(pa_hook_fire(&core->hooks[PA_CORE_HOOK_SOURCE_FIXATE], data) >= 0);
- s->parent.parent.free = source_free;
- s->parent.process_msg = pa_source_process_msg;
-
- s->core = core;
- s->state = PA_SOURCE_INIT;
s->flags = flags;
s->priority = 0;
s->suspend_cause = data->suspend_cause;
pa_source_set_mixer_dirty(s, FALSE);
- s->name = pa_xstrdup(name);
s->proplist = pa_proplist_copy(data->proplist);
s->driver = pa_xstrdup(pa_path_get_filename(data->driver));
s->module = data->module;
@@ -259,7 +245,6 @@ pa_source* pa_source_new(
s->alternate_sample_rate = 0;
}
- s->outputs = pa_idxset_new(NULL, NULL);
s->n_corked = 0;
s->monitor_of = NULL;
s->output_from_master = NULL;
@@ -347,6 +332,12 @@ pa_source* pa_source_new(
pa_xfree(pt);
return s;
+
+fail:
+ pa_source_unlink(s);
+ pa_source_unref(s);
+
+ return NULL;
}
/* Called from main context */
@@ -620,8 +611,9 @@ void pa_source_unlink(pa_source *s) {
if (linked)
pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SOURCE_UNLINK], s);
- if (s->state != PA_SOURCE_UNLINKED)
+ if (s->state != PA_SOURCE_UNLINKED && s->name)
pa_namereg_unregister(s->core, s->name);
+
pa_idxset_remove_by_data(s->core->sources, s, NULL);
if (s->card)
@@ -657,10 +649,14 @@ static void source_free(pa_object *o) {
if (PA_SOURCE_IS_LINKED(s->state))
pa_source_unlink(s);
- pa_log_info("Freeing source %u \"%s\"", s->index, s->name);
+ if (s->state != PA_SOURCE_INIT)
+ pa_log_info("Freeing source %u \"%s\"", s->index, s->name);
+
+ if (s->outputs)
+ pa_idxset_free(s->outputs, NULL);
- pa_idxset_free(s->outputs, NULL);
- pa_hashmap_free(s->thread_info.outputs, (pa_free_cb_t) pa_source_output_unref);
+ if (s->thread_info.outputs)
+ pa_hashmap_free(s->thread_info.outputs, (pa_free_cb_t) pa_source_output_unref);
if (s->silence.memblock)
pa_memblock_unref(s->silence.memblock);
--
1.8.1.2
More information about the pulseaudio-discuss
mailing list