Re: [PATCH BlueZ v2 09/14] core: device: Fix GATT profile probing

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Arman,

On Thu, Jan 8, 2015 at 3:48 AM, Arman Uguray <armansito@chromium.org> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz