Hi Arman,
On Thu, Jan 8, 2015 at 3:48 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
> This patch fixes a recently introduced issue which changed the behavior
> of profile probing for GATT-based profiles to create a btd_service
> instance for all discovered GATT services to be on a per-UUID basis
> rather than per-service.
> ---
> src/device.c | 176 ++++++++++++++++++++++-------------------------------------
> 1 file changed, 64 insertions(+), 112 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index 1236698..a9dc22d 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -295,21 +295,15 @@ static GSList *find_service_with_state(GSList *list,
> return NULL;
> }
>
> -static GSList *find_service_with_gatt_handles(GSList *list,
> - uint16_t start_handle,
> - uint16_t end_handle)
> +static GSList *find_service_with_uuid(GSList *list, char *uuid)
> {
> GSList *l;
> - uint16_t svc_start, svc_end;
>
> for (l = list; l != NULL; l = g_slist_next(l)) {
> struct btd_service *service = l->data;
> + struct btd_profile *profile = btd_service_get_profile(service);
>
> - if (!btd_service_get_gatt_handles(service, &svc_start,
> - &svc_end))
> - continue;
> -
> - if (svc_start == start_handle && svc_end == end_handle)
> + if (bt_uuid_strcmp(profile->remote_uuid, uuid) == 0)
> return l;
> }
>
> @@ -2420,6 +2414,7 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
> static void device_add_uuids(struct btd_device *device, GSList *uuids)
> {
> GSList *l;
> + bool changed = false;
>
> for (l = uuids; l != NULL; l = g_slist_next(l)) {
> GSList *match = g_slist_find_custom(device->uuids, l->data,
> @@ -2427,12 +2422,14 @@ static void device_add_uuids(struct btd_device *device, GSList *uuids)
> if (match)
> continue;
>
> + changed = true;
> device->uuids = g_slist_insert_sorted(device->uuids,
> g_strdup(l->data),
> bt_uuid_strcmp);
> }
>
> - g_dbus_emit_property_changed(dbus_conn, device->path,
> + if (changed)
> + g_dbus_emit_property_changed(dbus_conn, device->path,
> DEVICE_INTERFACE, "UUIDs");
> }
>
> @@ -2444,8 +2441,6 @@ struct gatt_probe_data {
> GSList *uuids;
> struct gatt_db_attribute *cur_attr;
> char cur_uuid[MAX_LEN_UUID_STR];
> - uint16_t start_handle, end_handle;
> - GSList *valid_services;
> };
>
> static bool device_match_profile(struct btd_device *device,
> @@ -2473,8 +2468,7 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data)
> if (!p->remote_uuid || bt_uuid_strcmp(p->remote_uuid, data->cur_uuid))
> return;
>
> - service = service_create_gatt(data->dev, p, data->start_handle,
> - data->end_handle);
> + service = service_create(data->dev, p);
> if (!service)
> return;
>
> @@ -2487,72 +2481,6 @@ static void dev_probe_gatt(struct btd_profile *p, void *user_data)
> gatt_db_service_set_active(data->cur_attr, false);
>
> data->dev->services = g_slist_append(data->dev->services, service);
> -
> - if (data->all_services)
> - data->valid_services = g_slist_append(data->valid_services,
> - service);
> -}
> -
> -static bool match_existing_service(struct gatt_probe_data *data)
> -{
> - struct btd_device *dev = data->dev;
> - struct btd_service *service;
> - struct btd_profile *profile;
> - uint16_t start, end;
> - GSList *l, *tmp;
> -
> - /*
> - * Check if the profiles should be probed for the service in the
> - * database.
> - */
> - for (l = dev->services; l != NULL;) {
> - service = l->data;
> - profile = btd_service_get_profile(service);
> -
> - /* If this is local or non-GATT based service, then skip. */
> - if (!profile->remote_uuid ||
> - !btd_service_get_gatt_handles(service, &start, &end)) {
> - l = g_slist_next(l);
> - continue;
> - }
> -
> - /* Skip this service if the handle ranges don't overlap. */
> - if (start > data->end_handle || end < data->start_handle) {
> - l = g_slist_next(l);
> - continue;
> - }
> -
> - /*
> - * If there is an exact match, then there's no need to probe the
> - * profiles. An exact match is when the service handles AND the
> - * service UUID match.
> - */
> - if (start == data->start_handle && end == data->end_handle &&
> - !bt_uuid_strcmp(profile->remote_uuid, data->cur_uuid)) {
> - if (data->all_services)
> - data->valid_services = g_slist_append(
> - data->valid_services, service);
> - return true;
> - }
> -
> - /*
> - * The handles overlap but there is no exact match. This means
> - * that the service is no longer valid. Remove it.
> - *
> - * TODO: The following code is fairly inefficient, especially
> - * when we consider all the extra searches that we're already
> - * doing. Consider changing the services list to a GList.
> - */
> - tmp = l->next;
> - dev->services = g_slist_delete_link(dev->services, l);
> - dev->pending = g_slist_remove(dev->pending, service);
> - service_remove(service);
> -
> - l = tmp;
> - }
> -
> - /* No match found */
> - return false;
> }
>
> static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
> @@ -2562,22 +2490,27 @@ static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
> bt_uuid_t uuid;
> GSList *l = NULL;
>
> - gatt_db_attribute_get_service_data(attr, &data->start_handle,
> - &data->end_handle, NULL,
> - &uuid);
> + gatt_db_attribute_get_service_uuid(attr, &uuid);
> bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
>
> data->cur_attr = attr;
>
> - /* Don't probe the profiles if a matching service already exists. */
> - if (!match_existing_service(data))
> - btd_profile_foreach(dev_probe_gatt, data);
> -
> - if (data->all_services) {
> + /*
> + * If we're probing for all services, store the UUID since device->uuids
> + * was cleared.
> + */
> + if (data->all_services)
> data->uuids = g_slist_append(data->uuids,
> g_strdup(data->cur_uuid));
> +
> + /* Don't probe the profiles if a matching service already exists. */
> + if (find_service_with_uuid(data->dev->services, data->cur_uuid))
> + return;
> +
> + btd_profile_foreach(dev_probe_gatt, data);
> +
> + if (data->all_services)
> return;
> - }
>
> l = g_slist_append(l, g_strdup(data->cur_uuid));
> device_add_uuids(data->dev, l);
> @@ -2600,12 +2533,15 @@ static void remove_invalid_services(struct gatt_probe_data *data)
> {
> struct btd_device *dev = data->dev;
> struct btd_service *service;
> + struct btd_profile *profile;
> GSList *l, *tmp;
>
> for (l = dev->services; l != NULL;) {
> service = l->data;
> + profile = btd_service_get_profile(service);
>
> - if (g_slist_find(data->valid_services, service)) {
> + if (g_slist_find_custom(dev->uuids, profile->remote_uuid,
> + bt_uuid_strcmp)) {
> l = g_slist_next(l);
> continue;
> }
> @@ -2639,11 +2575,15 @@ static void device_probe_gatt_profiles(struct btd_device *device)
>
> gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
> &data);
> +
> + /* Clear the UUIDs list */
> + g_slist_free_full(device->uuids, g_free);
> + device->uuids = NULL;
> +
This would actually cause duplicated UUIDs to be emitted, this
probably won't work properly for dual-mode devices either since the
whole list is cleared, also it is probably better to use a list to
track the removed ones so you don't need to do a double lookup at the
end, to do that copy the uuids list at the start and remove them once
they are found (we might be better off with a different UUID list for
Gatt based profiles in this case) which mean what remains at the must
have been removed.
> device_add_uuids(device, data.uuids);
> g_slist_free_full(data.uuids, g_free);
>
> remove_invalid_services(&data);
> - g_slist_free(data.valid_services);
> }
>
> static void device_accept_gatt_profiles(struct btd_device *device)
> @@ -2657,13 +2597,15 @@ static void device_accept_gatt_profiles(struct btd_device *device)
> static void device_remove_gatt_profile(struct btd_device *device,
> struct gatt_db_attribute *attr)
> {
> - uint16_t start, end;
> struct btd_service *service;
> + bt_uuid_t uuid;
> + char uuid_str[MAX_LEN_UUID_STR];
> GSList *l;
>
> - gatt_db_attribute_get_service_handles(attr, &start, &end);
> + gatt_db_attribute_get_service_uuid(attr, &uuid);
> + bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
>
> - l = find_service_with_gatt_handles(device->services, start, end);
> + l = find_service_with_uuid(device->services, uuid_str);
> if (!l)
> return;
>
> @@ -2677,13 +2619,16 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
> {
> struct btd_device *device = user_data;
> GSList *new_service = NULL;
> + bt_uuid_t uuid;
> + char uuid_str[MAX_LEN_UUID_STR];
> uint16_t start, end;
> GSList *l;
>
> if (!bt_gatt_client_is_ready(device->client))
> return;
>
> - gatt_db_attribute_get_service_handles(attr, &start, &end);
> + gatt_db_attribute_get_service_data(attr, &start, &end, NULL, &uuid);
> + bt_uuid_to_string(&uuid, uuid_str, sizeof(uuid_str));
>
> DBG("start: 0x%04x, end: 0x%04x", start, end);
>
> @@ -2695,14 +2640,19 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
> if (!new_service)
> return;
>
> - device_register_primaries(device, new_service, -1);
> + l = find_service_with_uuid(device->services, uuid_str);
>
> - device_probe_gatt_profile(device, attr);
> + device_register_primaries(device, new_service, -1);
>
> - l = find_service_with_gatt_handles(device->services, start, end);
> - if (l)
> + /*
> + * If the profile was probed for the first time then call accept on
> + * the service.
> + */
> + if (!l && (l = find_service_with_uuid(device->services, uuid_str)))
> service_accept(l->data);
>
> + device_probe_gatt_profile(device, attr);
> +
> store_device_info(device);
>
> btd_gatt_client_service_added(device->client_dbus, attr);
> @@ -2750,24 +2700,26 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
> prim = l->data;
> device->primaries = g_slist_delete_link(device->primaries, l);
>
> - /*
> - * If this happend since the db was cleared for a non-bonded device,
> - * then don't remove the btd_service just yet. We do this so that we can
> - * avoid re-probing the profile if the same GATT service is found on the
> - * device on re-connection. However, if the device is marked as
> - * temporary, then we remove it anyway.
> - */
> - if (device->client || device->temporary == TRUE)
> - device_remove_gatt_profile(device, attr);
> + /* Get the UUID entry to be removed below */
> + l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
>
> /*
> - * Remove the corresponding UUIDs entry, only if this is the last
> - * service with this UUID.
> + * Remove the corresponding UUIDs entry and the profile, only if this
> + * is the last service with this UUID.
> */
> - l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
> -
> if (!g_slist_find_custom(device->primaries, prim->uuid,
> prim_uuid_cmp)) {
> + /*
> + * If this happened since the db was cleared for a non-bonded
> + * device, then don't remove the btd_service just yet. We do
> + * this so that we can avoid re-probing the profile if the same
> + * GATT service is found on the device on re-connection.
> + * However, if the device is marked as temporary, then we remove
> + * it anyway.
> + */
> + if (device->client || device->temporary == TRUE)
> + device_remove_gatt_profile(device, attr);
> +
> g_free(l->data);
> device->uuids = g_slist_delete_link(device->uuids, l);
> g_dbus_emit_property_changed(dbus_conn, device->path,
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
