Well, refcounting is not completely removed from sinks and sources,
because they are still subclasses of pa_object, which implies
refcounting, but the only ones that have a reference to a sink or
source are the sink and source implementors themselves, so in practice
there's no recounting being used. (Actually, pa_asyncmsgq also holds
a reference to a sink or source when the IO thread sends a message to
the sink or source. To avoid problems with this, the messages have to
be dispatched using pa_thread_mq_done() after shutting down the IO
thread, but before unreffing the sink or source.)
This should fix an issue reported by Luiz von Dentz: freeing the
profiles hashmap of pa_device_port accessed already freed memory,
because the hashmap uses the string stored at pa_card_profile.name
as the key, and the profiles are freed when cards are freed. Card
ports might not be freed at that time, because they are (were)
refcounted.
I first tried to fix this by removing the profiles from
pa_device_port.profiles when the profiles are removed, but there are
more things that would have to be updated too on a port when it's not
connected to a card anymore, notably the card pointer. It looked like
the card pointer could not be safely set to NULL without a significant
amount of work, and since I dislike refcounting, I decided to spend
the effort on removing the refcounting from ports instead. That
required removing it also from sinks and sources.
Ports are now owned solely by cards, except if a sink or source
doesn't belong to a card, in which case the sink or source owns its
ports. Currently there aren't any instances where a sink or source
would have port but not be a part of a card, though. So, when a card
is removed, all its sinks, sources and ports are removed too. To avoid
stale pointers, other code is expected to use the card/sink/source
unlink hooks to drop references before the objects get freed.
A note about the removed pa_sink_ref() and pa_source_ref() calls in
module-bluetooth-device: things probably break badly if the SCO sink
or source is removed while being in the "SCO over PCM" mode, but I
don't think that mode has ever worked properly if the SCO sink or
source can disappear at runtime. On the systems that I know are
using the "SCO over PCM" mode, the SCO devices stay loaded all the
time. If this seems unacceptable, I can add some code to
module-bluetooth-device to better track the SCO sink and source
existence.
---
src/modules/alsa/alsa-mixer.c | 4 +---
src/modules/alsa/alsa-ucm.c | 4 +---
src/modules/bluetooth/module-bluetooth-device.c | 8 +-------
src/modules/dbus/iface-device.c | 13 +++++--------
src/modules/module-suspend-on-idle.c | 9 ++-------
src/pulsecore/card.c | 13 +++++++++----
src/pulsecore/device-port.c | 15 +++------------
src/pulsecore/device-port.h | 5 +----
src/pulsecore/sink.c | 20 ++------------------
src/pulsecore/source.c | 4 ++--
10 files changed, 27 insertions(+), 68 deletions(-)
diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 97f25af..a47b810 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -4473,10 +4473,8 @@ static pa_device_port* device_port_alsa_init(pa_hashmap *ports,
if (cp)
pa_hashmap_put(p->profiles, cp->name, cp);
- if (extra) {
+ if (extra)
pa_hashmap_put(extra, p->name, p);
- pa_device_port_ref(p);
- }
return p;
}
diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
index e01721f..61595e0 100644
--- a/src/modules/alsa/alsa-ucm.c
+++ b/src/modules/alsa/alsa-ucm.c
@@ -713,10 +713,8 @@ static void ucm_add_port_combination(
pa_hashmap_put(port->profiles, cp->name, cp);
}
- if (hash) {
+ if (hash)
pa_hashmap_put(hash, port->name, port);
- pa_device_port_ref(port);
- }
}
static int ucm_port_contains(const char *port_name, const char *dev_name, bool is_sink) {
diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
index 06fe0be..33b4b16 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -1548,13 +1548,11 @@ static void connect_ports(struct userdata *u, void *sink_or_source_new_data, pa_
case PROFILE_A2DP:
pa_assert_se(port = pa_hashmap_get(u->card->ports, "bluetooth-output"));
pa_assert_se(pa_hashmap_put(data.sink_new_data->ports, port->name, port) >= 0);
- pa_device_port_ref(port);
break;
case PROFILE_A2DP_SOURCE:
pa_assert_se(port = pa_hashmap_get(u->card->ports, "a2dp-input"));
pa_assert_se(pa_hashmap_put(data.source_new_data->ports, port->name, port) >= 0);
- pa_device_port_ref(port);
break;
case PROFILE_HSP:
@@ -1565,7 +1563,6 @@ static void connect_ports(struct userdata *u, void *sink_or_source_new_data, pa_
pa_assert_se(port = pa_hashmap_get(u->card->ports, "hsp-input"));
pa_assert_se(pa_hashmap_put(data.source_new_data->ports, port->name, port) >= 0);
}
- pa_device_port_ref(port);
break;
case PROFILE_HFGW:
@@ -1576,7 +1573,6 @@ static void connect_ports(struct userdata *u, void *sink_or_source_new_data, pa_
pa_assert_se(port = pa_hashmap_get(u->card->ports, "hfgw-input"));
pa_assert_se(pa_hashmap_put(data.source_new_data->ports, port->name, port) >= 0);
}
- pa_device_port_ref(port);
break;
default:
@@ -2023,8 +2019,6 @@ static int start_thread(struct userdata *u) {
return -1;
}
- pa_sink_ref(u->sink);
- pa_source_ref(u->source);
/* FIXME: monitor stream_fd error */
return 0;
}
@@ -2432,7 +2426,7 @@ static pa_hook_result_t uuid_added_cb(pa_bluetooth_discovery *y, const struct pa
pa_card_add_ports(u->card, new_ports);
- pa_hashmap_free(new_ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(new_ports, NULL);
return PA_HOOK_OK;
}
diff --git a/src/modules/dbus/iface-device.c b/src/modules/dbus/iface-device.c
index 888d407..bc8255f 100644
--- a/src/modules/dbus/iface-device.c
+++ b/src/modules/dbus/iface-device.c
@@ -1208,7 +1208,7 @@ pa_dbusiface_device *pa_dbusiface_device_new_sink(pa_dbusiface_core *core, pa_si
d = pa_xnew0(pa_dbusiface_device, 1);
d->core = core;
- d->sink = pa_sink_ref(sink);
+ d->sink = sink;
d->type = PA_DEVICE_TYPE_SINK;
d->path = pa_sprintf_malloc("%s/%s%u", PA_DBUS_CORE_OBJECT_PATH, SINK_OBJECT_NAME, sink->index);
d->volume = *pa_sink_get_volume(sink, FALSE);
@@ -1242,7 +1242,7 @@ pa_dbusiface_device *pa_dbusiface_device_new_source(pa_dbusiface_core *core, pa_
d = pa_xnew0(pa_dbusiface_device, 1);
d->core = core;
- d->source = pa_source_ref(source);
+ d->source = source;
d->type = PA_DEVICE_TYPE_SOURCE;
d->path = pa_sprintf_malloc("%s/%s%u", PA_DBUS_CORE_OBJECT_PATH, SOURCE_OBJECT_NAME, source->index);
d->volume = *pa_source_get_volume(source, FALSE);
@@ -1271,14 +1271,11 @@ void pa_dbusiface_device_free(pa_dbusiface_device *d) {
pa_assert_se(pa_dbus_protocol_remove_interface(d->dbus_protocol, d->path, device_interface_info.name) >= 0);
- if (d->type == PA_DEVICE_TYPE_SINK) {
+ if (d->type == PA_DEVICE_TYPE_SINK)
pa_assert_se(pa_dbus_protocol_remove_interface(d->dbus_protocol, d->path, sink_interface_info.name) >= 0);
- pa_sink_unref(d->sink);
-
- } else {
+ else
pa_assert_se(pa_dbus_protocol_remove_interface(d->dbus_protocol, d->path, source_interface_info.name) >= 0);
- pa_source_unref(d->source);
- }
+
pa_hashmap_free(d->ports, (pa_free_cb_t) pa_dbusiface_device_port_free);
pa_proplist_free(d->proplist);
pa_dbus_protocol_unref(d->dbus_protocol);
diff --git a/src/modules/module-suspend-on-idle.c b/src/modules/module-suspend-on-idle.c
index 02a6b91..2325308 100644
--- a/src/modules/module-suspend-on-idle.c
+++ b/src/modules/module-suspend-on-idle.c
@@ -344,8 +344,8 @@ static pa_hook_result_t device_new_hook_cb(pa_core *c, pa_object *o, struct user
d = pa_xnew(struct device_info, 1);
d->userdata = u;
- d->source = source ? pa_source_ref(source) : NULL;
- d->sink = sink ? pa_sink_ref(sink) : NULL;
+ d->source = source;
+ d->sink = sink;
d->time_event = pa_core_rttime_new(c, PA_USEC_INVALID, timeout_cb, d);
pa_hashmap_put(u->device_infos, o, d);
@@ -359,11 +359,6 @@ static pa_hook_result_t device_new_hook_cb(pa_core *c, pa_object *o, struct user
static void device_info_free(struct device_info *d) {
pa_assert(d);
- if (d->source)
- pa_source_unref(d->source);
- if (d->sink)
- pa_sink_unref(d->sink);
-
d->userdata->core->mainloop->time_free(d->time_event);
pa_xfree(d);
diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index c05fc7a..aa7f58a 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -125,12 +125,14 @@ void pa_card_new_data_done(pa_card_new_data *data) {
pa_proplist_free(data->proplist);
+ /* Ports must be freed before profiles, because the profiles hashmap in
+ * pa_device_port uses profile names as keys. */
+ if (data->ports)
+ pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_free);
+
if (data->profiles)
pa_hashmap_free(data->profiles, (pa_free_cb_t) pa_card_profile_free);
- if (data->ports)
- pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_unref);
-
pa_xfree(data->name);
pa_xfree(data->active_profile);
}
@@ -237,7 +239,10 @@ void pa_card_free(pa_card *c) {
pa_assert(pa_idxset_isempty(c->sources));
pa_idxset_free(c->sources, NULL);
- pa_hashmap_free(c->ports, (pa_free_cb_t) pa_device_port_unref);
+ /* Ports must be freed before profiles, because the profiles hashmap in
+ * pa_device_port uses profile names as keys. */
+ if (c->ports)
+ pa_hashmap_free(c->ports, (pa_free_cb_t) pa_device_port_free);
if (c->profiles)
pa_hashmap_free(c->profiles, (pa_free_cb_t) pa_card_profile_free);
diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
index e0f9560..5e5d48a 100644
--- a/src/pulsecore/device-port.c
+++ b/src/pulsecore/device-port.c
@@ -24,10 +24,7 @@
#include "device-port.h"
#include <pulsecore/card.h>
-PA_DEFINE_PUBLIC_CLASS(pa_device_port, pa_object);
-
-void pa_device_port_set_available(pa_device_port *p, pa_port_available_t status)
-{
+void pa_device_port_set_available(pa_device_port *p, pa_port_available_t status) {
pa_core *core;
pa_assert(p);
@@ -48,11 +45,8 @@ void pa_device_port_set_available(pa_device_port *p, pa_port_available_t status)
pa_hook_fire(&core->hooks[PA_CORE_HOOK_PORT_AVAILABLE_CHANGED], p);
}
-static void device_port_free(pa_object *o) {
- pa_device_port *p = PA_DEVICE_PORT(o);
-
+void pa_device_port_free(pa_device_port *p) {
pa_assert(p);
- pa_assert(pa_device_port_refcnt(p) == 0);
if (p->proplist)
pa_proplist_free(p->proplist);
@@ -65,15 +59,12 @@ static void device_port_free(pa_object *o) {
pa_xfree(p);
}
-
pa_device_port *pa_device_port_new(pa_core *c, const char *name, const char *description, size_t extra) {
pa_device_port *p;
pa_assert(name);
- p = PA_DEVICE_PORT(pa_object_new_internal(PA_ALIGN(sizeof(pa_device_port)) + extra, pa_device_port_type_id, pa_device_port_check_type));
- p->parent.free = device_port_free;
-
+ p = pa_xmalloc0(PA_ALIGN(sizeof(pa_device_port)) + extra);
p->name = pa_xstrdup(name);
p->description = pa_xstrdup(description);
p->core = c;
diff --git a/src/pulsecore/device-port.h b/src/pulsecore/device-port.h
index 40306f5..ac41bc0 100644
--- a/src/pulsecore/device-port.h
+++ b/src/pulsecore/device-port.h
@@ -39,7 +39,6 @@ typedef struct pa_device_port pa_device_port;
#include <pulsecore/card.h>
struct pa_device_port {
- pa_object parent; /* Needed for reference counting */
pa_core *core;
pa_card *card;
@@ -58,12 +57,10 @@ struct pa_device_port {
/* .. followed by some implementation specific data */
};
-PA_DECLARE_PUBLIC_CLASS(pa_device_port);
-#define PA_DEVICE_PORT(s) (pa_device_port_cast(s))
-
#define PA_DEVICE_PORT_DATA(d) ((void*) ((uint8_t*) d + PA_ALIGN(sizeof(pa_device_port))))
pa_device_port *pa_device_port_new(pa_core *c, const char *name, const char *description, size_t extra);
+void pa_device_port_free(pa_device_port *p);
/* The port's available status has changed */
void pa_device_port_set_available(pa_device_port *p, pa_port_available_t available);
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index e0039fa..3bbc661 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -141,7 +141,7 @@ void pa_sink_new_data_done(pa_sink_new_data *data) {
pa_proplist_free(data->proplist);
if (data->ports)
- pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(data->ports, data->card ? NULL : (pa_free_cb_t) pa_device_port_free);
pa_xfree(data->name);
pa_xfree(data->active_port);
@@ -742,7 +742,7 @@ static void sink_free(pa_object *o) {
pa_proplist_free(s->proplist);
if (s->ports)
- pa_hashmap_free(s->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(s->ports, s->card ? NULL : (pa_free_cb_t) pa_device_port_free);
pa_xfree(s);
}
@@ -1109,8 +1109,6 @@ void pa_sink_render(pa_sink*s, size_t length, pa_memchunk *result) {
return;
}
- pa_sink_ref(s);
-
if (length <= 0)
length = pa_frame_align(MIX_BUFFER_LENGTH, &s->sample_spec);
@@ -1168,8 +1166,6 @@ void pa_sink_render(pa_sink*s, size_t length, pa_memchunk *result) {
}
inputs_drop(s, info, n, result);
-
- pa_sink_unref(s);
}
/* Called from IO thread context */
@@ -1194,8 +1190,6 @@ void pa_sink_render_into(pa_sink*s, pa_memchunk *target) {
return;
}
- pa_sink_ref(s);
-
length = target->length;
block_size_max = pa_mempool_block_size_max(s->core->mempool);
if (length > block_size_max)
@@ -1253,8 +1247,6 @@ void pa_sink_render_into(pa_sink*s, pa_memchunk *target) {
}
inputs_drop(s, info, n, target);
-
- pa_sink_unref(s);
}
/* Called from IO thread context */
@@ -1278,8 +1270,6 @@ void pa_sink_render_into_full(pa_sink *s, pa_memchunk *target) {
return;
}
- pa_sink_ref(s);
-
l = target->length;
d = 0;
while (l > 0) {
@@ -1292,8 +1282,6 @@ void pa_sink_render_into_full(pa_sink *s, pa_memchunk *target) {
d += chunk.length;
l -= chunk.length;
}
-
- pa_sink_unref(s);
}
/* Called from IO thread context */
@@ -1308,8 +1296,6 @@ void pa_sink_render_full(pa_sink *s, size_t length, pa_memchunk *result) {
pa_assert(!s->thread_info.rewind_requested);
pa_assert(s->thread_info.rewind_nbytes == 0);
- pa_sink_ref(s);
-
pa_sink_render(s, length, result);
if (result->length < length) {
@@ -1325,8 +1311,6 @@ void pa_sink_render_full(pa_sink *s, size_t length, pa_memchunk *result) {
result->length = length;
}
-
- pa_sink_unref(s);
}
/* Called from main thread */
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 972113d..8f0ba68 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -133,7 +133,7 @@ void pa_source_new_data_done(pa_source_new_data *data) {
pa_proplist_free(data->proplist);
if (data->ports)
- pa_hashmap_free(data->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(data->ports, data->card ? NULL : (pa_free_cb_t) pa_device_port_free);
pa_xfree(data->name);
pa_xfree(data->active_port);
@@ -672,7 +672,7 @@ static void source_free(pa_object *o) {
pa_proplist_free(s->proplist);
if (s->ports)
- pa_hashmap_free(s->ports, (pa_free_cb_t) pa_device_port_unref);
+ pa_hashmap_free(s->ports, s->card ? NULL : (pa_free_cb_t) pa_device_port_free);
pa_xfree(s);
}
--
1.7.10.4
