[pulseaudio-discuss] [PATCH next v0 6/8] bluetooth: Add transport hashmap to discovery

[Tanu Kaskinen]

On Wed, 2012-12-05 at 17:23 +0100, Mikel Astiz wrote:
> @@ -913,16 +916,10 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us
>  
>          return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>      } else if (dbus_message_is_signal(m, "org.bluez.MediaTransport", "PropertyChanged")) {
> -        pa_bluetooth_device *d;
> -        pa_bluetooth_transport *t = NULL;
> -        void *state = NULL;
> +        pa_bluetooth_transport *t;
>          DBusMessageIter arg_i;
>  
> -        while ((d = pa_hashmap_iterate(y->devices, &state, NULL)))
> -            if ((t = pa_hashmap_get(d->transports, dbus_message_get_path(m))))
> -                break;
> -
> -        if (!t)
> +        if ((t = pa_hashmap_get(y->transports, dbus_message_get_path(m))) == NULL)

Cosmetic: the convention is to use !pointer instead of pointer == NULL.

>              goto fail;
>  
>          if (!dbus_message_iter_init(m, &arg_i)) {
> @@ -1193,6 +1190,7 @@ static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>      if (nrec)
>          t->nrec = nrec;
>      pa_hashmap_put(d->transports, t->path, t);
> +    pa_hashmap_put(y->transports, t->path, t);

The transport is removed from the hashmap in transport_free(), wouldn't
it be better to add the transport to the hashmap in transport_new(), for
symmetry?

Or I would actually prefer to do both adding and removing outside
transport_new() and transport_free(), for the reason that it's not
really the transport object's job to manage the discovery object. Doing
it in transport_new() and transport_free() may save some lines of code,
though, so feel free to disagree about this.

> @@ -1224,14 +1220,11 @@ static DBusMessage *endpoint_clear_configuration(DBusConnection *c, DBusMessage
>          goto fail;
>      }
>  
> -    while ((d = pa_hashmap_iterate(y->devices, &state, NULL))) {
> -        if ((t = pa_hashmap_get(d->transports, path))) {
> -            pa_log_debug("Clearing transport %s profile %d", t->path, t->profile);
> -            pa_hashmap_remove(d->transports, t->path);
> -            pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_REMOVED], NULL);
> -            transport_free(t);
> -            break;
> -        }
> +    if ((t = pa_hashmap_get(y->transports, path)) != NULL) {

The "!= NULL" part can be dropped.

> +        pa_log_debug("Clearing transport %s profile %d", t->path, t->profile);
> +        pa_hashmap_remove(t->device->transports, t->path);
> +        pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_REMOVED], NULL);
> +        transport_free(t);
>      }
>  
>      pa_assert_se(r = dbus_message_new_method_return(m));
> @@ -1475,6 +1468,7 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c) {
>      PA_REFCNT_INIT(y);
>      y->core = c;
>      y->devices = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> +    y->transports = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>      PA_LLIST_HEAD_INIT(pa_dbus_pending, y->pending);
>      pa_hook_init(&y->hook, y);
>      pa_shared_set(c, "bluetooth-discovery", y);
> @@ -1549,6 +1543,11 @@ void pa_bluetooth_discovery_unref(pa_bluetooth_discovery *y) {
>          pa_hashmap_free(y->devices, NULL, NULL);
>      }
>  
> +    if (y->transports) {
> +        pa_assert(pa_hashmap_size(y->transports) == 0);

Cosmetic: there's pa_hashmap_isempty() for this sort of checks.

-- 
Tanu