[PATCHv2 1/3] android/gatt: Fix parallel reading/writing attributes values from applications

[PATCHv2 1/3] android/gatt: Fix parallel reading/writing attributes values from applications

[Marcin Kraglak]

It is needed because in some cases we send few read requests to applications.
Now all transactions data are overriden by last one, and if application wants
to respond to previous requests, transaction data is not found.
---
 android/gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 59 insertions(+), 26 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 66d6224..80d9f30 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -106,9 +106,6 @@ struct gatt_app {
 
 	/* Valid for client applications */
 	struct queue *notifications;
-
-	/* Transaction data valid for server application */
-	struct pending_trans_data trans_id;
 };
 
 struct element_id {
@@ -175,6 +172,7 @@ struct gatt_device {
 struct app_connection {
 	struct gatt_device *device;
 	struct gatt_app *app;
+	struct queue *transactions;
 	int32_t id;
 };
 
@@ -182,6 +180,7 @@ static struct ipc *hal_ipc = NULL;
 static bdaddr_t adapter_addr;
 static bool scanning = false;
 static unsigned int advertising_cnt = 0;
+static int32_t trans_id = 1;
 
 static struct queue *gatt_apps = NULL;
 static struct queue *gatt_devices = NULL;
@@ -855,6 +854,8 @@ static void destroy_connection(void *data)
 	if (conn->device->conn_cnt == 0)
 		connection_cleanup(conn->device);
 
+	queue_destroy(conn->transactions, free);
+
 cleanup:
 	device_unref(conn->device);
 	free(conn);
@@ -1305,6 +1306,12 @@ static struct app_connection *create_connection(struct gatt_device *device,
 	new_conn->app = app;
 	new_conn->id = last_conn_id++;
 
+	new_conn->transactions = queue_new();
+	if (!new_conn->transactions) {
+		free(new_conn);
+		return NULL;
+	}
+
 	if (!queue_push_head(app_connections, new_conn)) {
 		error("gatt: Cannot push client on the client queue!?");
 
@@ -4169,26 +4176,29 @@ done:
 		send_dev_pending_response(dev, opcode);
 }
 
-static void set_trans_id(struct gatt_app *app, unsigned int id, int8_t opcode)
+static struct pending_trans_data *new_transaction(unsigned int id,
+								uint8_t opcode)
 {
-	app->trans_id.id = id;
-	app->trans_id.opcode = opcode;
-}
+	struct pending_trans_data *transaction;
 
-static void clear_trans_id(struct gatt_app *app)
-{
-	app->trans_id.id = 0;
-	app->trans_id.opcode = 0;
+	transaction = new0(struct pending_trans_data, 1);
+	if (!transaction)
+		return NULL;
+
+	transaction->id = id;
+	transaction->opcode = opcode;
+
+	return transaction;
 }
 
 static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 					bdaddr_t *bdaddr, void *user_data)
 {
+	struct pending_trans_data *transaction;
 	struct hal_ev_gatt_server_request_read ev;
 	struct gatt_app *app;
 	struct app_connection *conn;
 	int32_t id = PTR_TO_INT(user_data);
-	static int32_t trans_id = 1;
 
 	app = find_app_by_id(id);
 	if (!app) {
@@ -4205,14 +4215,23 @@ static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 	memset(&ev, 0, sizeof(ev));
 
 	/* Store the request data, complete callback and transaction id */
-	set_trans_id(app, trans_id++, att_opcode);
+	transaction = new_transaction(trans_id++, att_opcode);
+	if (!transaction) {
+		error("gatt: failed to allocate memory for transaction");
+		goto failed;
+	}
+
+	if (!queue_push_tail(conn->transactions, transaction)) {
+		free(transaction);
+		goto failed;
+	}
 
 	bdaddr2android(bdaddr, ev.bdaddr);
 	ev.conn_id = conn->id;
 	ev.attr_handle = handle;
 	ev.offset = offset;
 	ev.is_long = att_opcode == ATT_OP_READ_BLOB_REQ;
-	ev.trans_id = app->trans_id.id;
+	ev.trans_id = transaction->id;
 
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
 					HAL_EV_GATT_SERVER_REQUEST_READ,
@@ -4232,9 +4251,9 @@ static void write_cb(uint16_t handle, uint16_t offset,
 {
 	uint8_t buf[IPC_MTU];
 	struct hal_ev_gatt_server_request_write *ev = (void *) buf;
+	struct pending_trans_data *transaction;
 	struct gatt_app *app;
 	int32_t id = PTR_TO_INT(user_data);
-	static int32_t trans_id = 1;
 	struct app_connection *conn;
 
 	app = find_app_by_id(id);
@@ -4250,7 +4269,16 @@ static void write_cb(uint16_t handle, uint16_t offset,
 	}
 
 	/* Store the request data, complete callback and transaction id */
-	set_trans_id(app, trans_id++, att_opcode);
+	transaction = new_transaction(trans_id++, att_opcode);
+	if (!transaction) {
+		error("gatt: failed to allocate memory for transaction");
+		goto failed;
+	}
+
+	if (!queue_push_tail(conn->transactions, transaction)) {
+		free(transaction);
+		goto failed;
+	}
 
 	/* TODO figure it out */
 	if (att_opcode == ATT_OP_EXEC_WRITE_REQ)
@@ -4263,7 +4291,7 @@ static void write_cb(uint16_t handle, uint16_t offset,
 	ev->offset = offset;
 
 	ev->conn_id = conn->id;
-	ev->trans_id = app->trans_id.id;
+	ev->trans_id = transaction->id;
 
 	ev->is_prep = att_opcode == ATT_OP_PREP_WRITE_REQ;
 	ev->need_rsp = att_opcode == ATT_OP_WRITE_REQ;
@@ -4557,11 +4585,18 @@ reply:
 				HAL_OP_GATT_SERVER_SEND_INDICATION, status);
 }
 
+static bool match_trans_id(const void *data, const void *user_data)
+{
+	const struct pending_trans_data *transaction = data;
+
+	return transaction->id == PTR_TO_UINT(user_data);
+}
+
 static void handle_server_send_response(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_server_send_response *cmd = buf;
+	struct pending_trans_data *transaction;
 	struct app_connection *conn;
-	struct gatt_app *app;
 	uint8_t status;
 
 	DBG("");
@@ -4573,22 +4608,20 @@ static void handle_server_send_response(const void *buf, uint16_t len)
 		goto reply;
 	}
 
-	app = conn->app;
-
-	if ((unsigned int)cmd->trans_id != app->trans_id.id) {
-		error("gatt: transaction ID mismatch (%d!=%d)",
-					cmd->trans_id, app->trans_id.id);
-
+	transaction = queue_remove_if(conn->transactions, match_trans_id,
+						UINT_TO_PTR(cmd->trans_id));
+	if (!transaction) {
+		error("gatt: transaction ID = %d not found", cmd->trans_id);
 		status = HAL_STATUS_FAILED;
 		goto reply;
 	}
 
-	send_gatt_response(conn->app->trans_id.opcode, cmd->handle, cmd->offset,
+	send_gatt_response(transaction->opcode, cmd->handle, cmd->offset,
 					cmd->status, cmd->len, cmd->data,
 					&conn->device->bdaddr);
 
 	/* Clean request data */
-	clear_trans_id(app);
+	free(transaction);
 
 	status = HAL_STATUS_SUCCESS;
 
-- 
1.9.0

[PATCHv2 2/3] shared/gatt: Return bool in gatt_db_get_attribute_permissions

[Marcin Kraglak]

It will return true if attribute with given handle exists in db and
set permissions value, otherwise it will return false.
Now, if get_permissions failed, we should reply with
ATT_ECODE_ATTR_NOT_FOUND.
---
 android/gatt.c       | 23 +++++++++++++++++------
 src/shared/gatt-db.c | 11 +++++++----
 src/shared/gatt-db.h |  3 ++-
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 80d9f30..421cae4 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4070,8 +4070,13 @@ static void read_requested_attributes(void *data, void *user_data)
 	uint8_t *value;
 	int value_len;
 
-	permissions = gatt_db_get_attribute_permissions(gatt_db,
-							resp_data->handle);
+	if (!gatt_db_get_attribute_permissions(gatt_db,
+							resp_data->handle,
+							&permissions)) {
+		resp_data->error = ATT_ECODE_ATTR_NOT_FOUND;
+		resp_data->state = REQUEST_DONE;
+		return;
+	}
 
 	/*
 	 * Check if it is attribute we didn't declare permissions, like service
@@ -5067,7 +5072,9 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return;
 
-	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
+	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
+		return;
+
 	if (check_device_permissions(dev, cmd[0], permissions))
 		return;
 
@@ -5093,7 +5100,9 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 
 	len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
 
-	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
+	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
+		return;
+
 	if (check_device_permissions(dev, cmd[0], permissions))
 		return;
 
@@ -5133,7 +5142,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
-	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
+	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
+		return ATT_ECODE_ATTR_NOT_FOUND;
 
 	error = check_device_permissions(dev, cmd[0], permissions);
 	if (error)
@@ -5181,7 +5191,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
-	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
+	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
+		return ATT_ECODE_ATTR_NOT_FOUND;
 
 	error = check_device_permissions(dev, cmd[0], permissions);
 	if (error)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 998e93e..c11c5d1 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -734,7 +734,8 @@ uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle)
 	return service->attributes[0]->handle + service->num_handles - 1;
 }
 
-uint32_t gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle)
+bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
+							uint32_t *permissions)
 {
 	struct gatt_db_attribute *attribute;
 	struct gatt_db_service *service;
@@ -743,7 +744,7 @@ uint32_t gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle)
 	service = queue_find(db->services, find_service_for_handle,
 							INT_TO_PTR(handle));
 	if (!service)
-		return 0;
+		return false;
 
 	service_handle = service->attributes[0]->handle;
 
@@ -754,7 +755,9 @@ uint32_t gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle)
 	 */
 	attribute = service->attributes[handle - service_handle];
 	if (!attribute)
-		return 0;
+		return false;
+
+	*permissions = attribute->permissions;
+	return true;
 
-	return attribute->permissions;
 }
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index f2f2f4d..a88f637 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -92,4 +92,5 @@ const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
 
 uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle);
 
-uint32_t gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle);
+bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
+							uint32_t *permissions);
-- 
1.9.0

[PATCHv2 3/3] android/gatt: Check for invalid handle errors

[Marcin Kraglak]

Check if handle or handle range is valid for server. If is invalid,
reply with ATT_ECODE_INVALID_HANDLE.
---
 android/gatt.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 421cae4..ee14968 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4755,6 +4755,9 @@ static uint8_t read_by_group_type(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
+	if (start > end || start == 0)
+		return ATT_ECODE_INVALID_HANDLE;
+
 	q = queue_new();
 	if (!q)
 		return ATT_ECODE_INSUFF_RESOURCES;
@@ -4808,7 +4811,7 @@ static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
-	if (start > end)
+	if (start > end || start == 0)
 		return ATT_ECODE_INVALID_HANDLE;
 
 	q = queue_new();
@@ -4871,6 +4874,9 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_REQ_NOT_SUPP;
 	}
 
+	if (handle == 0)
+		return ATT_ECODE_INVALID_HANDLE;
+
 	data = new0(struct pending_request, 1);
 	if (!data)
 		return ATT_ECODE_INSUFF_RESOURCES;
@@ -4950,6 +4956,9 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
+	if (start > end || start == 0)
+		return ATT_ECODE_INVALID_HANDLE;
+
 	q = queue_new();
 	if (!q)
 		return ATT_ECODE_UNLIKELY;
@@ -5019,6 +5028,9 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
+	if (start > end || start == 0)
+		return ATT_ECODE_INVALID_HANDLE;
+
 	q = queue_new();
 	if (!q)
 		return ATT_ECODE_UNLIKELY;
@@ -5072,6 +5084,9 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return;
 
+	if (handle == 0)
+		return;
+
 	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
 		return;
 
@@ -5100,6 +5115,9 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 
 	len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
 
+	if (handle == 0)
+		return;
+
 	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
 		return;
 
@@ -5142,6 +5160,9 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
+	if (handle == 0)
+		return ATT_ECODE_INVALID_HANDLE;
+
 	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
 		return ATT_ECODE_ATTR_NOT_FOUND;
 
@@ -5191,6 +5212,9 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
 	if (!len)
 		return ATT_ECODE_INVALID_PDU;
 
+	if (handle == 0)
+		return ATT_ECODE_INVALID_HANDLE;
+
 	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
 		return ATT_ECODE_ATTR_NOT_FOUND;
 
-- 
1.9.0

Re: [PATCHv2 1/3] android/gatt: Fix parallel reading/writing attributes values from applications

[Szymon Janc]

Hi Marcin,

On Friday 30 of May 2014 11:37:40 Marcin Kraglak wrote:
> It is needed because in some cases we send few read requests to
> applications.

Please add example of those cases in commit message.

> Now all transactions data are overriden by last one, and if
> application wants to respond to previous requests, transaction data is not
> found.
> ---
>  android/gatt.c | 85
> ++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed,
> 59 insertions(+), 26 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 66d6224..80d9f30 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -106,9 +106,6 @@ struct gatt_app {
> 
>  	/* Valid for client applications */
>  	struct queue *notifications;
> -
> -	/* Transaction data valid for server application */
> -	struct pending_trans_data trans_id;
>  };
> 
>  struct element_id {
> @@ -175,6 +172,7 @@ struct gatt_device {
>  struct app_connection {
>  	struct gatt_device *device;
>  	struct gatt_app *app;
> +	struct queue *transactions;
>  	int32_t id;
>  };
> 
> @@ -182,6 +180,7 @@ static struct ipc *hal_ipc = NULL;
>  static bdaddr_t adapter_addr;
>  static bool scanning = false;
>  static unsigned int advertising_cnt = 0;
> +static int32_t trans_id = 1;

This can be local to new_transaction().

> 
>  static struct queue *gatt_apps = NULL;
>  static struct queue *gatt_devices = NULL;
> @@ -855,6 +854,8 @@ static void destroy_connection(void *data)
>  	if (conn->device->conn_cnt == 0)
>  		connection_cleanup(conn->device);
> 
> +	queue_destroy(conn->transactions, free);

Shouldn't this be after cleanup label?

> +
>  cleanup:
>  	device_unref(conn->device);
>  	free(conn);
> @@ -1305,6 +1306,12 @@ static struct app_connection
> *create_connection(struct gatt_device *device, new_conn->app = app;
>  	new_conn->id = last_conn_id++;
> 
> +	new_conn->transactions = queue_new();
> +	if (!new_conn->transactions) {
> +		free(new_conn);
> +		return NULL;
> +	}
> +
>  	if (!queue_push_head(app_connections, new_conn)) {
>  		error("gatt: Cannot push client on the client queue!?");

freeing transaction is missing here.

> 
> @@ -4169,26 +4176,29 @@ done:
>  		send_dev_pending_response(dev, opcode);
>  }
> 
> -static void set_trans_id(struct gatt_app *app, unsigned int id, int8_t
> opcode) +static struct pending_trans_data *new_transaction(unsigned int id,
> +								uint8_t opcode)
>  {
> -	app->trans_id.id = id;
> -	app->trans_id.opcode = opcode;
> -}
> +	struct pending_trans_data *transaction;
> 
> -static void clear_trans_id(struct gatt_app *app)
> -{
> -	app->trans_id.id = 0;
> -	app->trans_id.opcode = 0;
> +	transaction = new0(struct pending_trans_data, 1);
> +	if (!transaction)
> +		return NULL;
> +
> +	transaction->id = id;
> +	transaction->opcode = opcode;
> +
> +	return transaction;
>  }
> 
>  static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>  					bdaddr_t *bdaddr, void *user_data)
>  {
> +	struct pending_trans_data *transaction;
>  	struct hal_ev_gatt_server_request_read ev;
>  	struct gatt_app *app;
>  	struct app_connection *conn;
>  	int32_t id = PTR_TO_INT(user_data);
> -	static int32_t trans_id = 1;
> 
>  	app = find_app_by_id(id);
>  	if (!app) {
> @@ -4205,14 +4215,23 @@ static void read_cb(uint16_t handle, uint16_t
> offset, uint8_t att_opcode, memset(&ev, 0, sizeof(ev));
> 
>  	/* Store the request data, complete callback and transaction id */
> -	set_trans_id(app, trans_id++, att_opcode);
> +	transaction = new_transaction(trans_id++, att_opcode);
> +	if (!transaction) {
> +		error("gatt: failed to allocate memory for transaction");
> +		goto failed;
> +	}
> +
> +	if (!queue_push_tail(conn->transactions, transaction)) {
> +		free(transaction);
> +		goto failed;
> +	}
> 
>  	bdaddr2android(bdaddr, ev.bdaddr);
>  	ev.conn_id = conn->id;
>  	ev.attr_handle = handle;
>  	ev.offset = offset;
>  	ev.is_long = att_opcode == ATT_OP_READ_BLOB_REQ;
> -	ev.trans_id = app->trans_id.id;
> +	ev.trans_id = transaction->id;
> 
>  	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
>  					HAL_EV_GATT_SERVER_REQUEST_READ,
> @@ -4232,9 +4251,9 @@ static void write_cb(uint16_t handle, uint16_t offset,
> {
>  	uint8_t buf[IPC_MTU];
>  	struct hal_ev_gatt_server_request_write *ev = (void *) buf;
> +	struct pending_trans_data *transaction;
>  	struct gatt_app *app;
>  	int32_t id = PTR_TO_INT(user_data);
> -	static int32_t trans_id = 1;
>  	struct app_connection *conn;
> 
>  	app = find_app_by_id(id);
> @@ -4250,7 +4269,16 @@ static void write_cb(uint16_t handle, uint16_t
> offset, }
> 
>  	/* Store the request data, complete callback and transaction id */
> -	set_trans_id(app, trans_id++, att_opcode);

Maybe this...

> +	transaction = new_transaction(trans_id++, att_opcode);
> +	if (!transaction) {
> +		error("gatt: failed to allocate memory for transaction");
> +		goto failed;
> +	}
> +
> +	if (!queue_push_tail(conn->transactions, transaction)) {
> +		free(transaction);
> +		goto failed;
> +	}

...could be factored out to some helper eg.
bool conn_add_transaction(conn, opcode)?

> 
>  	/* TODO figure it out */
>  	if (att_opcode == ATT_OP_EXEC_WRITE_REQ)
> @@ -4263,7 +4291,7 @@ static void write_cb(uint16_t handle, uint16_t offset,
> ev->offset = offset;
> 
>  	ev->conn_id = conn->id;
> -	ev->trans_id = app->trans_id.id;
> +	ev->trans_id = transaction->id;
> 
>  	ev->is_prep = att_opcode == ATT_OP_PREP_WRITE_REQ;
>  	ev->need_rsp = att_opcode == ATT_OP_WRITE_REQ;
> @@ -4557,11 +4585,18 @@ reply:
>  				HAL_OP_GATT_SERVER_SEND_INDICATION, status);
>  }
> 
> +static bool match_trans_id(const void *data, const void *user_data)
> +{
> +	const struct pending_trans_data *transaction = data;
> +
> +	return transaction->id == PTR_TO_UINT(user_data);
> +}
> +
>  static void handle_server_send_response(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_gatt_server_send_response *cmd = buf;
> +	struct pending_trans_data *transaction;
>  	struct app_connection *conn;
> -	struct gatt_app *app;
>  	uint8_t status;
> 
>  	DBG("");
> @@ -4573,22 +4608,20 @@ static void handle_server_send_response(const void
> *buf, uint16_t len) goto reply;
>  	}
> 
> -	app = conn->app;
> -
> -	if ((unsigned int)cmd->trans_id != app->trans_id.id) {
> -		error("gatt: transaction ID mismatch (%d!=%d)",
> -					cmd->trans_id, app->trans_id.id);
> -
> +	transaction = queue_remove_if(conn->transactions, match_trans_id,
> +						UINT_TO_PTR(cmd->trans_id));
> +	if (!transaction) {
> +		error("gatt: transaction ID = %d not found", cmd->trans_id);
>  		status = HAL_STATUS_FAILED;
>  		goto reply;
>  	}
> 
> -	send_gatt_response(conn->app->trans_id.opcode, cmd->handle, cmd->offset,
> +	send_gatt_response(transaction->opcode, cmd->handle, cmd->offset,
>  					cmd->status, cmd->len, cmd->data,
>  					&conn->device->bdaddr);
> 
>  	/* Clean request data */
> -	clear_trans_id(app);
> +	free(transaction);
> 
>  	status = HAL_STATUS_SUCCESS;

-- 
BR
Szymon Janc

Re: [PATCHv2 2/3] shared/gatt: Return bool in gatt_db_get_attribute_permissions

[Szymon Janc]

On Friday 30 of May 2014 11:37:41 Marcin Kraglak wrote:
> It will return true if attribute with given handle exists in db and
> set permissions value, otherwise it will return false.
> Now, if get_permissions failed, we should reply with
> ATT_ECODE_ATTR_NOT_FOUND.
> ---
>  android/gatt.c       | 23 +++++++++++++++++------
>  src/shared/gatt-db.c | 11 +++++++----
>  src/shared/gatt-db.h |  3 ++-
>  3 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 80d9f30..421cae4 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4070,8 +4070,13 @@ static void read_requested_attributes(void *data,
> void *user_data) uint8_t *value;
>  	int value_len;
> 
> -	permissions = gatt_db_get_attribute_permissions(gatt_db,
> -							resp_data->handle);
> +	if (!gatt_db_get_attribute_permissions(gatt_db,
> +							resp_data->handle,
> +							&permissions)) {
> +		resp_data->error = ATT_ECODE_ATTR_NOT_FOUND;
> +		resp_data->state = REQUEST_DONE;
> +		return;
> +	}
> 
>  	/*
>  	 * Check if it is attribute we didn't declare permissions, like service
> @@ -5067,7 +5072,9 @@ static void write_cmd_request(const uint8_t *cmd,
> uint16_t cmd_len, if (!len)
>  		return;
> 
> -	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
> +	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
> +		return;
> +
>  	if (check_device_permissions(dev, cmd[0], permissions))
>  		return;
> 
> @@ -5093,7 +5100,9 @@ static void write_signed_cmd_request(const uint8_t
> *cmd, uint16_t cmd_len,
> 
>  	len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
> 
> -	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
> +	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
> +		return;
> +
>  	if (check_device_permissions(dev, cmd[0], permissions))
>  		return;
> 
> @@ -5133,7 +5142,8 @@ static uint8_t write_req_request(const uint8_t *cmd,
> uint16_t cmd_len, if (!len)
>  		return ATT_ECODE_INVALID_PDU;
> 
> -	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
> +	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
> +		return ATT_ECODE_ATTR_NOT_FOUND;
> 
>  	error = check_device_permissions(dev, cmd[0], permissions);
>  	if (error)
> @@ -5181,7 +5191,8 @@ static uint8_t write_prep_request(const uint8_t *cmd,
> uint16_t cmd_len, if (!len)
>  		return ATT_ECODE_INVALID_PDU;
> 
> -	permissions = gatt_db_get_attribute_permissions(gatt_db, handle);
> +	if (!gatt_db_get_attribute_permissions(gatt_db, handle, &permissions))
> +		return ATT_ECODE_ATTR_NOT_FOUND;
> 
>  	error = check_device_permissions(dev, cmd[0], permissions);
>  	if (error)
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 998e93e..c11c5d1 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -734,7 +734,8 @@ uint16_t gatt_db_get_end_handle(struct gatt_db *db,
> uint16_t handle) return service->attributes[0]->handle +
> service->num_handles - 1; }
> 
> -uint32_t gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t
> handle) +bool gatt_db_get_attribute_permissions(struct gatt_db *db,
> uint16_t handle, +							uint32_t *permissions)
>  {
>  	struct gatt_db_attribute *attribute;
>  	struct gatt_db_service *service;
> @@ -743,7 +744,7 @@ uint32_t gatt_db_get_attribute_permissions(struct
> gatt_db *db, uint16_t handle) service = queue_find(db->services,
> find_service_for_handle,
>  							INT_TO_PTR(handle));
>  	if (!service)
> -		return 0;
> +		return false;
> 
>  	service_handle = service->attributes[0]->handle;
> 
> @@ -754,7 +755,9 @@ uint32_t gatt_db_get_attribute_permissions(struct
> gatt_db *db, uint16_t handle) */
>  	attribute = service->attributes[handle - service_handle];
>  	if (!attribute)
> -		return 0;
> +		return false;
> +
> +	*permissions = attribute->permissions;
> +	return true;
> 
> -	return attribute->permissions;
>  }
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index f2f2f4d..a88f637 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -92,4 +92,5 @@ const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db
> *db,
> 
>  uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle);
> 
> -uint32_t gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t
> handle); +bool gatt_db_get_attribute_permissions(struct gatt_db *db,
> uint16_t handle, +							uint32_t *permissions);

Patches 2/3 and 3/3 are now applied, thanks.

-- 
BR
Szymon Janc