Hi Inga,
On Thu, Jun 17, 2010, Inga Stotland wrote:
> ---
> plugins/service.c | 18 ++++++
> src/adapter.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> src/adapter.h | 5 +-
> src/dbus-hci.c | 6 +-
> src/sdpd-service.c | 106 +++++++++++++++++++++++++++++++------
> src/sdpd.h | 20 +++++++
> 6 files changed, 277 insertions(+), 26 deletions(-)
Could you please split this patch up into smaller chunks if possible. It
seems it has several parts which could be considered logically
independent.
Some other comments:
> void adapter_set_class_complete(bdaddr_t *bdaddr, uint8_t status)
> {
> uint8_t class[3];
> @@ -2677,6 +2698,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,
> @@ -2688,7 +2710,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 *);
> }
>
It looks like this at least could be a separate patch (DBUS_TYPE_ARRAY
support for append_dict_valist).
> @@ -2719,8 +2746,106 @@ 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)
Why the extra empty line? In general there should never be a need for
two consecutive empty lines.
> +{
> + uint8_t type;
> + uint8_t len = 0;
> + uint8_t field_len;
> + int count = 0;
> +
> + while (len < EIR_DATA_LENGTH) {
> + type = eir_data[1];
> + 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;
> +}
Variables should always be declared in the minimum scope possible. In
the above function only the count and len variables need to be in the
function scope whereas all other should be declared inside the while
loop.
> +static void get_uuids_eir(char **uuids, uint8_t *eir_data)
> +{
> + uint8_t type;
> + uint8_t len = 0;
> + uint8_t field_len = 0;
> + int count = 0;
> + int size;
> + uuid_t service;
> + gboolean service_found;
> +
> + /* Count UUID16, UUID32 and UUID128 */
> + while (len < EIR_DATA_LENGTH) {
> + type = eir_data[1];
> + field_len = eir_data[0];
> + service_found = FALSE;
Same comment regarding the scope of variables.
> + if ((type == EIR_UUID16_SOME) || (type == EIR_UUID16_ALL)) {
> + size = 2;
> + service.type = SDP_UUID16;
> + service_found = TRUE;
> + } else if ((type == EIR_UUID32_SOME) || (type == EIR_UUID32_ALL)) {
> + size = 4;
> + service.type = SDP_UUID32;
> + service_found = TRUE;
> + } else if ((type == EIR_UUID128_SOME) || (type == EIR_UUID128_ALL)) {
> + size = 16;
> + service.type = SDP_UUID128;
> + service_found = TRUE;
> + }
> +
> + if (service_found) {
I think it'd be more readable to have
if (!service_found)
continue;
The two last operations in the loop still need to be executed to which
the solution would be to use a for loop and have the commands in the
third part (i.e. for (...; ...; <whatever needs to be done every time>)
or use a label+goto.
> + uint8_t *data = &eir_data[2];
> + uint16_t val16;
> + uint32_t val32;
> + int i, k;
> +
> + count = field_len/size;
Space before and after /
> +
> + /* Generate uuids in SDP format (EIR data is Little Endian) */
> + switch (service.type) {
> + case SDP_UUID16:
> + for (i = 0; i < count; i++) {
> + val16 = data[1];
> + val16 = (val16<<8) + data[0];
> + service.value.uuid16 = val16;
> + *uuids++ = bt_uuid2string(&service);
> + data += size;
> + }
> + break;
> + case SDP_UUID32:
> + for (i = 0; i < count; i++) {
> + val32 = data[3];
> + for (k = size-2 ; k >= 0; k--)
> + val32 = (val32<<8) + data[k];
Space before and after <<
> + service.value.uuid32 = val32;
> + *uuids++ = bt_uuid2string(&service);
> + data += size;
> + }
> + break;
> + case SDP_UUID128:
> + for (i = 0; i < count; i++) {
> + for (k = 0; k < size; k++)
> + service.value.uuid128.data[k] = data[size-k-1];
Space before and after -
You're running into trouble with the 80-character line limit while still
keeping the code readable, but imho the core issue is just a too complex
and long function. Please consider if you could find a way to refactor
or rearrange it somehow.
> void adapter_emit_device_found(struct btd_adapter *adapter,
> - struct remote_dev_info *dev);
> + struct remote_dev_info *dev, uint8_t *eir_data);
Looks like a mixed tabs & spaces coding style violation.
> diff --git a/src/sdpd-service.c b/src/sdpd-service.c
> index 798e49d..5aa00f1 100644
> --- a/src/sdpd-service.c
> +++ b/src/sdpd-service.c
> @@ -181,23 +181,30 @@ void create_ext_inquiry_response(const char *name,
With your changes this function is growing quite ridiculously huge.
Would there be any way to avoid that?
> - ptr += len + 2;
> + eir_len += (len + 2);
Tab & space before the += (it should just be a space)
> *ptr++ = (did_version & 0xff00) >> 8;
> + eir_len += 10;
> }
Same here.
> + /* Group all UUID128 types */
> + if (!truncated && (eir_len <= EIR_DATA_LENGTH - 2 )) {
> +
> + list = services;
> + index = 0;
> +
> + /* Store UUID128 in place, skipping first 2 bytes to insert type and length later */
Looks to me like this is beyond the 80-character limit.
> + 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 ((eir_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] !=
Space before and after *
> + rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])
80-character limit exceeded
> + ptr = data + eir_len;
Tab & space before = (it should be just a space)
> +#define EIR_DATA_LENGTH 240
> +
> +#define EIR_FLAGS 0x01 /* Flags */
> +#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 OOB_CLASS_OF_DEVICE 0x0D /* Class of Device (OOB only) */
> +#define OOB_SIMPLE_PAIRING_HASH_C 0x0E /* Simple Pairing HashC (OOB only) */
> +#define OOB_SIMPLE_PAIRING_RAND_R 0x0F /* Simple Pairing RandR (OOB only) */
> +#define EIR_DEVICE_ID 0x10 /* Device ID */
> +#define EIR_MANF_DATA 0xFF /* Manufacturer Specific Data */
> +
> +#define SIZEOF_UUID128 16
Do all of these defines really need to be in the sdpd.h header? At least
stuff like SIZEOF_UUID128 should be moved into the .c file that uses it
(if it's really needed at all).
Johan
--
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
