Re: [PATCH 3/8] android/gatt: Refactor ATT read operations

[Szymon Janc <szymon.janc@tieto.com>]

Hi,

On Monday 12 of May 2014 15:02:33 Marcin Kraglak wrote:
> This changes device info and gap services callbacks to use response queue.
> It will allow them work with plain read and read by type as well.
> 
> This starts transition to single response data queue, which should be
> filled by various read type functions and processed in one place. This
> will unify the way that responses are send, regardless of data source
> (value taken directly from database, returned by read callback or sent
> from upper layers asynchronously).
> 
> We will also introduce 'getter' type functions, using handles to
> retrieve data from database. This will make various read and find
> operations return handles instead of their own custom structures,
> different for every operation performed.
> ---
>  android/gatt.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 181 insertions(+), 29 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 0a2abe3..f8539d2 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -156,6 +156,8 @@ struct gatt_device {
>  
>  	int ref;
>  	int conn_cnt;
> +
> +	struct queue *pending_requests;
>  };
>  
>  struct app_connection {
> @@ -748,6 +750,21 @@ static void disconnect_notify_by_device(void *data, void *user_data)
>  		send_app_connect_notify(conn, GATT_FAILURE);
>  }
>  
> +struct response_data {
> +	uint16_t handle;
> +	uint16_t length;
> +	uint8_t *value;
> +	uint16_t offset;
> +};
> +
> +static void destroy_response_data(void *data)
> +{
> +	struct response_data *entry = data;
> +
> +	free(entry->value);
> +	free(entry);
> +}
> +
>  static void destroy_device(void *data)
>  {
>  	struct gatt_device *dev = data;
> @@ -756,6 +773,7 @@ static void destroy_device(void *data)
>  		return;
>  
>  	queue_destroy(dev->services, destroy_service);
> +	queue_destroy(dev->pending_requests, destroy_response_data);
>  
>  	free(dev);
>  }
> @@ -1138,6 +1156,13 @@ static struct gatt_device *create_device(const bdaddr_t *addr)
>  		return NULL;
>  	}
>  
> +	dev->pending_requests = queue_new();
> +	if (!dev->pending_requests) {
> +		error("gatt: Failed to allocate memory for client");
> +		destroy_device(dev);
> +		return NULL;
> +	}
> +
>  	if (!queue_push_head(gatt_devices, dev)) {
>  		error("gatt: Cannot push device to queue");
>  		destroy_device(dev);
> @@ -4027,6 +4052,62 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
>  	return 0;
>  }
>  
> +static void send_pending_response(uint8_t opcode, struct gatt_device *device)
> +{
> +	uint8_t rsp[ATT_DEFAULT_LE_MTU];
> +	struct response_data *val;
> +	uint16_t len = 0;
> +
> +	if (queue_isempty(device->pending_requests))
> +		goto done;
> +
> +	switch (opcode) {
> +	case ATT_OP_READ_BLOB_REQ:
> +		val = queue_pop_head(device->pending_requests);
> +		if (!val)
> +			goto done;
> +
> +		len = enc_read_blob_resp(val->value, val->length, val->offset,
> +							rsp, sizeof(rsp));
> +		break;
> +	case ATT_OP_READ_REQ:
> +		val = queue_pop_head(device->pending_requests);
> +		if (!val)
> +			goto done;
> +
> +		len = enc_read_resp(val->value, val->length, rsp, sizeof(rsp));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +done:

This label is not really needed. Just break where needed.

> +	if (!len)
> +		len = enc_error_resp(opcode, 0x0000, ATT_ECODE_UNLIKELY,
> +						rsp, ATT_DEFAULT_LE_MTU);
> +
> +	g_attrib_send(device->attrib, 0, rsp, len, NULL, NULL, NULL);
> +
> +	queue_remove_all(device->pending_requests, NULL, NULL,
> +							destroy_response_data);
> +}
> +
> +static bool match_handle_val_by_empty_len(const void *data,
> +							const void *user_data)
> +{
> +	const struct response_data *handle_data = data;
> +
> +	return !handle_data->length;
> +}
> +
> +static bool match_handle_val_by_handle(const void *data, const void *user_data)
> +{
> +	const struct response_data *handle_data = data;
> +	uint16_t handle = PTR_TO_UINT(user_data);
> +
> +	return handle_data->handle == handle;
> +}
> +
>  static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
>  						uint8_t *rsp, size_t rsp_size,
>  						uint16_t *length)
> @@ -4086,6 +4167,9 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
>  	uint16_t handle;
>  	uint16_t len;
>  	uint16_t offset = 0;
> +	uint8_t *value;
> +	uint16_t length;
> +	struct response_data *data;
>  
>  	DBG("");
>  
> @@ -4105,10 +4189,29 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
>  		return ATT_ECODE_REQ_NOT_SUPP;
>  	}
>  
> -	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, NULL,
> -									NULL))
> +	data = new0(struct response_data, 1);
> +	if (!data)
> +		return ATT_ECODE_INSUFF_RESOURCES;
> +
> +	data->handle = handle;
> +
> +	if (!gatt_db_read(gatt_db, handle, offset, cmd[0], &dev->bdaddr, &value,
> +								&length))
>  		return ATT_ECODE_UNLIKELY;
>  
> +	if (length) {
> +		data->value = malloc0(length);
> +		if (!data->value) {
> +			free(data);
> +			return ATT_ECODE_INSUFF_RESOURCES;
> +	}
> +
> +	data->length = length;
> +	memcpy(data->value, value, length);

Put this memcpy inside if

> +
> +	send_pending_response(cmd[0], dev);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -4308,9 +4411,6 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>  	case ATT_OP_READ_REQ:
>  	case ATT_OP_READ_BLOB_REQ:
>  		status = read_request(ipdu, len, dev);
> -		if (!status)
> -			/* Response will be sent in callback */
> -			return;
>  		break;
>  	case ATT_OP_MTU_REQ:
>  		status = mtu_att_handle(ipdu, len, opdu, sizeof(opdu), dev,
> @@ -4449,8 +4549,7 @@ static struct gap_srvc_handles gap_srvc_data;
>  static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>  					bdaddr_t *bdaddr, void *user_data)
>  {
> -	uint8_t pdu[ATT_DEFAULT_LE_MTU];
> -	uint16_t len;
> +	struct response_data *entry;
>  	struct gatt_device *dev;
>  
>  	DBG("");
> @@ -4461,33 +4560,61 @@ static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>  		return;
>  	}
>  
> +	entry = queue_find(dev->pending_requests, match_handle_val_by_handle,
> +							UINT_TO_PTR(handle));
> +
> +	/*
> +	 * There will already be such record if its batch read
> +	 * (i.e. read by type), else - its normal read.
> +	 */
> +	if (!entry) {
> +		entry = new0(struct response_data, 1);
> +		if (!entry)
> +			goto done;
> +
> +		queue_push_tail(dev->pending_requests, entry);
> +	}
> +
>  	if (handle == gap_srvc_data.dev_name) {
>  		const char *name = bt_get_adapter_name();
>  
> -		len = enc_read_resp((uint8_t *)name, strlen(name),
> -							pdu, sizeof(pdu));
> -		goto done;
> -	}
> +		entry->value = malloc0(strlen(name));
> +		if (!entry->value) {
> +			queue_remove(dev->pending_requests, entry);
> +			free(entry);
> +			goto done;
> +		}
>  
> -	if (handle == gap_srvc_data.appear) {
> -		uint8_t val[2];
> -		put_le16(APPEARANCE_GENERIC_PHONE, val);
> -		len = enc_read_resp(val, sizeof(val), pdu, sizeof(pdu));
> -		goto done;
> -	}
> +		entry->length = strlen(name);
> +		memcpy(entry->value, bt_get_adapter_name(), entry->length);
> +	} else if (handle == gap_srvc_data.appear) {
> +		entry->value = malloc0(2);

A bit OT here..

This...

> +		if (!entry->value) {
> +			queue_remove(dev->pending_requests, entry);
> +			free(entry);
> +			goto done;
> +		}
>  
> -	if (handle == gap_srvc_data.priv) {
> -		uint8_t val = PERIPHERAL_PRIVACY_DISABLE;
> -		len = enc_read_resp(&val, sizeof(val), pdu, sizeof(pdu));
> -		goto done;
> +		put_le16(APPEARANCE_GENERIC_PHONE, entry->value);
> +		entry->length = sizeof(uint8_t) * 2;
> +	} else if (handle == gap_srvc_data.priv) {
> +		entry->value = malloc0(1);
> +		if (!entry->value) {
> +			queue_remove(dev->pending_requests, entry);
> +			free(entry);
> +			goto done;
> +		}

... and this makes me wonder if we should consider having abort policy
for OOM scenarios (at least for fixed size allocations) like GLib APIs do.

> +
> +		*entry->value = PERIPHERAL_PRIVACY_DISABLE;
> +		entry->length = sizeof(uint8_t);
>  	}
>  
> -	error("gatt: Unknown handle 0x%02x", handle);
> -	len = enc_error_resp(ATT_OP_READ_REQ, handle,
> -					ATT_ECODE_UNLIKELY, pdu, sizeof(pdu));
>  
> +	entry->offset = offset;
>  done:
> -	g_attrib_send(dev->attrib, 0, pdu, len, NULL, NULL, NULL);
> +	if (!queue_find(dev->pending_requests, match_handle_val_by_empty_len,
> +									NULL))
> +		send_pending_response(att_opcode, dev);
>  }
>  
>  static void register_gap_service(void)
> @@ -4551,10 +4678,9 @@ static void device_info_read_cb(uint16_t handle, uint16_t offset,
>  					uint8_t att_opcode, bdaddr_t *bdaddr,
>  					void *user_data)
>  {
> -	uint8_t pdu[ATT_DEFAULT_LE_MTU];
> +	struct response_data *entry;
>  	struct gatt_device *dev;
>  	char *buf = user_data;
> -	uint16_t len;
>  
>  	dev = find_device_by_addr(bdaddr);
>  	if (!dev) {
> @@ -4562,9 +4688,35 @@ static void device_info_read_cb(uint16_t handle, uint16_t offset,
>  		return;
>  	}
>  
> -	len = enc_read_resp((uint8_t *) buf, strlen(buf), pdu, sizeof(pdu));
> +	entry = queue_find(dev->pending_requests, match_handle_val_by_handle,
> +							UINT_TO_PTR(handle));
>  
> -	g_attrib_send(dev->attrib, 0, pdu, len, NULL, NULL, NULL);
> +	/* there will already be such record if its batch read
> +	 * (i.e. read by type), else - its normal read.
> +	 */

/*
 * foo
 * bar
 */

Lets keep this consistent.

> +	if (!entry) {
> +		entry = new0(struct response_data, 1);
> +		if (!entry)
> +			goto done;
> +
> +		queue_push_tail(dev->pending_requests, entry);
> +	}
> +
> +	entry->value = malloc0(strlen(buf));
> +	if (!entry->value) {
> +		queue_remove(dev->pending_requests, entry);
> +		free(entry);
> +		goto done;
> +	}
> +
> +	entry->length = strlen(buf);
> +	memcpy(entry->value, buf, entry->length);
> +	entry->offset = offset;
> +
> +done:
> +	if (!queue_find(dev->pending_requests, match_handle_val_by_empty_len,
> +									NULL))
> +		send_pending_response(att_opcode, dev);
>  }
>  
>  static void register_device_info_service(void)
> 

-- 
Best regards, 
Szymon Janc