[PATCH v2 1/2] Simplify eir_parse function

[PATCH v2 1/2] Simplify eir_parse function

[Frédéric Danis]

---
 src/eir.c |  155 +++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 80 insertions(+), 75 deletions(-)

diff --git a/src/eir.c b/src/eir.c
index e82d30b..f188031 100644
--- a/src/eir.c
+++ b/src/eir.c
@@ -54,24 +54,72 @@
 void eir_data_free(struct eir_data *eir)
 {
 	g_slist_free_full(eir->services, g_free);
+	eir->services = NULL;
 	g_free(eir->name);
+	eir->name = NULL;
 }
 
-int eir_parse(struct eir_data *eir, uint8_t *eir_data)
+static void eir_parse_uuid16(struct eir_data *eir, uint8_t *data, uint8_t len)
 {
-	uint16_t len = 0;
-	size_t total;
-	size_t uuid16_count = 0;
-	size_t uuid32_count = 0;
-	size_t uuid128_count = 0;
-	uint8_t *uuid16 = NULL;
-	uint8_t *uuid32 = NULL;
-	uint8_t *uuid128 = NULL;
+	uint8_t *uuid_ptr = data;
+	uuid_t service;
+	char *uuid_str;
+	unsigned int i;
+	uint16_t val16;
+
+	service.type = SDP_UUID16;
+	for (i = 0; i < len / 2; i++) {
+		val16 = uuid_ptr[1];
+		val16 = (val16 << 8) + uuid_ptr[0];
+		service.value.uuid16 = val16;
+		uuid_str = bt_uuid2string(&service);
+		eir->services = g_slist_append(eir->services, uuid_str);
+		uuid_ptr += 2;
+	}
+}
+
+static void eir_parse_uuid32(struct eir_data *eir, uint8_t *data, uint8_t len)
+{
+	uint8_t *uuid_ptr = data;
 	uuid_t service;
 	char *uuid_str;
-	const char *name = NULL;
-	size_t name_len;
 	unsigned int i;
+	uint32_t val32;
+	int k;
+
+	service.type = SDP_UUID32;
+	for (i = 0; i < len / 4; i++) {
+		val32 = uuid_ptr[3];
+		for (k = 2; k >= 0; k--)
+			val32 = (val32 << 8) + uuid_ptr[k];
+		service.value.uuid32 = val32;
+		uuid_str = bt_uuid2string(&service);
+		eir->services = g_slist_append(eir->services, uuid_str);
+		uuid_ptr += 4;
+	}
+}
+
+static void eir_parse_uuid128(struct eir_data *eir, uint8_t *data, uint8_t len)
+{
+	uint8_t *uuid_ptr = data;
+	uuid_t service;
+	char *uuid_str;
+	unsigned int i;
+	int k;
+
+	service.type = SDP_UUID128;
+	for (i = 0; i < len / 16; i++) {
+		for (k = 0; k < 16; k++)
+			service.value.uuid128.data[k] = uuid_ptr[16 - k - 1];
+		uuid_str = bt_uuid2string(&service);
+		eir->services = g_slist_append(eir->services, uuid_str);
+		uuid_ptr += 16;
+	}
+}
+
+int eir_parse(struct eir_data *eir, uint8_t *eir_data)
+{
+	uint16_t len = 0;
 
 	eir->flags = -1;
 
@@ -86,92 +134,49 @@ int eir_parse(struct eir_data *eir, uint8_t *eir_data)
 		if (field_len == 0)
 			break;
 
+		len += field_len + 1;
+
+		/* Bail out if got incorrect length */
+		if (len > HCI_MAX_EIR_LENGTH) {
+			eir_data_free(eir);
+			return -EINVAL;
+		}
+
 		switch (eir_data[1]) {
 		case EIR_UUID16_SOME:
 		case EIR_UUID16_ALL:
-			uuid16_count = field_len / 2;
-			uuid16 = &eir_data[2];
+			eir_parse_uuid16(eir, &eir_data[2], field_len);
 			break;
+
 		case EIR_UUID32_SOME:
 		case EIR_UUID32_ALL:
-			uuid32_count = field_len / 4;
-			uuid32 = &eir_data[2];
+			eir_parse_uuid32(eir, &eir_data[2], field_len);
 			break;
+
 		case EIR_UUID128_SOME:
 		case EIR_UUID128_ALL:
-			uuid128_count = field_len / 16;
-			uuid128 = &eir_data[2];
+			eir_parse_uuid128(eir, &eir_data[2], field_len);
 			break;
+
 		case EIR_FLAGS:
 			eir->flags = eir_data[2];
 			break;
+
 		case EIR_NAME_SHORT:
 		case EIR_NAME_COMPLETE:
-			name = (const char *) &eir_data[2];
-			name_len = field_len - 1;
+			if (g_utf8_validate((char *) &eir_data[2],
+							field_len - 1, NULL))
+				eir->name = g_strndup((char *) &eir_data[2],
+								field_len - 1);
+			else
+				eir->name = g_strdup("");
 			eir->name_complete = eir_data[1] == EIR_NAME_COMPLETE;
 			break;
 		}
 
-		len += field_len + 1;
 		eir_data += field_len + 1;
 	}
 
-	/* Bail out if got incorrect length */
-	if (len > HCI_MAX_EIR_LENGTH)
-		return -EINVAL;
-
-	if (name != NULL) {
-		if (g_utf8_validate(name, name_len, NULL))
-			eir->name = g_strndup(name, name_len);
-		else
-			eir->name = g_strdup("");
-	}
-
-	total = uuid16_count + uuid32_count + uuid128_count;
-
-	/* No UUIDs were parsed, so skip code below */
-	if (!total)
-		return 0;
-
-	/* Generate uuids in SDP format (EIR data is Little Endian) */
-	service.type = SDP_UUID16;
-	for (i = 0; i < uuid16_count; i++) {
-		uint16_t val16 = uuid16[1];
-
-		val16 = (val16 << 8) + uuid16[0];
-		service.value.uuid16 = val16;
-		uuid_str = bt_uuid2string(&service);
-		eir->services = g_slist_append(eir->services, uuid_str);
-		uuid16 += 2;
-	}
-
-	service.type = SDP_UUID32;
-	for (i = uuid16_count; i < uuid32_count + uuid16_count; i++) {
-		uint32_t val32 = uuid32[3];
-		int k;
-
-		for (k = 2; k >= 0; k--)
-			val32 = (val32 << 8) + uuid32[k];
-
-		service.value.uuid32 = val32;
-		uuid_str = bt_uuid2string(&service);
-		eir->services = g_slist_append(eir->services, uuid_str);
-		uuid32 += 4;
-	}
-
-	service.type = SDP_UUID128;
-	for (i = uuid32_count + uuid16_count; i < total; i++) {
-		int k;
-
-		for (k = 0; k < 16; k++)
-			service.value.uuid128.data[k] = uuid128[16 - k - 1];
-
-		uuid_str = bt_uuid2string(&service);
-		eir->services = g_slist_append(eir->services, uuid_str);
-		uuid128 += 16;
-	}
-
 	return 0;
 }
 
-- 
1.7.1

[PATCH v2 2/2] add unit/test-eir to .gitignore

[Frédéric Danis]

---
 .gitignore |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index c2fa9e9..4083b7c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -84,6 +84,7 @@ test/mpris-player
 compat/dund
 compat/hidd
 compat/pand
+unit/test-eir
 
 doc/*.bak
 doc/*.stamp
-- 
1.7.1

Re: [PATCH v2 1/2] Simplify eir_parse function

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/10/20 Frédéric Danis <frederic.danis@linux.intel.com>:
> ---
>  src/eir.c |  155 +++++++++++++++++++++++++++++++-----------------------------
>  1 files changed, 80 insertions(+), 75 deletions(-)
>
> diff --git a/src/eir.c b/src/eir.c
> index e82d30b..f188031 100644
> --- a/src/eir.c
> +++ b/src/eir.c
> @@ -54,24 +54,72 @@
>  void eir_data_free(struct eir_data *eir)
>  {
>        g_slist_free_full(eir->services, g_free);
> +       eir->services = NULL;
>        g_free(eir->name);
> +       eir->name = NULL;
>  }
>
> -int eir_parse(struct eir_data *eir, uint8_t *eir_data)
> +static void eir_parse_uuid16(struct eir_data *eir, uint8_t *data, uint8_t len)
>  {
> -       uint16_t len = 0;
> -       size_t total;
> -       size_t uuid16_count = 0;
> -       size_t uuid32_count = 0;
> -       size_t uuid128_count = 0;
> -       uint8_t *uuid16 = NULL;
> -       uint8_t *uuid32 = NULL;
> -       uint8_t *uuid128 = NULL;
> +       uint8_t *uuid_ptr = data;
> +       uuid_t service;
> +       char *uuid_str;
> +       unsigned int i;
> +       uint16_t val16;
> +
> +       service.type = SDP_UUID16;
> +       for (i = 0; i < len / 2; i++) {
> +               val16 = uuid_ptr[1];
> +               val16 = (val16 << 8) + uuid_ptr[0];
> +               service.value.uuid16 = val16;
> +               uuid_str = bt_uuid2string(&service);
> +               eir->services = g_slist_append(eir->services, uuid_str);
> +               uuid_ptr += 2;
> +       }
> +}
> +
> +static void eir_parse_uuid32(struct eir_data *eir, uint8_t *data, uint8_t len)
> +{
> +       uint8_t *uuid_ptr = data;
>        uuid_t service;
>        char *uuid_str;
> -       const char *name = NULL;
> -       size_t name_len;
>        unsigned int i;
> +       uint32_t val32;
> +       int k;
> +
> +       service.type = SDP_UUID32;
> +       for (i = 0; i < len / 4; i++) {
> +               val32 = uuid_ptr[3];
> +               for (k = 2; k >= 0; k--)
> +                       val32 = (val32 << 8) + uuid_ptr[k];
> +               service.value.uuid32 = val32;
> +               uuid_str = bt_uuid2string(&service);
> +               eir->services = g_slist_append(eir->services, uuid_str);
> +               uuid_ptr += 4;
> +       }
> +}
> +
> +static void eir_parse_uuid128(struct eir_data *eir, uint8_t *data, uint8_t len)
> +{
> +       uint8_t *uuid_ptr = data;
> +       uuid_t service;
> +       char *uuid_str;
> +       unsigned int i;
> +       int k;
> +
> +       service.type = SDP_UUID128;
> +       for (i = 0; i < len / 16; i++) {
> +               for (k = 0; k < 16; k++)
> +                       service.value.uuid128.data[k] = uuid_ptr[16 - k - 1];
> +               uuid_str = bt_uuid2string(&service);
> +               eir->services = g_slist_append(eir->services, uuid_str);
> +               uuid_ptr += 16;
> +       }
> +}
> +
> +int eir_parse(struct eir_data *eir, uint8_t *eir_data)
> +{
> +       uint16_t len = 0;
>
>        eir->flags = -1;
>
> @@ -86,92 +134,49 @@ int eir_parse(struct eir_data *eir, uint8_t *eir_data)
>                if (field_len == 0)
>                        break;
>
> +               len += field_len + 1;
> +
> +               /* Bail out if got incorrect length */
> +               if (len > HCI_MAX_EIR_LENGTH) {
> +                       eir_data_free(eir);
> +                       return -EINVAL;
> +               }
> +
>                switch (eir_data[1]) {
>                case EIR_UUID16_SOME:
>                case EIR_UUID16_ALL:
> -                       uuid16_count = field_len / 2;
> -                       uuid16 = &eir_data[2];
> +                       eir_parse_uuid16(eir, &eir_data[2], field_len);
>                        break;
> +
>                case EIR_UUID32_SOME:
>                case EIR_UUID32_ALL:
> -                       uuid32_count = field_len / 4;
> -                       uuid32 = &eir_data[2];
> +                       eir_parse_uuid32(eir, &eir_data[2], field_len);
>                        break;
> +
>                case EIR_UUID128_SOME:
>                case EIR_UUID128_ALL:
> -                       uuid128_count = field_len / 16;
> -                       uuid128 = &eir_data[2];
> +                       eir_parse_uuid128(eir, &eir_data[2], field_len);
>                        break;
> +
>                case EIR_FLAGS:
>                        eir->flags = eir_data[2];
>                        break;
> +
>                case EIR_NAME_SHORT:
>                case EIR_NAME_COMPLETE:
> -                       name = (const char *) &eir_data[2];
> -                       name_len = field_len - 1;
> +                       if (g_utf8_validate((char *) &eir_data[2],
> +                                                       field_len - 1, NULL))
> +                               eir->name = g_strndup((char *) &eir_data[2],
> +                                                               field_len - 1);
> +                       else
> +                               eir->name = g_strdup("");
>                        eir->name_complete = eir_data[1] == EIR_NAME_COMPLETE;
>                        break;
>                }
>
> -               len += field_len + 1;
>                eir_data += field_len + 1;
>        }
>
> -       /* Bail out if got incorrect length */
> -       if (len > HCI_MAX_EIR_LENGTH)
> -               return -EINVAL;
> -
> -       if (name != NULL) {
> -               if (g_utf8_validate(name, name_len, NULL))
> -                       eir->name = g_strndup(name, name_len);
> -               else
> -                       eir->name = g_strdup("");
> -       }
> -
> -       total = uuid16_count + uuid32_count + uuid128_count;
> -
> -       /* No UUIDs were parsed, so skip code below */
> -       if (!total)
> -               return 0;
> -
> -       /* Generate uuids in SDP format (EIR data is Little Endian) */
> -       service.type = SDP_UUID16;
> -       for (i = 0; i < uuid16_count; i++) {
> -               uint16_t val16 = uuid16[1];
> -
> -               val16 = (val16 << 8) + uuid16[0];
> -               service.value.uuid16 = val16;
> -               uuid_str = bt_uuid2string(&service);
> -               eir->services = g_slist_append(eir->services, uuid_str);
> -               uuid16 += 2;
> -       }
> -
> -       service.type = SDP_UUID32;
> -       for (i = uuid16_count; i < uuid32_count + uuid16_count; i++) {
> -               uint32_t val32 = uuid32[3];
> -               int k;
> -
> -               for (k = 2; k >= 0; k--)
> -                       val32 = (val32 << 8) + uuid32[k];
> -
> -               service.value.uuid32 = val32;
> -               uuid_str = bt_uuid2string(&service);
> -               eir->services = g_slist_append(eir->services, uuid_str);
> -               uuid32 += 4;
> -       }
> -
> -       service.type = SDP_UUID128;
> -       for (i = uuid32_count + uuid16_count; i < total; i++) {
> -               int k;
> -
> -               for (k = 0; k < 16; k++)
> -                       service.value.uuid128.data[k] = uuid128[16 - k - 1];
> -
> -               uuid_str = bt_uuid2string(&service);
> -               eir->services = g_slist_append(eir->services, uuid_str);
> -               uuid128 += 16;
> -       }
> -
>        return 0;
>  }
>
> --
> 1.7.1
>
> --

Ack.


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/2] Simplify eir_parse function

[Johan Hedberg]

Hi Frédéric,

On Thu, Oct 20, 2011, Frédéric Danis wrote:
> -int eir_parse(struct eir_data *eir, uint8_t *eir_data)
> +static void eir_parse_uuid16(struct eir_data *eir, uint8_t *data, uint8_t len)

If you change data to void * it'd make the following changes easier:

> +	uint8_t *uuid_ptr = data;

How about uint16_t *u16 = data;

It would then make the iteration and conversion easier:

> +	service.type = SDP_UUID16;
> +	for (i = 0; i < len / 2; i++) {
> +		val16 = uuid_ptr[1];
> +		val16 = (val16 << 8) + uuid_ptr[0];
> +		service.value.uuid16 = val16;

This could be simply:

	service.value.uuid16 = btohs(bt_get_unaligned(u16));

> +		uuid_str = bt_uuid2string(&service);
> +		eir->services = g_slist_append(eir->services, uuid_str);
> +		uuid_ptr += 2;

And this u16++, actually just put it after i++ directly in the loop
definintion.

> +static void eir_parse_uuid32(struct eir_data *eir, uint8_t *data, uint8_t len)
> +{
> +	uint8_t *uuid_ptr = data;

uint32_t *u32 = data; (assuming you make data void * like above)

> +	service.type = SDP_UUID32;
> +	for (i = 0; i < len / 4; i++) {
> +		val32 = uuid_ptr[3];
> +		for (k = 2; k >= 0; k--)
> +			val32 = (val32 << 8) + uuid_ptr[k];
> +		service.value.uuid32 = val32;

Just change this all to:

		service.value.uuid32 = bt_get_unaligned(btohl(u32));

> +		uuid_ptr += 4;

And move this to the for-loop definition after i++ as u32++

>  		case EIR_NAME_SHORT:
>  		case EIR_NAME_COMPLETE:
> -			name = (const char *) &eir_data[2];
> -			name_len = field_len - 1;
> +			if (g_utf8_validate((char *) &eir_data[2],
> +							field_len - 1, NULL))
> +				eir->name = g_strndup((char *) &eir_data[2],
> +								field_len - 1);
> +			else
> +				eir->name = g_strdup("");
>  			eir->name_complete = eir_data[1] == EIR_NAME_COMPLETE;
>  			break;

If there are multiple name tags in the EIR data you would leak here all
of them except the last one (devices shouldn't have this but you can't
control all sorts of crazy things people do). To keep the behavior as
the existing code, free eir->name when encountering multiple tags and
just keep the last one.

Also, If the name isn't valid UTF-8 I suppose you can just keep
eir->name as NULL instead of pointing to an empty string?

Johan

Re: [PATCH v2 2/2] add unit/test-eir to .gitignore

[Johan Hedberg]

On Thu, Oct 20, 2011, Frédéric Danis wrote:
> ---
>  .gitignore |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

This has been pushed upstream, so no need to resend it.

Btw, in the future please start your commit messages with a capital
letter ("Add.." instead of "add..").

Johan

Re: [PATCH v2 1/2] Simplify eir_parse function

[Andrei Emeltchenko]

Hi Johan,

On Thu, Oct 20, 2011 at 02:19:56PM +0300, Johan Hedberg wrote:
> Hi Frédéric,
> 
> On Thu, Oct 20, 2011, Frédéric Danis wrote:
> > -int eir_parse(struct eir_data *eir, uint8_t *eir_data)
> > +static void eir_parse_uuid16(struct eir_data *eir, uint8_t *data, uint8_t len)
> 
> If you change data to void * it'd make the following changes easier:
> 
> > +	uint8_t *uuid_ptr = data;
> 
> How about uint16_t *u16 = data;

I do not think this is a good name, sometimes u16 specifies type.

> It would then make the iteration and conversion easier:
> 
> > +	service.type = SDP_UUID16;
> > +	for (i = 0; i < len / 2; i++) {
> > +		val16 = uuid_ptr[1];
> > +		val16 = (val16 << 8) + uuid_ptr[0];
> > +		service.value.uuid16 = val16;
> 
> This could be simply:
> 
> 	service.value.uuid16 = btohs(bt_get_unaligned(u16));
> 
> > +		uuid_str = bt_uuid2string(&service);
> > +		eir->services = g_slist_append(eir->services, uuid_str);
> > +		uuid_ptr += 2;
> 
> And this u16++, actually just put it after i++ directly in the loop
> definintion.
> 
> > +static void eir_parse_uuid32(struct eir_data *eir, uint8_t *data, uint8_t len)
> > +{
> > +	uint8_t *uuid_ptr = data;
> 
> uint32_t *u32 = data; (assuming you make data void * like above)
> 
> > +	service.type = SDP_UUID32;
> > +	for (i = 0; i < len / 4; i++) {
> > +		val32 = uuid_ptr[3];
> > +		for (k = 2; k >= 0; k--)
> > +			val32 = (val32 << 8) + uuid_ptr[k];
> > +		service.value.uuid32 = val32;
> 
> Just change this all to:
> 
> 		service.value.uuid32 = bt_get_unaligned(btohl(u32));

btohl(bt_get_unaligned(u32))
maybe we could also use get_u32() ?

Best regards 
Andrei Emeltchenko 

> 
> > +		uuid_ptr += 4;
> 
> And move this to the for-loop definition after i++ as u32++
> 
> >  		case EIR_NAME_SHORT:
> >  		case EIR_NAME_COMPLETE:
> > -			name = (const char *) &eir_data[2];
> > -			name_len = field_len - 1;
> > +			if (g_utf8_validate((char *) &eir_data[2],
> > +							field_len - 1, NULL))
> > +				eir->name = g_strndup((char *) &eir_data[2],
> > +								field_len - 1);
> > +			else
> > +				eir->name = g_strdup("");
> >  			eir->name_complete = eir_data[1] == EIR_NAME_COMPLETE;
> >  			break;
> 
> If there are multiple name tags in the EIR data you would leak here all
> of them except the last one (devices shouldn't have this but you can't
> control all sorts of crazy things people do). To keep the behavior as
> the existing code, free eir->name when encountering multiple tags and
> just keep the last one.
> 
> Also, If the name isn't valid UTF-8 I suppose you can just keep
> eir->name as NULL instead of pointing to an empty string?
> 
> Johan
> --
> 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

RE: [PATCH v2 1/2] Simplify eir_parse function

[Ganir, Chen]

Johan,Frederic,

> 
> 
> If there are multiple name tags in the EIR data you would leak here all
> of them except the last one (devices shouldn't have this but you can't
> control all sorts of crazy things people do). To keep the behavior as
> the existing code, free eir->name when encountering multiple tags and
> just keep the last one.
> 

The spec is very unspecific here (BT 4.0, Vol.3 Part C, section 11.1). Other than the Flags AD Data type which explicitly prohibits sending more than one, and the service solicitation AD type which implicitly limits it to one instance ("One of the Service Solicitation AD types may be sent to invite...."), all other AD Types are not limited in any way. A broadcaster/peripheral may send multiple instance of AD types. I'm not sure it is very clear how to handle two service AD types with different services, or handling two Full name AD types with different data. Maybe we should take a decision here to use the latest values advertised for those inconclusive AD Data types ?

Chen Ganir.

Re: [PATCH v2 1/2] Simplify eir_parse function

[Anderson Lizardo]

Hi,

2011/10/20 Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com>:
>> Just change this all to:
>>
>>               service.value.uuid32 = bt_get_unaligned(btohl(u32));
>
> btohl(bt_get_unaligned(u32))
> maybe we could also use get_u32() ?

There is also att_get_u32()  in attrib/att.h (exact same code). Maybe
we need some code reuse here :)

Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil