Hi Tanu,
On Mon, Sep 8, 2014 at 2:30 PM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> This should not have any effect on behaviour. The goal is to align
> with the pattern that I think we should follow:
>
> Object initialization:
> - put() is the place to create references from other objects to the
> newly created object. In this case, adding the transport to
> discovery->transports was moved from new() to put, and adding the
> transport to device->transports was moved from set_state() to
> put().
>
> Object destruction:
> - unlink() undoes put() and removes all references from other objects
> to the object being unlinked. In this case setting the
> device->transports pointer to NULL was moved from set_state() to
> unlink(), and setting the discovery->transports pointer to NULL was
> moved from free() to unlink().
> - free() undoes new(), but also calls unlink() so that object owners
> don't need to remember to call unlink() before free().
> ---
> src/modules/bluetooth/bluez5-util.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index efe87d3..83b78e7 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -149,8 +149,6 @@ pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const
> memcpy(t->config, config, size);
> }
>
> - pa_assert_se(pa_hashmap_put(d->discovery->transports, t->path, t) >= 0);
> -
> return t;
> }
>
> @@ -181,10 +179,6 @@ void pa_bluetooth_transport_set_state(pa_bluetooth_transport *t, pa_bluetooth_tr
> t->path, transport_state_to_string(t->state), transport_state_to_string(state));
>
> t->state = state;
> - if (state == PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
> - t->device->transports[t->profile] = NULL;
> - else
> - t->device->transports[t->profile] = t;
>
> pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED], t);
>
> @@ -193,17 +187,26 @@ void pa_bluetooth_transport_set_state(pa_bluetooth_transport *t, pa_bluetooth_tr
> }
>
> void pa_bluetooth_transport_put(pa_bluetooth_transport *t) {
> + pa_assert(t);
> +
> + t->device->transports[t->profile] = t;
> + pa_assert_se(pa_hashmap_put(t->device->discovery->transports, t->path, t) >= 0);
> pa_bluetooth_transport_set_state(t, PA_BLUETOOTH_TRANSPORT_STATE_IDLE);
> }
>
> void pa_bluetooth_transport_unlink(pa_bluetooth_transport *t) {
> + pa_assert(t);
> +
> pa_bluetooth_transport_set_state(t, PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
> + pa_hashmap_remove(t->device->discovery->transports, t->path);
> + t->device->transports[t->profile] = NULL;
> }
>
> void pa_bluetooth_transport_free(pa_bluetooth_transport *t) {
> pa_assert(t);
>
> - pa_hashmap_remove(t->device->discovery->transports, t->path);
> + pa_bluetooth_transport_unlink(t);
> +
> pa_xfree(t->owner);
> pa_xfree(t->path);
> pa_xfree(t->config);
> @@ -418,7 +421,6 @@ static void device_free(pa_bluetooth_device *d) {
> if (!(t = d->transports[i]))
> continue;
>
> - pa_bluetooth_transport_set_state(t, PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
> pa_bluetooth_transport_free(t);
> }
>
> @@ -1439,7 +1441,6 @@ static DBusMessage *endpoint_clear_configuration(DBusConnection *conn, DBusMessa
>
> 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));
> - pa_bluetooth_transport_set_state(t, PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
> pa_bluetooth_transport_free(t);
> }
>
> --
> 1.9.3
This looks fine to me, perhaps we can check the state before calling
pa_bluetooth_transport_unlink but that is just a minor optimization.
--
Luiz Augusto von Dentz
