[pulseaudio-discuss] [PATCHv2 32/60] bluetooth: Implement org.bluez.MediaEndpoint1.ClearConfiguration()

[Tanu Kaskinen]

[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
> <tanu.kaskinen at linux.intel.com> 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
> >> <tanu.kaskinen at linux.intel.com> wrote:
> >> > On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote:
> >> >> From: João Paulo Rechi Vita <jprvita at openbossa.org>
> >> >>
> >> >> ---
> >> >>  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