On Wed, 2014-09-10 at 15:10 +0300, Luiz Augusto von Dentz wrote:
> 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.
I'd rather not add the assumption that state == DISCONNECTED means that
unlink() has nothing to do. The assumption will break if we ever add
something to put() that can fail before we set the state to IDLE. In
such case there may be something that put() did before the failure that
unlink() should undo.
Anyway, I won't apply this patch before I'm done with reviewing the v2
of the ofono patches, because if I applied this patch now, then I'd need
to edit the ofono patches (at least the CardRemoved handling patch), and
I prefer to edit this patch instead after the ofono patches are in.
--
Tanu
