2010-07-31 11:45:06

by Inga Stotland

[permalink] [raw]
Subject: [PATCH v5 0/6] Enhanced support for extended inquiry response

EIR needs to be updated whenever local SDP record database changes.
Previously, when an SDP record was added/removed/updated, EIR would
be written in callback fired off HCI "Command Complete" event set off by
"Write Class Of Device" command. However, if the class of device did not
need to be updated, the EIR write was not happening either. This
implementation has been augmented to write new EIR when modification
in SDP database does not lead to updating of class of device.

Added support for UUID128 service descriptors in local EIR.

Service UUIDs are written to "device properties" dictionary when emitting
"Device Found" signal. This allows peek at available services offered by
a remote device without establishing SDP connection.


--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.


2010-07-31 11:45:12

by Inga Stotland

[permalink] [raw]
Subject: [PATCH 6/6] Support for generating dictionary value of service UUIDs.

Add service UUIDs from EIR to device properties in "Device Found" signal.
---
src/adapter.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
src/adapter.h | 4 +-
src/dbus-hci.c | 6 ++--
3 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 2560440..b353fa6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2748,8 +2748,84 @@ static void emit_device_found(const char *path, const char *address,
g_dbus_send_message(connection, signal);
}

+static int get_uuid_count_eir(uint8_t *eir_data)
+{
+ uint8_t len = 0;
+ int count = 0;
+
+ while (len < EIR_DATA_LENGTH) {
+ uint8_t type = eir_data[1];
+ uint8_t field_len = eir_data[0];
+ if (type == EIR_UUID16_SOME || type == EIR_UUID16_ALL)
+ count += field_len/2;
+ else if (type == EIR_UUID32_SOME || type == EIR_UUID32_ALL)
+ count += field_len/4;
+ else if (type == EIR_UUID128_SOME || type == EIR_UUID128_ALL)
+ count += field_len/16;
+ len += field_len + 1;
+ eir_data += field_len + 1;
+ }
+
+ return count;
+}
+
+static void get_uuids_eir(char **uuids, uint8_t *eir_data)
+{
+ uint8_t len = 0;
+
+ /* Count UUID16, UUID32 and UUID128 */
+ while (len < EIR_DATA_LENGTH) {
+ uint8_t field_len = eir_data[0];
+ uint8_t type = eir_data[1];
+ int count;
+ uuid_t service;
+ int size;
+ uint8_t *data = &eir_data[2];
+ int i, k;
+
+ /* Generate uuids in SDP format (EIR data is Little Endian) */
+ if (type == EIR_UUID16_SOME || type == EIR_UUID16_ALL) {
+ size = 2;
+ count = field_len/size;
+ service.type = SDP_UUID16;
+ for (i = 0; i < count; i++) {
+ uint16_t val16 = data[1];
+ val16 = (val16<<8) + data[0];
+ service.value.uuid16 = val16;
+ *uuids++ = bt_uuid2string(&service);
+ data += size;
+ }
+ } else if (type == EIR_UUID32_SOME || type == EIR_UUID32_ALL) {
+ size = 4;
+ count = field_len/size;
+ service.type = SDP_UUID32;
+ for (i = 0; i < count; i++) {
+ uint32_t val32 = data[3];
+ for (k = size-2; k >= 0; k--)
+ val32 = (val32<<8) + data[k];
+ service.value.uuid32 = val32;
+ *uuids++ = bt_uuid2string(&service);
+ data += size;
+ }
+ } else if (type == EIR_UUID128_SOME || type == EIR_UUID128_ALL) {
+ size = 16;
+ count = field_len/size;
+ service.type = SDP_UUID128;
+ for (i = 0; i < count; i++) {
+ for (k = 0; k < size; k++)
+ service.value.uuid128.data[k] = data[size-k-1];
+ *uuids++ = bt_uuid2string(&service);
+ data += size;
+ }
+ }
+
+ len += field_len + 1;
+ eir_data += field_len + 1;
+ }
+}
+
void adapter_emit_device_found(struct btd_adapter *adapter,
- struct remote_dev_info *dev)
+ struct remote_dev_info *dev, uint8_t *eir_data)
{
struct btd_device *device;
char peer_addr[18], local_addr[18];
@@ -2757,6 +2833,8 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
dbus_bool_t paired = FALSE;
dbus_int16_t rssi = dev->rssi;
char *alias;
+ char **uuids = NULL;
+ int uuid_count = 0;

ba2str(&dev->bdaddr, peer_addr);
ba2str(&adapter->bdaddr, local_addr);
@@ -2776,6 +2854,15 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
} else
alias = g_strdup(dev->alias);

+ /* Extract UUIDs from extended inquiry response if any*/
+ if (eir_data != NULL)
+ uuid_count = get_uuid_count_eir(eir_data);
+
+ if (uuid_count > 0) {
+ uuids = g_new0(char *, uuid_count + 1);
+ get_uuids_eir(uuids, eir_data);
+ }
+
emit_device_found(adapter->path, paddr,
"Address", DBUS_TYPE_STRING, &paddr,
"Class", DBUS_TYPE_UINT32, &dev->class,
@@ -2785,15 +2872,17 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
"Alias", DBUS_TYPE_STRING, &alias,
"LegacyPairing", DBUS_TYPE_BOOLEAN, &dev->legacy,
"Paired", DBUS_TYPE_BOOLEAN, &paired,
+ "UUIDs", DBUS_TYPE_ARRAY, &uuids, uuid_count,
NULL);

g_free(alias);
+ g_strfreev(uuids);
}

void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
int8_t rssi, uint32_t class, const char *name,
const char *alias, gboolean legacy,
- name_status_t name_status)
+ name_status_t name_status, uint8_t *eir_data)
{
struct remote_dev_info *dev, match;

@@ -2832,7 +2921,7 @@ done:
adapter->found_devices = g_slist_sort(adapter->found_devices,
(GCompareFunc) dev_rssi_cmp);

- adapter_emit_device_found(adapter, dev);
+ adapter_emit_device_found(adapter, dev, eir_data);
}

int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index 8226514..a7eca0e 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -113,10 +113,10 @@ struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
int8_t rssi, uint32_t class, const char *name,
const char *alias, gboolean legacy,
- name_status_t name_status);
+ name_status_t name_status, uint8_t *eir_data);
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
void adapter_emit_device_found(struct btd_adapter *adapter,
- struct remote_dev_info *dev);
+ struct remote_dev_info *dev, uint8_t *eir_data);
void adapter_update_oor_devices(struct btd_adapter *adapter);
void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode);
void adapter_setname_complete(bdaddr_t *local, uint8_t status);
diff --git a/src/dbus-hci.c b/src/dbus-hci.c
index b83506f..6d27caa 100644
--- a/src/dbus-hci.c
+++ b/src/dbus-hci.c
@@ -515,7 +515,7 @@ void hcid_dbus_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
if (dev) {
adapter_update_found_devices(adapter, peer, rssi, class,
NULL, NULL, dev->legacy,
- NAME_NOT_REQUIRED);
+ NAME_NOT_REQUIRED, data);
return;
}

@@ -566,7 +566,7 @@ void hcid_dbus_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,

/* add in the list to track name sent/pending */
adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
- legacy, name_status);
+ legacy, name_status, data);

g_free(name);
g_free(alias);
@@ -642,7 +642,7 @@ void hcid_dbus_remote_name(bdaddr_t *local, bdaddr_t *peer, uint8_t status,
if (dev_info) {
g_free(dev_info->name);
dev_info->name = g_strdup(name);
- adapter_emit_device_found(adapter, dev_info);
+ adapter_emit_device_found(adapter, dev_info, NULL);
}

if (device)
--
1.7.2

--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

2010-07-31 11:45:11

by Inga Stotland

[permalink] [raw]
Subject: [PATCH 5/6] Handle arrays in device properties dictionary

---
src/adapter.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index af60495..2560440 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2700,6 +2700,7 @@ static void append_dict_valist(DBusMessageIter *iter,
DBusMessageIter dict;
const char *key;
int type;
+ int n_elements;
void *val;

dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
@@ -2711,7 +2712,12 @@ static void append_dict_valist(DBusMessageIter *iter,
while (key) {
type = va_arg(var_args, int);
val = va_arg(var_args, void *);
- dict_append_entry(&dict, key, type, val);
+ if (type == DBUS_TYPE_ARRAY) {
+ n_elements = va_arg(var_args, int);
+ if (n_elements > 0)
+ dict_append_array(&dict, key, DBUS_TYPE_STRING, val, n_elements);
+ } else
+ dict_append_entry(&dict, key, type, val);
key = va_arg(var_args, char *);
}

--
1.7.2

--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

2010-07-31 11:45:10

by Inga Stotland

[permalink] [raw]
Subject: [PATCH 4/6] Fix in logic to write EIR when SDP records are changed.

Whenver SDP service record is added/deleted/modified check for whether
class of device needs to be updated as well. If the update is
needed, proceed as before and new EIR will be written subsequently.
If the class of device is already present, just update EIR and return.
---
src/adapter.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 789a196..af60495 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -137,6 +137,8 @@ struct btd_adapter {
gint ref;
};

+static void update_ext_inquiry_response(struct btd_adapter *adapter);
+
static void adapter_set_pairable_timeout(struct btd_adapter *adapter,
guint interval);

@@ -216,11 +218,15 @@ static int adapter_set_service_classes(struct btd_adapter *adapter,
adapter->wanted_cod &= 0x00ffff;
adapter->wanted_cod |= (value << 16);

- /* If we already have the CoD we want or the cache is enabled or an
- * existing CoD write is in progress just bail out */
- if (adapter->current_cod == adapter->wanted_cod ||
- adapter->cache_enable || adapter->pending_cod)
+ /* If the cache is enabled or an existing CoD write is in progress
+ * just bail out */
+ if(adapter->cache_enable || adapter->pending_cod)
return 0;
+ /* If we already have the CoD we want, update EIR and return */
+ if (adapter->current_cod == adapter->wanted_cod) {
+ update_ext_inquiry_response(adapter);
+ return 0;
+ }

DBG("Changing service classes to 0x%06x", adapter->wanted_cod);

--
1.7.2

--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

2010-07-31 11:45:09

by Inga Stotland

[permalink] [raw]
Subject: [PATCH 3/6] Support for adding UUID128 to extended inquiry response

---
src/sdpd-service.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/src/sdpd-service.c b/src/sdpd-service.c
index c20df2e..1314ada 100644
--- a/src/sdpd-service.c
+++ b/src/sdpd-service.c
@@ -49,6 +49,8 @@
#include "manager.h"
#include "adapter.h"

+#define SIZEOF_UUID128 16
+
static sdp_record_t *server = NULL;

static uint16_t did_vendor = 0x0000;
@@ -174,6 +176,62 @@ static void update_svclass_list(const bdaddr_t *src)

}

+static void eir_generate_uuid128(sdp_list_t *list,
+ uint8_t *ptr, uint16_t *eir_len)
+{
+ int i, k, index = 0;
+ uint16_t len = *eir_len;
+ uint8_t *uuid128;
+ gboolean truncated = FALSE;
+
+ /* Store UUID128 in place, skip 2 bytes to insert type and length later */
+ uuid128 = ptr + 2;
+
+ for (; list; list = list->next) {
+ sdp_record_t *rec = (sdp_record_t *) list->data;
+
+ if (rec->svclass.type != SDP_UUID128)
+ continue;
+
+ /* Stop if not enough space to put next UUID128 */
+ if ((len + 2 + SIZEOF_UUID128) > EIR_DATA_LENGTH) {
+ truncated = TRUE;
+ break;
+ }
+
+ /* Check for duplicates, EIR data is Little Endian */
+ for (i = 0; i < index; i++) {
+ for (k = 0; k < SIZEOF_UUID128; k++) {
+ if (uuid128[i*SIZEOF_UUID128 + k] !=
+ rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])
+ break;
+ }
+ if (k == SIZEOF_UUID128)
+ break;
+ }
+
+ if (i < index)
+ continue;
+
+ /* EIR data is Little Endian */
+ for (k = 0; k < SIZEOF_UUID128; k++)
+ uuid128[index*SIZEOF_UUID128 + k] =
+ rec->svclass.value.uuid128.data[SIZEOF_UUID128 - 1 - k];
+
+ len += SIZEOF_UUID128;
+ index++;
+ }
+
+ if (index > 0 || truncated) {
+ /* EIR Data length */
+ ptr[0] = (index * SIZEOF_UUID128) + 1;
+ /* EIR Data type */
+ ptr[1] = (truncated) ? EIR_UUID128_SOME : EIR_UUID128_ALL;
+ len += 2;
+ *eir_len = len;
+ }
+}
+
void create_ext_inquiry_response(const char *name,
int8_t tx_power, sdp_list_t *services,
uint8_t *data)
@@ -271,6 +329,10 @@ void create_ext_inquiry_response(const char *name,
*ptr++ = (uuid16[i] & 0xff00) >> 8;
}
}
+
+ /* Group all UUID128 types */
+ if (eir_len <= EIR_DATA_LENGTH - 2)
+ eir_generate_uuid128(services, ptr, &eir_len);
}

void register_public_browse_group(void)
--
1.7.2

--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

2010-07-31 11:45:08

by Inga Stotland

[permalink] [raw]
Subject: [PATCH 2/6] Clean up code that generates extended inquiry response.

Minor fix when skipping duplicate UUID16 from EIR.
---
src/sdpd-service.c | 46 ++++++++++++++++++++++++++++++----------------
1 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/sdpd-service.c b/src/sdpd-service.c
index cdbb4f4..c20df2e 100644
--- a/src/sdpd-service.c
+++ b/src/sdpd-service.c
@@ -180,35 +180,41 @@ void create_ext_inquiry_response(const char *name,
{
sdp_list_t *list = services;
uint8_t *ptr = data;
- uint16_t uuid[24];
+ uint16_t eir_len = 0;
+ uint16_t uuid16[EIR_DATA_LENGTH/2];
int i, index = 0;
+ gboolean truncated = FALSE;

if (name) {
int len = strlen(name);

+ /* EIR Data type */
if (len > 48) {
len = 48;
- ptr[1] = 0x08;
+ ptr[1] = EIR_NAME_SHORT;
} else
- ptr[1] = 0x09;
+ ptr[1] = EIR_NAME_COMPLETE;

+ /* EIR Data length */
ptr[0] = len + 1;

memcpy(ptr + 2, name, len);

- ptr += len + 2;
+ eir_len += (len + 2);
+ ptr += (len + 2);
}

if (tx_power != 0) {
*ptr++ = 2;
- *ptr++ = 0x0a;
+ *ptr++ = EIR_TX_POWER;
*ptr++ = (uint8_t) tx_power;
+ eir_len += 3;
}

if (did_vendor != 0x0000) {
uint16_t source = 0x0002;
*ptr++ = 9;
- *ptr++ = 0x10;
+ *ptr++ = EIR_DEVICE_ID;
*ptr++ = (source & 0x00ff);
*ptr++ = (source & 0xff00) >> 8;
*ptr++ = (did_vendor & 0x00ff);
@@ -217,10 +223,10 @@ void create_ext_inquiry_response(const char *name,
*ptr++ = (did_product & 0xff00) >> 8;
*ptr++ = (did_version & 0x00ff);
*ptr++ = (did_version & 0xff00) >> 8;
+ eir_len += 10;
}

- ptr[1] = 0x03;
-
+ /* Group all UUID16 types */
for (; list; list = list->next) {
sdp_record_t *rec = (sdp_record_t *) list->data;

@@ -233,28 +239,36 @@ void create_ext_inquiry_response(const char *name,
if (rec->svclass.value.uuid16 == PNP_INFO_SVCLASS_ID)
continue;

- if (index > 23) {
- ptr[1] = 0x02;
+ /* Stop if not enough space to put next UUID16 */
+ if ((eir_len + 2 + sizeof(uint16_t)) > EIR_DATA_LENGTH) {
+ truncated = TRUE;
break;
}

+ /* Check for duplicates */
for (i = 0; i < index; i++)
- if (uuid[i] == rec->svclass.value.uuid16)
+ if (uuid16[i] == rec->svclass.value.uuid16)
break;

- if (i == index - 1)
+ if (i < index)
continue;

- uuid[index++] = rec->svclass.value.uuid16;
+ uuid16[index++] = rec->svclass.value.uuid16;
+ eir_len += sizeof(uint16_t);
}

if (index > 0) {
- ptr[0] = (index * 2) + 1;
+ /* EIR Data length */
+ ptr[0] = (index * sizeof(uint16_t)) + 1;
+ /* EIR Data type */
+ ptr[1] = (truncated) ? EIR_UUID16_SOME : EIR_UUID16_ALL;
+
ptr += 2;
+ eir_len += 2;

for (i = 0; i < index; i++) {
- *ptr++ = (uuid[i] & 0x00ff);
- *ptr++ = (uuid[i] & 0xff00) >> 8;
+ *ptr++ = (uuid16[i] & 0x00ff);
+ *ptr++ = (uuid16[i] & 0xff00) >> 8;
}
}
}
--
1.7.2

2010-07-31 11:45:07

by Inga Stotland

[permalink] [raw]
Subject: [PATCH 1/6] Spec constants for Extended Inquiry Response field types

---
src/sdpd.h | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/src/sdpd.h b/src/sdpd.h
index e93b0b6..c1d69a9 100644
--- a/src/sdpd.h
+++ b/src/sdpd.h
@@ -34,6 +34,19 @@
#define SDPDBG(fmt...)
#endif

+#define EIR_DATA_LENGTH 240
+
+#define EIR_UUID16_SOME 0x02 /* 16-bit UUID, more available */
+#define EIR_UUID16_ALL 0x03 /* 16-bit UUID, all listed */
+#define EIR_UUID32_SOME 0x04 /* 32-bit UUID, more available */
+#define EIR_UUID32_ALL 0x05 /* 32-bit UUID, all listed */
+#define EIR_UUID128_SOME 0x06 /* 128-bit UUID, more available */
+#define EIR_UUID128_ALL 0x07 /* 128-bit UUID, all listed */
+#define EIR_NAME_SHORT 0x08 /* shortened local name */
+#define EIR_NAME_COMPLETE 0x09 /* complete local name */
+#define EIR_TX_POWER 0x0A /* transmit power level */
+#define EIR_DEVICE_ID 0x10 /* device ID */
+
typedef struct request {
bdaddr_t device;
bdaddr_t bdaddr;
--
1.7.2

--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

2010-08-03 16:26:31

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 2/6] Clean up code that generates extended inquiry response.

Hi Inga,

On Tue, Aug 03, 2010, [email protected] wrote:
> Just a clarification. I will still have to have the "clean up" patch as a
> part of this patch set since it relies on the header file with defines
> (getting rid of magic numbers).>

That's fine. I.e. the first patch of the set can remain as is.

Johan

2010-08-03 15:37:05

by Inga Stotland

[permalink] [raw]
Subject: Re: [PATCH 2/6] Clean up code that generates extended inquiry response.

Hi Johan,

>> ---
>> src/sdpd-service.c | 46
>> ++++++++++++++++++++++++++++++----------------
>> 1 files changed, 30 insertions(+), 16 deletions(-)
>
> So your commit message says two things: "Clean up code..." and "Minor
> fix..". Cleanups and fixes should be in separate patches. Don't be
> afraid of "too small" or trivial patches. As long as a patch contains a
> single logically independent change it's fine.
>

Just a clarification. I will still have to have the "clean up" patch as a
part of this patch set since it relies on the header file with defines
(getting rid of magic numbers). Not sure, if you want me to cut the "fix"
one to be separate patch submission altogether...

Thanks,

Inga

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.




2010-08-03 08:36:41

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 6/6] Support for generating dictionary value of service UUIDs.

Hi Inga,

On Sat, Jul 31, 2010, Inga Stotland wrote:
> Add service UUIDs from EIR to device properties in "Device Found" signal.
> ---
> src/adapter.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> src/adapter.h | 4 +-
> src/dbus-hci.c | 6 ++--
> 3 files changed, 97 insertions(+), 8 deletions(-)

A few more coding style issues:

> + while (len < EIR_DATA_LENGTH) {
> + uint8_t type = eir_data[1];
> + uint8_t field_len = eir_data[0];

Add an empty line here (it's almost always a good thing to have after
variable declarations).

> + if (type == EIR_UUID16_SOME || type == EIR_UUID16_ALL)
> + count += field_len/2;

Space before and after /

> + else if (type == EIR_UUID32_SOME || type == EIR_UUID32_ALL)
> + count += field_len/4;

Same here.

> + else if (type == EIR_UUID128_SOME || type == EIR_UUID128_ALL)
> + count += field_len/16;

And here

> + count = field_len/size;

And here

> + count = field_len/size;

And here


> + val32 = (val32<<8) + data[k];

Space before and after <<

> + count = field_len/size;

Space before and after /

> + service.value.uuid128.data[k] = data[size-k-1];

Space before and after -

> + /* Extract UUIDs from extended inquiry response if any*/
> + if (eir_data != NULL)
> + uuid_count = get_uuid_count_eir(eir_data);
> +
> + if (uuid_count > 0) {
> + uuids = g_new0(char *, uuid_count + 1);
> + get_uuids_eir(uuids, eir_data);
> + }
> +

I don't think these two get_uuids_eir and get_uuid_count_eir functions
are really needed. Could you change it to just one function:

char **get_eir_uuids(const uint8_t *eir_data, size_t *count)

Johan

2010-08-03 08:23:23

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 4/6] Fix in logic to write EIR when SDP records are changed.

Hi Inga,

On Sat, Jul 31, 2010, Inga Stotland wrote:
> Whenver SDP service record is added/deleted/modified check for whether
> class of device needs to be updated as well. If the update is
> needed, proceed as before and new EIR will be written subsequently.
> If the class of device is already present, just update EIR and return.
> ---
> src/adapter.c | 14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)

A few minor coding style issues

> - /* If we already have the CoD we want or the cache is enabled or an
> - * existing CoD write is in progress just bail out */
> - if (adapter->current_cod == adapter->wanted_cod ||
> - adapter->cache_enable || adapter->pending_cod)
> + /* If the cache is enabled or an existing CoD write is in progress
> + * just bail out */
> + if(adapter->cache_enable || adapter->pending_cod)

Missing space beteween if and (

> return 0;

Add an empty line here.

> + /* If we already have the CoD we want, update EIR and return */
> + if (adapter->current_cod == adapter->wanted_cod) {
> + update_ext_inquiry_response(adapter);
> + return 0;
> + }

Johan

2010-08-03 08:18:46

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 3/6] Support for adding UUID128 to extended inquiry response

Hi Inga,

On Sat, Jul 31, 2010, Inga Stotland wrote:
> ---
> src/sdpd-service.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 62 insertions(+), 0 deletions(-)

A couple of coding style comments here:

> + /* Check for duplicates, EIR data is Little Endian */
> + for (i = 0; i < index; i++) {
> + for (k = 0; k < SIZEOF_UUID128; k++) {
> + if (uuid128[i*SIZEOF_UUID128 + k] !=

Space before and after *

> + rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])

Looks like you're going beyond 80-characters. Maybe you could create an
extra variable and assign rec->svclass.value.uuid.data to it.

> + for (k = 0; k < SIZEOF_UUID128; k++)
> + uuid128[index*SIZEOF_UUID128 + k] =

Space before and after *

> + rec->svclass.value.uuid128.data[SIZEOF_UUID128 - 1 - k];

Again over 80-characters, but if you use the extra variable that I
previously suggested for rec->svclass.value.uuid128.data it should help.

> + ptr[1] = (truncated) ? EIR_UUID128_SOME : EIR_UUID128_ALL;

Since truncated is just a gboolean and not a more complex statement I'd
just leave out the parenthesis around it, but quite a minor thing in the
end.

Johan

2010-08-03 08:12:58

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 2/6] Clean up code that generates extended inquiry response.

Hi Inga,

On Sat, Jul 31, 2010, Inga Stotland wrote:
> Minor fix when skipping duplicate UUID16 from EIR.
> ---
> src/sdpd-service.c | 46 ++++++++++++++++++++++++++++++----------------
> 1 files changed, 30 insertions(+), 16 deletions(-)

So your commit message says two things: "Clean up code..." and "Minor
fix..". Cleanups and fixes should be in separate patches. Don't be
afraid of "too small" or trivial patches. As long as a patch contains a
single logically independent change it's fine.

Johan

> + uint16_t uuid16[EIR_DATA_LENGTH/2];

Minor nitpick: space before and after /

Johan