Re: [PATCH 2/2] Added support for updating EIR whenever record is added or removed

[ingas@codeaurora.org]

Hi Johan,

Thank you for your comprehensive comments. I will incorporate your
suggestions and investigate the feasibility of splitting the patch into
smaller functional pieces.

Inga

> 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
>