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
