[Added pulseaudio-discuss back to CC.] On Mon, 2013-09-09 at 09:30 -0300, João Paulo Rechi Vita wrote: > On Wed, Sep 4, 2013 at 7:02 PM, Tanu Kaskinen > <[email protected]> wrote: > > On Wed, 2013-09-04 at 15:17 -0300, João Paulo Rechi Vita wrote: > >> On Sun, Aug 18, 2013 at 8:15 AM, Tanu Kaskinen > >> <[email protected]> wrote: > >> > On Tue, 2013-08-13 at 01:54 -0300, [email protected] wrote: > >> >> From: João Paulo Rechi Vita <[email protected]> > >> >> > >> >> --- > >> >> src/modules/bluetooth/bluez5-util.c | 28 ++++++++++++++++++++++++++-- > >> >> 1 file changed, 26 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/src/modules/bluetooth/bluez5-util.c > >> >> b/src/modules/bluetooth/bluez5-util.c > >> >> index 1194503..1d98174 100644 > >> >> --- a/src/modules/bluetooth/bluez5-util.c > >> >> +++ b/src/modules/bluetooth/bluez5-util.c > >> >> @@ -697,11 +697,35 @@ fail: > >> >> } > >> >> > >> >> static DBusMessage *endpoint_clear_configuration(DBusConnection *conn, > >> >> DBusMessage *m, void *userdata) { > >> >> + pa_bluetooth_discovery *y = userdata; > >> >> + pa_bluetooth_transport *t; > >> >> DBusMessage *r; > >> >> + DBusError err; > >> >> + const char *path; > >> >> > >> >> - pa_assert_se(r = dbus_message_new_error(m, > >> >> BLUEZ_MEDIA_ENDPOINT_INTERFACE ".Error.NotImplemented", > >> >> - "Method not implemented")); > >> >> + dbus_error_init(&err); > >> >> > >> >> + if (!dbus_message_get_args(m, &err, DBUS_TYPE_OBJECT_PATH, &path, > >> >> DBUS_TYPE_INVALID)) { > >> >> + pa_log_error("Endpoint ClearConfiguration(): %s", err.message); > >> >> + dbus_error_free(&err); > >> >> + goto fail; > >> >> + } > >> >> + > >> >> + if ((t = pa_hashmap_get(y->transports, path))) { > >> >> + pa_log_debug("Clearing transport %s profile %s", t->path, > >> >> pa_bluetooth_profile_to_string(t->profile)); > >> >> + transport_state_changed(t, > >> >> PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED); > >> >> + t->device->transports[t->profile] = NULL; > >> >> + /* TODO: check if there is no risk of calling > >> >> + * transport_connection_changed_cb() when the transport is > >> >> already freed */ > >> >> + pa_bluetooth_transport_free(t); > >> > > >> > Similar to what I said about device_free(), the core code shouldn't call > >> > pa_bluetooth_transport_free() here either. I think > >> > pa_bluetooth_transport should have a configuration_cleared() callback, > >> > and the transport backend can then call pa_bluetooth_transport_free() if > >> > it wants. > >> > > >> > >> All endpoint methods on the D-Bus interface are specific to the BlueZ > >> transport-backend, so it's not the core calling > >> pa_bluetooth_transport_free(). If we ever decide to separate the BlueZ > >> 5 transport backend to a different file, the endpoint functions should > >> be moved altogether. > > > > OK. What would you think about moving > > > > t->device->transports[t->profile] = NULL; > > > > to transport_state_changed()? Writing to pa_bluetooth_device variables > > should ideally be done by the core, so if endpoint_clear_configuration() > > is backend code, then this line is likely in the wrong place. > > > > The problem is that it can't be assumed the every time the transport > state changes to PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED means we > should remove it. And the same problem exists when a new transport is > created through endpoint_set_configuration().
Why can't that be assumed? Unless I missed something, the current code always frees the transport object when the transport state changes to DISCONNECTED. -- Tanu _______________________________________________ pulseaudio-discuss mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
