[PATCH 1/3] Advertising data: extract local name

[PATCH 1/3] Advertising data: extract local name

[Bruna Moreira]

Move extract_eir_name() to glib-helper.c file and rename function to
bt_extract_eir_name(). The local name is extracted from the advertising
data.
---
 src/adapter.c     |    9 +++++++++
 src/event.c       |   23 +----------------------
 src/glib-helper.c |   22 ++++++++++++++++++++++
 src/glib-helper.h |    1 +
 4 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 7fb0d3f..0089d6d 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3007,6 +3007,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
 	bdaddr_t bdaddr;
 	gboolean new_dev;
 	int8_t rssi;
+	uint8_t type;
 
 	rssi = *(info->data + info->length);
 	bdaddr = info->bdaddr;
@@ -3023,6 +3024,14 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
 
 	adapter->found_devices = g_slist_sort(adapter->found_devices,
 						(GCompareFunc) dev_rssi_cmp);
+
+	if (info->length) {
+		char *tmp_name = bt_extract_eir_name(info->data, &type);
+		if (tmp_name) {
+			g_free(dev->name);
+			dev->name = tmp_name;
+		}
+	}
 }
 
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
diff --git a/src/event.c b/src/event.c
index 008bbe3..daab71a 100644
--- a/src/event.c
+++ b/src/event.c
@@ -301,27 +301,6 @@ void btd_event_simple_pairing_complete(bdaddr_t *local, bdaddr_t *peer,
 	device_simple_pairing_complete(device, status);
 }
 
-static char *extract_eir_name(uint8_t *data, uint8_t *type)
-{
-	if (!data || !type)
-		return NULL;
-
-	if (data[0] == 0)
-		return NULL;
-
-	*type = data[1];
-
-	switch (*type) {
-	case 0x08:
-	case 0x09:
-		if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
-			return strdup("");
-		return strndup((char *) (data + 2), data[0] - 1);
-	}
-
-	return NULL;
-}
-
 void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
 {
 	struct btd_adapter *adapter;
@@ -410,7 +389,7 @@ void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	} else
 		legacy = TRUE;
 
-	tmp_name = extract_eir_name(data, &name_type);
+	tmp_name = bt_extract_eir_name(data, &name_type);
 	if (tmp_name) {
 		if (name_type == 0x09) {
 			write_device_name(local, peer, tmp_name);
diff --git a/src/glib-helper.c b/src/glib-helper.c
index 9d76626..33668d7 100644
--- a/src/glib-helper.c
+++ b/src/glib-helper.c
@@ -43,6 +43,7 @@
 #include <glib.h>
 
 #include "glib-helper.h"
+#include "sdpd.h"
 
 /* Number of seconds to keep a sdp_session_t in the cache */
 #define CACHE_TIMEOUT 2
@@ -576,3 +577,24 @@ GSList *bt_string2list(const gchar *str)
 
 	return l;
 }
+
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type)
+{
+	if (!data || !type)
+		return NULL;
+
+	if (data[0] == 0)
+		return NULL;
+
+	*type = data[1];
+
+	switch (*type) {
+	case EIR_NAME_SHORT:
+	case EIR_NAME_COMPLETE:
+		if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
+			return strdup("");
+		return strndup((char *) (data + 2), data[0] - 1);
+	}
+
+	return NULL;
+}
diff --git a/src/glib-helper.h b/src/glib-helper.h
index e89c2c6..dfe4123 100644
--- a/src/glib-helper.h
+++ b/src/glib-helper.h
@@ -38,3 +38,4 @@ char *bt_name2string(const char *string);
 int bt_string2uuid(uuid_t *uuid, const char *string);
 gchar *bt_list2string(GSList *list);
 GSList *bt_string2list(const gchar *str);
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type);
-- 
1.7.0.4

[PATCH 2/3] Extract service UUIDs from advertising data

[Bruna Moreira]

Make get_eir_uuids() return a GSList of strings, so it can be reused to
extract UUIDs from LE advertising data. The bt_strlist2array() helper
function was created to convert a GSList into a plain array of strings
(needed to send through D-Bus).
---
 src/adapter.c |   71 ++++++++++++++++++++++++++++++++++++++++++++-------------
 src/adapter.h |    1 +
 2 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 0089d6d..a7e78bc 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -202,6 +202,8 @@ static void dev_info_free(struct remote_dev_info *dev)
 {
 	g_free(dev->name);
 	g_free(dev->alias);
+	g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+	g_slist_free(dev->services);
 	g_free(dev);
 }
 
@@ -2826,11 +2828,27 @@ static void emit_device_found(const char *path, const char *address,
 	g_dbus_send_message(connection, signal);
 }
 
-static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
-							size_t *uuid_count)
+static char **strlist2array(GSList *list)
+{
+	GSList *l;
+	unsigned int i, n;
+	char **array;
+
+	if (list == NULL)
+		return NULL;
+
+	n = g_slist_length(list);
+	array = g_new0(char *, n + 1);
+
+	for (l = list, i = 0; l; l = l->next, i++)
+		array[i] = g_strdup((const gchar *) l->data);
+
+	return array;
+}
+
+static GSList *get_eir_uuids(uint8_t *eir_data, size_t eir_length, GSList *list)
 {
 	uint16_t len = 0;
-	char **uuids;
 	size_t total;
 	size_t uuid16_count = 0;
 	size_t uuid32_count = 0;
@@ -2839,10 +2857,11 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 	uint8_t *uuid32;
 	uint8_t *uuid128;
 	uuid_t service;
+	char *uuid_str;
 	unsigned int i;
 
 	if (eir_data == NULL || eir_length == 0)
-		return NULL;
+		return list;
 
 	while (len < eir_length - 1) {
 		uint8_t field_len = eir_data[0];
@@ -2875,15 +2894,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 
 	/* Bail out if got incorrect length */
 	if (len > eir_length)
-		return NULL;
+		return list;
 
 	total = uuid16_count + uuid32_count + uuid128_count;
-	*uuid_count = total;
 
 	if (!total)
-		return NULL;
-
-	uuids = g_new0(char *, total + 1);
+		return list;
 
 	/* Generate uuids in SDP format (EIR data is Little Endian) */
 	service.type = SDP_UUID16;
@@ -2892,7 +2908,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 
 		val16 = (val16 << 8) + uuid16[0];
 		service.value.uuid16 = val16;
-		uuids[i] = bt_uuid2string(&service);
+		uuid_str = bt_uuid2string(&service);
+		if (g_slist_find_custom(list, uuid_str,
+						(GCompareFunc) strcmp) == NULL)
+			list = g_slist_append(list, uuid_str);
+		else
+			g_free(uuid_str);
 		uuid16 += 2;
 	}
 
@@ -2905,7 +2926,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 			val32 = (val32 << 8) + uuid32[k];
 
 		service.value.uuid32 = val32;
-		uuids[i] = bt_uuid2string(&service);
+		uuid_str = bt_uuid2string(&service);
+		if (g_slist_find_custom(list, uuid_str,
+						(GCompareFunc) strcmp) == NULL)
+			list = g_slist_append(list, uuid_str);
+		else
+			g_free(uuid_str);
 		uuid32 += 4;
 	}
 
@@ -2916,11 +2942,16 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
 		for (k = 0; k < 16; k++)
 			service.value.uuid128.data[k] = uuid128[16 - k - 1];
 
-		uuids[i] = bt_uuid2string(&service);
+		uuid_str = bt_uuid2string(&service);
+		if (g_slist_find_custom(list, uuid_str,
+						(GCompareFunc) strcmp) == NULL)
+			list = g_slist_append(list, uuid_str);
+		else
+			g_free(uuid_str);
 		uuid128 += 16;
 	}
 
-	return uuids;
+	return list;
 }
 
 void adapter_emit_device_found(struct btd_adapter *adapter,
@@ -2934,7 +2965,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	dbus_int16_t rssi = dev->rssi;
 	char *alias;
 	char **uuids = NULL;
-	size_t uuid_count = 0;
+	size_t uuid_count;
 
 	ba2str(&dev->bdaddr, peer_addr);
 	ba2str(&adapter->bdaddr, local_addr);
@@ -2954,8 +2985,16 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	} else
 		alias = g_strdup(dev->alias);
 
-	/* Extract UUIDs from extended inquiry response if any*/
-	uuids = get_eir_uuids(eir_data, eir_length, &uuid_count);
+	/* Extract UUIDs from extended inquiry response if any */
+	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
+	uuid_count = g_slist_length(dev->services);
+
+	if (dev->services) {
+		uuids = strlist2array(dev->services);
+		g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+		g_slist_free(dev->services);
+		dev->services = NULL;
+	}
 
 	emit_device_found(adapter->path, paddr,
 			"Address", DBUS_TYPE_STRING, &paddr,
diff --git a/src/adapter.h b/src/adapter.h
index a744f61..4af69b3 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -71,6 +71,7 @@ struct remote_dev_info {
 	name_status_t name_status;
 	gboolean le;
 	/* LE adv data */
+	GSList *services;
 	uint8_t evt_type;
 	uint8_t bdaddr_type;
 };
-- 
1.7.0.4

[PATCH 3/3] Emit "DeviceFound" signal for LE devices

[Bruna Moreira]

The adapter_emit_device_found() function was modified to emit
DeviceFound signal for LE devices as well.
---
 src/adapter.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index a7e78bc..e376f59 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2974,6 +2974,27 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	if (device)
 		paired = device_is_paired(device);
 
+	/* Extract UUIDs from extended inquiry response if any */
+	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
+	uuid_count = g_slist_length(dev->services);
+
+	if (dev->services)
+		uuids = strlist2array(dev->services);
+
+	if (dev->le) {
+		emit_device_found(adapter->path, paddr,
+				"Address", DBUS_TYPE_STRING, &paddr,
+				"RSSI", DBUS_TYPE_INT16, &rssi,
+				"Name", DBUS_TYPE_STRING, &dev->name,
+				"Paired", DBUS_TYPE_BOOLEAN, &paired,
+				"UUIDs", DBUS_TYPE_ARRAY, &uuids, uuid_count,
+				NULL);
+
+		g_strfreev(uuids);
+
+		return;
+	}
+
 	icon = class_to_icon(dev->class);
 
 	if (!dev->alias) {
@@ -2985,12 +3006,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	} else
 		alias = g_strdup(dev->alias);
 
-	/* Extract UUIDs from extended inquiry response if any */
-	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
-	uuid_count = g_slist_length(dev->services);
-
 	if (dev->services) {
-		uuids = strlist2array(dev->services);
 		g_slist_foreach(dev->services, (GFunc) g_free, NULL);
 		g_slist_free(dev->services);
 		dev->services = NULL;
@@ -3071,6 +3087,10 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
 			dev->name = tmp_name;
 		}
 	}
+
+	/* FIXME: check if other information was changed before emitting the
+	 * signal */
+	adapter_emit_device_found(adapter, dev, info->data, info->length);
 }
 
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
-- 
1.7.0.4

Re: [PATCH 1/3] Advertising data: extract local name

[Johan Hedberg]

Hi Bruna,

On Wed, Nov 17, 2010, Bruna Moreira wrote:
> @@ -3007,6 +3007,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
>  	bdaddr_t bdaddr;
>  	gboolean new_dev;
>  	int8_t rssi;
> +	uint8_t type;
>  
>  	rssi = *(info->data + info->length);
>  	bdaddr = info->bdaddr;
> @@ -3023,6 +3024,14 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
>  
>  	adapter->found_devices = g_slist_sort(adapter->found_devices,
>  						(GCompareFunc) dev_rssi_cmp);
> +
> +	if (info->length) {
> +		char *tmp_name = bt_extract_eir_name(info->data, &type);
> +		if (tmp_name) {
> +			g_free(dev->name);
> +			dev->name = tmp_name;
> +		}
> +	}

Variables should be always declared in the smallest possible scope, so
your new type variable is in the wrong place (it should be declared
inside the if-statement. Since this was the only issue I found with this
patch I fixed it myself and pushed it upstream. Btw, is it really safe
to ignore the type here? What if it's EIR_NAME_SHORT? Wouldn't you then
want to perform full name discovery using e.g. the GAP GATT service
later?

Johan

Re: [PATCH 2/3] Extract service UUIDs from advertising data

[Johan Hedberg]

Hi Bruna,

On Wed, Nov 17, 2010, Bruna Moreira wrote:
> +	/* Extract UUIDs from extended inquiry response if any */
> +	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
> +	uuid_count = g_slist_length(dev->services);
> +
> +	if (dev->services) {
> +		uuids = strlist2array(dev->services);
> +		g_slist_foreach(dev->services, (GFunc) g_free, NULL);
> +		g_slist_free(dev->services);
> +		dev->services = NULL;
> +	}

What's the reason that the get_eir_uuids API is designed so that it can
handle a list which already contains elements before calling the
function? Since you free the list right after creating the uuids array
it seems like this shouldn't happen. Or am I missing something?

Btw, is there something that could be optimized here since it seems like
you're regenerating the uuids array before every signal even though the
set of service might not have changed since the previous one. Maybe you
should track this and only regenerate the array if there has been a
changed from the previous signal. What do you think?

Johan

Re: [PATCH 1/3] Advertising data: extract local name

[Anderson Lizardo]

Hi Johan,

On Thu, Nov 18, 2010 at 8:33 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Variables should be always declared in the smallest possible scope, so
> your new type variable is in the wrong place (it should be declared
> inside the if-statement. Since this was the only issue I found with this
> patch I fixed it myself and pushed it upstream. Btw, is it really safe
> to ignore the type here? What if it's EIR_NAME_SHORT? Wouldn't you then
> want to perform full name discovery using e.g. the GAP GATT service
> later?

The idea is to populate dev->name with some temporary information
available on the advertising data, and later when GATT service
discovery happens (as part of device creation), we get the definitive
name. At the moment, the GATT service discovery is not happening
automatically (but Claudio's patches are a beginning of that, IIRC),
so the advertising information is the only name source for LE devices,
currently.

Of course, we need to make sure that once name resolution through GATT
service discovery is done, any future names in advertising data would
be ignored, but we have not implemented this yet.

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH 2/3] Extract service UUIDs from advertising data

[Anderson Lizardo]

On Thu, Nov 18, 2010 at 8:43 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Bruna,
>
> On Wed, Nov 17, 2010, Bruna Moreira wrote:
>> +     /* Extract UUIDs from extended inquiry response if any */
>> +     dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
>> +     uuid_count = g_slist_length(dev->services);
>> +
>> +     if (dev->services) {
>> +             uuids = strlist2array(dev->services);
>> +             g_slist_foreach(dev->services, (GFunc) g_free, NULL);
>> +             g_slist_free(dev->services);
>> +             dev->services = NULL;
>> +     }
>
> What's the reason that the get_eir_uuids API is designed so that it can
> handle a list which already contains elements before calling the
> function? Since you free the list right after creating the uuids array
> it seems like this shouldn't happen. Or am I missing something?

The list is destroyed here just to keep the old semantics for BR/EDR,
which ignores service UUIDs from previous EIR data.

For LE devices (as you can see on the following 3/3 patch) we do not
destroy the previous list when new advertising data is parsed.
Instead, we "merge" the UUIDs list.

Do you think we can unify semantics of BR/EDR and LE and always merge
the service UUIDs for both EIR and advertising data?

> Btw, is there something that could be optimized here since it seems like
> you're regenerating the uuids array before every signal even though the
> set of service might not have changed since the previous one. Maybe you
> should track this and only regenerate the array if there has been a
> changed from the previous signal. What do you think?

On the current code, the UUIDs array is always created on each EIR
data (inside get_eir_uuids()) and destroyed right after the
DeviceFound signal is emitted. For simplicity, we kept this same
behavior.

I agree we can make some optimization here and avoid heap
allocations/deallocations when UUIDs do not change. One idea might be
to keep the uuids char* array as well as the GSList on the
remote_dev_info struct, and only recreate this array if any new UUIDs
were added to the GSList (this can be identified if the uuidcount has
changed, because we never delete UUIDs). What do you think?

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH 2/3] Extract service UUIDs from advertising data

[Johan Hedberg]

Hi Anderson,

On Thu, Nov 18, 2010, Anderson Lizardo wrote:
> > What's the reason that the get_eir_uuids API is designed so that it can
> > handle a list which already contains elements before calling the
> > function? Since you free the list right after creating the uuids array
> > it seems like this shouldn't happen. Or am I missing something?
> 
> The list is destroyed here just to keep the old semantics for BR/EDR,
> which ignores service UUIDs from previous EIR data.
> 
> For LE devices (as you can see on the following 3/3 patch) we do not
> destroy the previous list when new advertising data is parsed.
> Instead, we "merge" the UUIDs list.

Fair enough. I've pushed the patch upstream now.

> Do you think we can unify semantics of BR/EDR and LE and always merge
> the service UUIDs for both EIR and advertising data?

Yeah, I think that could make sense. Feel free to send a patch for it.

> On the current code, the UUIDs array is always created on each EIR
> data (inside get_eir_uuids()) and destroyed right after the
> DeviceFound signal is emitted. For simplicity, we kept this same
> behavior.
> 
> I agree we can make some optimization here and avoid heap
> allocations/deallocations when UUIDs do not change. One idea might be
> to keep the uuids char* array as well as the GSList on the
> remote_dev_info struct, and only recreate this array if any new UUIDs
> were added to the GSList (this can be identified if the uuidcount has
> changed, because we never delete UUIDs). What do you think?

Yes, that sounds like a good optimization.

Johan

Re: [PATCH 3/3] Emit "DeviceFound" signal for LE devices

[Johan Hedberg]

Hi Bruna,

On Wed, Nov 17, 2010, Bruna Moreira wrote:
> The adapter_emit_device_found() function was modified to emit
> DeviceFound signal for LE devices as well.
> ---
>  src/adapter.c |   30 +++++++++++++++++++++++++-----
>  1 files changed, 25 insertions(+), 5 deletions(-)

This patch has also been pushed upstream. Thanks.

Johan