[RFC 00/13] Connection and request handling improvements

[RFC 00/13] Connection and request handling improvements

[Jakub Tyszkowski]

Patches from 1 to 9 adds subtle improvements to the existing functionality to
prepare ground for patches 10 to 13, which adds proper handling of server app
connections.

Server app connections should exist for all devices currently connected and for
each server app registered so we properly propagate requests for reads and
writes. Such connections are not owned by server apps so they cannot disconnect
connections owned by other. But they should be notified of any connection state
changes.

For this kind of functionality app_connection received owner flag to control
disconnection permissions. Ownerless connections are created automatically for
every device if server app registers. and for every server app on device
connecting for proper event propagation purpose.

This fixes some issues spotted on the UPF.

Jakub Tyszkowski (13):
  android/gatt: Rename app connection handling function
  android/gatt: Always check for complete response queue
  android/gatt: Dont process pending requests if queue is empty
  android/gatt: Fix double processing of pending responses list
  android/gatt: Improve response filling function
  android/gatt: Move response filling functions up
  android/gatt: Use fill_gatt_response in response processing
  android/gatt: Use common code for server and client apps unregister
  android/gatt: Rename app unregister function
  android/gatt: Factor out device disconn. part from app disconn.
  android/gatt: Create server connections on register
  android/gatt: Verify client and server disconnect callers
  android/gatt: Postpone disconnect notifications sent to server apps

 android/gatt.c | 399 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 217 insertions(+), 182 deletions(-)

--
2.0.0

[RFC 01/13] android/gatt: Rename app connection handling function

[Jakub Tyszkowski]

These are handling app connections thus we should make this clear by
proper naming.
---
 android/gatt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 3fd88fa..a1570fd 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -843,7 +843,7 @@ static void device_unref(struct gatt_device *device)
 	destroy_device(device);
 }
 
-static void destroy_connection(void *data)
+static void destroy_app_connection(void *data)
 {
 	struct app_connection *conn = data;
 
@@ -867,7 +867,7 @@ static void device_disconnect_clients(struct gatt_device *dev)
 
 	/* Remove all clients by given device's */
 	queue_remove_all(app_connections, match_connection_by_device, dev,
-							destroy_connection);
+							destroy_app_connection);
 }
 
 static void send_client_primary_notify(void *data, void *user_data)
@@ -1341,7 +1341,7 @@ static struct gatt_device *create_device(const bdaddr_t *addr)
 	return device_ref(dev);
 }
 
-static struct app_connection *create_connection(struct gatt_device *device,
+static struct app_connection *create_app_connection(struct gatt_device *device,
 						struct gatt_app *app)
 {
 	struct app_connection *new_conn;
@@ -1381,7 +1381,7 @@ static void trigger_disconnection(struct app_connection *connection)
 	if (queue_remove(app_connections, connection))
 			send_app_disconnect_notify(connection, GATT_SUCCESS);
 
-	destroy_connection(connection);
+	destroy_app_connection(connection);
 }
 
 static void app_disconnect_devices(struct gatt_app *client)
@@ -1510,7 +1510,7 @@ static uint8_t handle_connect(int32_t app_id, const bdaddr_t *addr)
 	conn = queue_find(app_connections, match_connection_by_device_and_app,
 								&conn_match);
 	if (!conn) {
-		conn = create_connection(device, app);
+		conn = create_app_connection(device, app);
 		if (!conn)
 			return HAL_STATUS_NOMEM;
 	}
@@ -5500,7 +5500,7 @@ static void create_listen_connections(void *data, void *user_data)
 
 	app = find_app_by_id(id);
 	if (app)
-		create_connection(dev, app);
+		create_app_connection(dev, app);
 }
 
 static void connect_confirm(GIOChannel *io, void *user_data)
@@ -5960,7 +5960,7 @@ void bt_gatt_unregister(void)
 	queue_destroy(gatt_apps, destroy_gatt_app);
 	gatt_apps = NULL;
 
-	queue_destroy(app_connections, destroy_connection);
+	queue_destroy(app_connections, destroy_app_connection);
 	app_connections = NULL;
 
 	queue_destroy(gatt_devices, destroy_device);
-- 
2.0.0

[RFC 02/13] android/gatt: Always check for complete response queue

[Jakub Tyszkowski]

This moves the check for pending responses to response sending function
as it should always be used with this check. Since sending only complete
response is allowed, the function name was changed to better represent
what it does.
---
 android/gatt.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index a1570fd..c173b89 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3790,7 +3790,14 @@ static bool is_service(const bt_uuid_t *type)
 	return false;
 }
 
-static void send_dev_pending_response(struct gatt_device *device,
+static bool match_pending_dev_request(const void *data, const void *user_data)
+{
+	const struct pending_request *pending_request = data;
+
+	return pending_request->state == REQUEST_PENDING;
+}
+
+static void send_dev_complete_response(struct gatt_device *device,
 								uint8_t opcode)
 {
 	size_t mtu;
@@ -3799,6 +3806,12 @@ static void send_dev_pending_response(struct gatt_device *device,
 	uint16_t len = 0;
 	uint8_t error = 0;
 
+	if (queue_find(device->pending_requests, match_pending_dev_request,
+									NULL)) {
+		DBG("Still pending requests");
+		return;
+	}
+
 	switch (opcode) {
 	case ATT_OP_READ_BY_TYPE_REQ: {
 		struct att_data_list *adl;
@@ -4021,13 +4034,6 @@ struct request_processing_data {
 	struct gatt_device *device;
 };
 
-static bool match_pending_dev_request(const void *data, const void *user_data)
-{
-	const struct pending_request *pending_request = data;
-
-	return pending_request->state == REQUEST_PENDING;
-}
-
 static bool match_dev_request_by_handle(const void *data, const void *user_data)
 {
 	const struct pending_request *handle_data = data;
@@ -4176,9 +4182,7 @@ static void process_dev_pending_requests(struct gatt_device *device,
 	queue_foreach(device->pending_requests, read_requested_attributes,
 								&process_data);
 
-	if (!queue_find(device->pending_requests,
-					match_pending_dev_request, NULL))
-		send_dev_pending_response(device, att_opcode);
+	send_dev_complete_response(device, att_opcode);
 }
 
 static void send_gatt_response(uint8_t opcode, uint16_t handle,
@@ -4221,8 +4225,7 @@ static void send_gatt_response(uint8_t opcode, uint16_t handle,
 	memcpy(entry->value, data, len);
 
 done:
-	if (!queue_find(dev->pending_requests, match_pending_dev_request, NULL))
-		send_dev_pending_response(dev, opcode);
+	send_dev_complete_response(dev, opcode);
 }
 
 static struct pending_trans_data *conn_add_transact(struct app_connection *conn,
@@ -5289,8 +5292,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_UNLIKELY;
 	}
 
-	if (!queue_find(dev->pending_requests, match_pending_dev_request, NULL))
-		send_dev_pending_response(dev, cmd[0]);
+	send_dev_complete_response(dev, cmd[0]);
 
 	return 0;
 }
-- 
2.0.0

[RFC 03/13] android/gatt: Dont process pending requests if queue is empty

[Jakub Tyszkowski]

Processing empty queue can result with crash:

bluetoothd[1670]:
    external/bluetooth/bluez/attrib/gattrib.c:g_attrib_ref() 0x6035878:
    ref=2
    02-23 21:36:05.650 I/bluetoothd( 1669): ==1670== Invalid
    read of size 1
    02-23 21:36:05.650 I/bluetoothd( 1669): ==1670==    at 0x12151E:
    send_dev_pending_response (gatt.c:3914)
    02-23 21:36:05.650 I/bluetoothd( 1669): ==1670==    by 0x121B33:
    process_dev_pending_requests (gatt.c:4228)
    02-23 21:36:05.650 I/bluetoothd( 1669): ==1670==    by 0x123955:
    att_handler (gatt.c:5049)
---
 android/gatt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index c173b89..c3dc41c 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4175,6 +4175,9 @@ static void process_dev_pending_requests(struct gatt_device *device,
 {
 	struct request_processing_data process_data;
 
+	if (queue_isempty(device->pending_requests))
+		return;
+
 	process_data.device = device;
 	process_data.opcode = att_opcode;
 
-- 
2.0.0

[RFC 04/13] android/gatt: Fix double processing of pending responses list

[Jakub Tyszkowski]

send_gat_response should have been used to only fill the response
data since when pending reponses queue was introduced and response
sending was moved to queue processing function.
---
 android/gatt.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index c3dc41c..5d01d8b 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4199,7 +4199,7 @@ static void send_gatt_response(uint8_t opcode, uint16_t handle,
 	dev = find_device_by_addr(bdaddr);
 	if (!dev) {
 		error("gatt: send_gatt_response, could not find dev");
-		goto done;
+		return;
 	}
 
 	entry = queue_find(dev->pending_requests, match_dev_request_by_handle,
@@ -4216,19 +4216,16 @@ static void send_gatt_response(uint8_t opcode, uint16_t handle,
 	entry->error = status;
 
 	if (!len)
-		goto done;
+		return;
 
 	entry->value = malloc0(len);
 	if (!entry->value) {
 		entry->error = ATT_ECODE_INSUFF_RESOURCES;
 
-		goto done;
+		return;
 	}
 
 	memcpy(entry->value, data, len);
-
-done:
-	send_dev_complete_response(dev, opcode);
 }
 
 static struct pending_trans_data *conn_add_transact(struct app_connection *conn,
@@ -4720,6 +4717,7 @@ static void handle_server_send_response(const void *buf, uint16_t len)
 	send_gatt_response(transaction->opcode, handle, cmd->offset,
 					cmd->status, cmd->len, cmd->data,
 					&conn->device->bdaddr);
+	send_dev_complete_response(conn->device, transaction->opcode);
 
 done:
 	/* Clean request data */
-- 
2.0.0

[RFC 05/13] android/gatt: Improve response filling function

[Jakub Tyszkowski]

Some functionality was extracted so it can be used from functions
already having device and pending response pointers without redundant
search. Parameters order was improved and some were removed. Function
name was changed as the real sending is done elsewere.
---
 android/gatt.c | 69 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 5d01d8b..fe8655a 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4188,44 +4188,44 @@ static void process_dev_pending_requests(struct gatt_device *device,
 	send_dev_complete_response(device, att_opcode);
 }
 
-static void send_gatt_response(uint8_t opcode, uint16_t handle,
+static void fill_gatt_response(struct pending_request *request, uint16_t handle,
 					uint16_t offset, uint8_t status,
-					uint16_t len, const uint8_t *data,
-					bdaddr_t *bdaddr)
+					uint16_t len, const uint8_t *data)
 {
-	struct gatt_device *dev;
-	struct pending_request *entry;
+	request->handle = handle;
+	request->offset = offset;
+	request->length = len;
+	request->state = REQUEST_DONE;
+	request->error = status;
 
-	dev = find_device_by_addr(bdaddr);
-	if (!dev) {
-		error("gatt: send_gatt_response, could not find dev");
+	if (!len)
 		return;
-	}
 
-	entry = queue_find(dev->pending_requests, match_dev_request_by_handle,
-							UINT_TO_PTR(handle));
-	if (!entry) {
-		DBG("No pending response found! Bogus android response?");
+	request->value = malloc0(len);
+	if (!request->value) {
+		request->error = ATT_ECODE_INSUFF_RESOURCES;
+
 		return;
 	}
 
-	entry->handle = handle;
-	entry->offset = offset;
-	entry->length = len;
-	entry->state = REQUEST_DONE;
-	entry->error = status;
-
-	if (!len)
-		return;
+	memcpy(request->value, data, len);
+}
 
-	entry->value = malloc0(len);
-	if (!entry->value) {
-		entry->error = ATT_ECODE_INSUFF_RESOURCES;
+static void fill_gatt_response_by_handle(uint16_t handle, uint16_t offset,
+						uint8_t status, uint16_t len,
+						const uint8_t *data,
+						struct gatt_device *dev)
+{
+	struct pending_request *entry;
 
+	entry = queue_find(dev->pending_requests, match_dev_request_by_handle,
+							UINT_TO_PTR(handle));
+	if (entry) {
+		DBG("No pending response found! Bogus android response?");
 		return;
 	}
 
-	memcpy(entry->value, data, len);
+	fill_gatt_response(entry, handle, offset, status, len, data);
 }
 
 static struct pending_trans_data *conn_add_transact(struct app_connection *conn,
@@ -4257,6 +4257,7 @@ static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 	struct gatt_app *app;
 	struct app_connection *conn;
 	int32_t id = PTR_TO_INT(user_data);
+	struct gatt_device *dev;
 
 	app = find_app_by_id(id);
 	if (!app) {
@@ -4291,8 +4292,10 @@ static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 	return;
 
 failed:
-	send_gatt_response(att_opcode, handle, 0, ATT_ECODE_UNLIKELY, 0,
-							NULL, bdaddr);
+	dev = find_device_by_addr(bdaddr);
+	if (dev)
+		fill_gatt_response_by_handle(handle, 0, ATT_ECODE_UNLIKELY, 0,
+							NULL, dev);
 }
 
 static void write_cb(uint16_t handle, uint16_t offset,
@@ -4306,6 +4309,7 @@ static void write_cb(uint16_t handle, uint16_t offset,
 	struct gatt_app *app;
 	int32_t id = PTR_TO_INT(user_data);
 	struct app_connection *conn;
+	struct gatt_device *dev;
 
 	app = find_app_by_id(id);
 	if (!app) {
@@ -4352,8 +4356,10 @@ static void write_cb(uint16_t handle, uint16_t offset,
 	return;
 
 failed:
-	send_gatt_response(att_opcode, handle, 0, ATT_ECODE_UNLIKELY, 0, NULL,
-								bdaddr);
+	dev = find_device_by_addr(bdaddr);
+	if (dev)
+		fill_gatt_response_by_handle(handle, 0, ATT_ECODE_UNLIKELY, 0,
+								NULL, dev);
 }
 
 static uint32_t android_to_gatt_permissions(int32_t hal_permissions)
@@ -4714,9 +4720,8 @@ static void handle_server_send_response(const void *buf, uint16_t len)
 		 */
 	}
 
-	send_gatt_response(transaction->opcode, handle, cmd->offset,
-					cmd->status, cmd->len, cmd->data,
-					&conn->device->bdaddr);
+	fill_gatt_response_by_handle(handle, cmd->offset, cmd->status, cmd->len,
+						cmd->data, conn->device);
 	send_dev_complete_response(conn->device, transaction->opcode);
 
 done:
-- 
2.0.0

[RFC 06/13] android/gatt: Move response filling functions up

[Jakub Tyszkowski]

Its needed to be called from read_requested_attributes.
---
 android/gatt.c | 80 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index fe8655a..35a163d 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4112,6 +4112,46 @@ static uint8_t check_device_permissions(struct gatt_device *device,
 	return 0;
 }
 
+static void fill_gatt_response(struct pending_request *request, uint16_t handle,
+					uint16_t offset, uint8_t status,
+					uint16_t len, const uint8_t *data)
+{
+	request->handle = handle;
+	request->offset = offset;
+	request->length = len;
+	request->state = REQUEST_DONE;
+	request->error = status;
+
+	if (!len)
+		return;
+
+	request->value = malloc0(len);
+	if (!request->value) {
+		request->error = ATT_ECODE_INSUFF_RESOURCES;
+
+		return;
+	}
+
+	memcpy(request->value, data, len);
+}
+
+static void fill_gatt_response_by_handle(uint16_t handle, uint16_t offset,
+						uint8_t status, uint16_t len,
+						const uint8_t *data,
+						struct gatt_device *dev)
+{
+	struct pending_request *entry;
+
+	entry = queue_find(dev->pending_requests, match_dev_request_by_handle,
+							UINT_TO_PTR(handle));
+	if (entry) {
+		DBG("No pending response found! Bogus android response?");
+		return;
+	}
+
+	fill_gatt_response(entry, handle, offset, status, len, data);
+}
+
 static void read_requested_attributes(void *data, void *user_data)
 {
 	struct pending_request *resp_data = data;
@@ -4188,46 +4228,6 @@ static void process_dev_pending_requests(struct gatt_device *device,
 	send_dev_complete_response(device, att_opcode);
 }
 
-static void fill_gatt_response(struct pending_request *request, uint16_t handle,
-					uint16_t offset, uint8_t status,
-					uint16_t len, const uint8_t *data)
-{
-	request->handle = handle;
-	request->offset = offset;
-	request->length = len;
-	request->state = REQUEST_DONE;
-	request->error = status;
-
-	if (!len)
-		return;
-
-	request->value = malloc0(len);
-	if (!request->value) {
-		request->error = ATT_ECODE_INSUFF_RESOURCES;
-
-		return;
-	}
-
-	memcpy(request->value, data, len);
-}
-
-static void fill_gatt_response_by_handle(uint16_t handle, uint16_t offset,
-						uint8_t status, uint16_t len,
-						const uint8_t *data,
-						struct gatt_device *dev)
-{
-	struct pending_request *entry;
-
-	entry = queue_find(dev->pending_requests, match_dev_request_by_handle,
-							UINT_TO_PTR(handle));
-	if (entry) {
-		DBG("No pending response found! Bogus android response?");
-		return;
-	}
-
-	fill_gatt_response(entry, handle, offset, status, len, data);
-}
-
 static struct pending_trans_data *conn_add_transact(struct app_connection *conn,
 								uint8_t opcode)
 {
-- 
2.0.0

[RFC 07/13] android/gatt: Use fill_gatt_response in response processing

[Jakub Tyszkowski]

This makes use of fill_gatt_response in function doing reads on queue
with pending responses.
---
 android/gatt.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 35a163d..76844f7 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4157,8 +4157,8 @@ static void read_requested_attributes(void *data, void *user_data)
 	struct pending_request *resp_data = data;
 	struct request_processing_data *process_data = user_data;
 	uint32_t permissions;
-	uint8_t *value;
-	int value_len;
+	uint8_t *value, error;
+	int value_len = 0;
 
 	if (!gatt_db_get_attribute_permissions(gatt_db, resp_data->handle,
 								&permissions)) {
@@ -4174,40 +4174,27 @@ static void read_requested_attributes(void *data, void *user_data)
 	if (permissions == 0)
 		permissions = GATT_PERM_READ;
 
-	resp_data->error = check_device_permissions(process_data->device,
+	error = check_device_permissions(process_data->device,
 							process_data->opcode,
 							permissions);
-	if (resp_data->error) {
-		resp_data->state = REQUEST_DONE;
-		return;
-	}
+	if (error)
+		goto done;
 
 	if (!gatt_db_read(gatt_db, resp_data->handle,
 						resp_data->offset,
 						process_data->opcode,
 						&process_data->device->bdaddr,
 						&value, &value_len)) {
-		resp_data->state = REQUEST_DONE;
-		resp_data->error = ATT_ECODE_UNLIKELY;
-		return;
+		error = ATT_ECODE_UNLIKELY;
+		goto done;
 	}
 
+done:
 	/* We have value here already if no callback will be called */
-	if (value_len >= 0) {
-		resp_data->state = REQUEST_DONE;
-
-		resp_data->value = malloc0(value_len);
-		if (!resp_data->value) {
-			/* If data cannot be copied, act like when read fails */
-			resp_data->error = ATT_ECODE_INSUFF_RESOURCES;
-			return;
-		}
-
-		memcpy(resp_data->value, value, value_len);
-		resp_data->length = value_len;
-	} else if (resp_data->state == REQUEST_INIT) {
-		resp_data->state = REQUEST_PENDING;
-	}
+	if (value_len >= 0)
+		fill_gatt_response(resp_data, resp_data->handle,
+					resp_data->offset, error, value_len,
+					value);
 }
 
 static void process_dev_pending_requests(struct gatt_device *device,
-- 
2.0.0

[RFC 08/13] android/gatt: Use common code for server and client apps unregister

[Jakub Tyszkowski]

---
 android/gatt.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 76844f7..7d22901 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3640,21 +3640,11 @@ static void handle_server_unregister(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_server_unregister *cmd = buf;
 	uint8_t status;
-	struct gatt_app *server;
 
 	DBG("");
 
-	server = find_app_by_id(cmd->server_if);
-	if (!server) {
-		error("gatt: server_if=%d not found", cmd->server_if);
-		status = HAL_STATUS_FAILED;
-		goto failed;
-	}
-
-	destroy_gatt_app(server);
-	status = HAL_STATUS_SUCCESS;
+	status = unregister_client(cmd->server_if);
 
-failed:
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 					HAL_OP_GATT_SERVER_UNREGISTER, status);
 }
-- 
2.0.0

[RFC 09/13] android/gatt: Rename app unregister function

[Jakub Tyszkowski]

Its used for client and server apps unregistering
---
 android/gatt.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7d22901..558b203 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1423,7 +1423,7 @@ static bool trigger_connection(struct app_connection *connection)
 	return true;
 }
 
-static uint8_t unregister_client(int client_if)
+static uint8_t unregister_app(int client_if)
 {
 	struct gatt_app *cl;
 
@@ -1434,10 +1434,7 @@ static uint8_t unregister_client(int client_if)
 		return HAL_STATUS_FAILED;
 	}
 
-	/*
-	 * Check if there is any connect request or connected device
-	 * for this client. If so, remove this client from those lists.
-	 */
+	/* Destroy app connections with proper notifications for this app. */
 	app_disconnect_devices(cl);
 	destroy_gatt_app(cl);
 
@@ -1451,7 +1448,7 @@ static void handle_client_unregister(const void *buf, uint16_t len)
 
 	DBG("");
 
-	status = unregister_client(cmd->client_if);
+	status = unregister_app(cmd->client_if);
 
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 					HAL_OP_GATT_CLIENT_UNREGISTER, status);
@@ -3577,7 +3574,7 @@ static void handle_client_test_command(const void *buf, uint16_t len)
 			else
 				status = HAL_STATUS_FAILED;
 		} else {
-			status = unregister_client(test_client_if);
+			status = unregister_app(test_client_if);
 			test_client_if = 0;
 		}
 		break;
@@ -3643,7 +3640,7 @@ static void handle_server_unregister(const void *buf, uint16_t len)
 
 	DBG("");
 
-	status = unregister_client(cmd->server_if);
+	status = unregister_app(cmd->server_if);
 
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 					HAL_OP_GATT_SERVER_UNREGISTER, status);
-- 
2.0.0

[RFC 10/13] android/gatt: Factor out device disconn. part from app disconn.

[Jakub Tyszkowski]

Device disconnection will be needed to cleanup incomming connections
when acting as server.
---
 android/gatt.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 558b203..4bc4f35 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -843,20 +843,22 @@ static void device_unref(struct gatt_device *device)
 	destroy_device(device);
 }
 
+static void disconnect_device(struct gatt_device *dev)
+{
+	dev->conn_cnt--;
+	if (dev->conn_cnt == 0)
+		connection_cleanup(dev);
+
+	device_unref(dev);
+}
+
 static void destroy_app_connection(void *data)
 {
 	struct app_connection *conn = data;
 
-	if (!queue_find(gatt_devices, match_by_value, conn->device))
-		goto cleanup;
-
-	conn->device->conn_cnt--;
-	if (conn->device->conn_cnt == 0)
-		connection_cleanup(conn->device);
+	disconnect_device(conn->device);
 
-cleanup:
 	queue_destroy(conn->transactions, free);
-	device_unref(conn->device);
 	free(conn);
 }
 
-- 
2.0.0

[RFC 11/13] android/gatt: Create server connections on register

[Jakub Tyszkowski]

We need those to make server apps aware of current connections.
---
 android/gatt.c | 179 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 115 insertions(+), 64 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 4bc4f35..9c2f3ba 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -173,6 +173,7 @@ struct app_connection {
 	int32_t id;
 
 	bool wait_execute_write;
+	bool owner;
 };
 
 static struct ipc *hal_ipc = NULL;
@@ -565,6 +566,9 @@ static void device_set_state(struct gatt_device *dev, uint32_t state)
 	dev->state = state;
 }
 
+static void disconnect_notify_by_device(void *data, void *user_data);
+static void destroy_app_connection(void *data);
+
 static void connection_cleanup(struct gatt_device *device)
 {
 	if (device->watch_id) {
@@ -589,6 +593,16 @@ static void connection_cleanup(struct gatt_device *device)
 		g_attrib_unref(attrib);
 	}
 
+	if (queue_find(app_connections, match_connection_by_device, device)) {
+		/* Notify disconnection to all clients */
+		queue_foreach(app_connections, disconnect_notify_by_device,
+									device);
+
+		/* Remove all clients by given device's */
+		queue_remove_all(app_connections, match_connection_by_device,
+						device, destroy_app_connection);
+	}
+
 	/*
 	 * If device was in connection_pending or connectable state we
 	 * search device list if we should stop the scan.
@@ -771,7 +785,7 @@ static void disconnect_notify_by_device(void *data, void *user_data)
 	struct app_connection *conn = data;
 	struct gatt_device *dev = user_data;
 
-	if (dev != conn->device)
+	if (dev != conn->device && conn->app->type == APP_CLIENT)
 		return;
 
 	if (dev->state == DEVICE_CONNECTED)
@@ -848,28 +862,19 @@ static void disconnect_device(struct gatt_device *dev)
 	dev->conn_cnt--;
 	if (dev->conn_cnt == 0)
 		connection_cleanup(dev);
-
-	device_unref(dev);
 }
 
 static void destroy_app_connection(void *data)
 {
 	struct app_connection *conn = data;
 
-	disconnect_device(conn->device);
+	if (conn->owner)
+		disconnect_device(conn->device);
 
 	queue_destroy(conn->transactions, free);
-	free(conn);
-}
 
-static void device_disconnect_clients(struct gatt_device *dev)
-{
-	/* Notify disconnection to all clients */
-	queue_foreach(app_connections, disconnect_notify_by_device, dev);
-
-	/* Remove all clients by given device's */
-	queue_remove_all(app_connections, match_connection_by_device, dev,
-							destroy_app_connection);
+	device_unref(conn->device);
+	free(conn);
 }
 
 static void send_client_primary_notify(void *data, void *user_data)
@@ -991,7 +996,19 @@ static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
 	if (!getsockopt(sock, SOL_SOCKET, SO_ERROR, &err, &len))
 		DBG("%s (%d)", strerror(err), err);
 
-	device_disconnect_clients(dev);
+	if (queue_find(app_connections, match_connection_by_device, dev)) {
+		/* Notify disconnection to all clients */
+		queue_foreach(app_connections, disconnect_notify_by_device,
+									dev);
+
+		/* Remove all clients by given device's */
+		queue_remove_all(app_connections, match_connection_by_device,
+						dev, destroy_app_connection);
+	}
+
+	/* In case of incoming connection (no owner) we clean it up manually */
+	if (!queue_find(app_connections, match_connection_by_device, dev))
+		connection_cleanup(dev);
 
 	return FALSE;
 }
@@ -1006,7 +1023,9 @@ static void send_app_connect_notifications(void *data, void *user_data)
 	struct app_connection *conn = data;
 	struct connect_data *con_data = user_data;
 
-	if (conn->device == con_data->dev)
+
+	if ((conn->device == con_data->dev && conn->owner) ||
+						conn->app->type == APP_SERVER)
 		send_app_connect_notify(conn, con_data->status);
 }
 
@@ -1113,6 +1132,59 @@ static void notify_att_range_change(struct gatt_device *dev,
 		g_attrib_send(dev->attrib, 0, pdu, length, NULL, NULL, NULL);
 }
 
+static struct app_connection *create_app_connection(struct gatt_device *device,
+							struct gatt_app *app)
+{
+	struct app_connection *new_conn;
+	static int32_t last_conn_id = 1;
+
+	/* Check if already connected */
+	new_conn = new0(struct app_connection, 1);
+	if (!new_conn)
+		return NULL;
+
+	/* Make connection id unique to connection record (app, device) pair */
+	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!?");
+		queue_destroy(new_conn->transactions, free);
+		free(new_conn);
+		return NULL;
+	}
+
+	new_conn->device = device_ref(device);
+
+	if (device->state == DEVICE_CONNECTED)
+		send_app_connect_notify(new_conn, GATT_SUCCESS);
+
+	return new_conn;
+}
+
+static void create_server_app_connection(void *data, void *user_data)
+{
+	struct app_connection match;
+
+	match.app = data;
+	match.device = user_data;
+
+	if (match.app->type == APP_CLIENT)
+		return;
+
+	if (queue_find(app_connections, match_connection_by_device_and_app,
+									&match))
+		return;
+
+	create_app_connection(match.device, match.app);
+}
+
 static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 {
 	struct gatt_device *dev = user_data;
@@ -1184,9 +1256,14 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 reply:
 	data.dev = dev;
 	data.status = status;
-	queue_foreach(app_connections, send_app_connect_notifications, &data);
 	device_unref(dev);
 
+	queue_foreach(app_connections, send_app_connect_notifications, &data);
+
+	/* Create connection for server apps without it */
+	if (!status)
+		queue_foreach(gatt_apps, create_server_app_connection, dev);
+
 	/* Check if we should restart scan */
 	if (scanning)
 		bt_le_discovery_start(le_device_found_handler);
@@ -1343,40 +1420,6 @@ static struct gatt_device *create_device(const bdaddr_t *addr)
 	return device_ref(dev);
 }
 
-static struct app_connection *create_app_connection(struct gatt_device *device,
-						struct gatt_app *app)
-{
-	struct app_connection *new_conn;
-	static int32_t last_conn_id = 1;
-
-	/* Check if already connected */
-	new_conn = new0(struct app_connection, 1);
-	if (!new_conn)
-		return NULL;
-
-	/* Make connection id unique to connection record (app, device) pair */
-	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!?");
-		queue_destroy(new_conn->transactions, free);
-		free(new_conn);
-		return NULL;
-	}
-
-	new_conn->device = device_ref(device);
-	new_conn->device->conn_cnt++;
-
-	return new_conn;
-}
-
 static void trigger_disconnection(struct app_connection *connection)
 {
 	/* Notify client */
@@ -1391,28 +1434,21 @@ static void app_disconnect_devices(struct gatt_app *client)
 	struct app_connection *conn;
 
 	/* find every connection for client record and trigger disconnect */
-	conn = queue_remove_if(app_connections, match_connection_by_app,
-									client);
+	conn = queue_find(app_connections, match_connection_by_app, client);
 	while (conn) {
 		trigger_disconnection(conn);
 
-		conn = queue_remove_if(app_connections,
-					match_connection_by_app, client);
+		conn = queue_find(app_connections, match_connection_by_app,
+									client);
 	}
 }
 
 static bool trigger_connection(struct app_connection *connection)
 {
-	switch (connection->device->state) {
-	case DEVICE_DISCONNECTED:
+	DBG("");
+
+	if (connection->device->state == DEVICE_DISCONNECTED)
 		device_set_state(connection->device, DEVICE_CONNECT_INIT);
-		break;
-	case DEVICE_CONNECTED:
-		send_app_connect_notify(connection, GATT_SUCCESS);
-		break;
-	default:
-		break;
-	}
 
 	/* after state change trigger discovering */
 	if (!scanning && (connection->device->state == DEVICE_CONNECT_INIT))
@@ -1422,6 +1458,9 @@ static bool trigger_connection(struct app_connection *connection)
 			return false;
 		}
 
+	connection->device->conn_cnt++;
+	connection->owner = true;
+
 	return true;
 }
 
@@ -3610,6 +3649,15 @@ static void handle_client_test_command(const void *buf, uint16_t len)
 				HAL_OP_GATT_CLIENT_TEST_COMMAND, status);
 }
 
+static void create_server_connections(void *data, void *user_data)
+{
+	struct gatt_device *dev = data;
+	struct gatt_app *app = user_data;
+
+	if (dev->state == DEVICE_CONNECTED)
+		create_app_connection(dev, app);
+}
+
 static void handle_server_register(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_server_register *cmd = buf;
@@ -3633,6 +3681,9 @@ static void handle_server_register(const void *buf, uint16_t len)
 
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT, HAL_OP_GATT_SERVER_REGISTER,
 							HAL_STATUS_SUCCESS);
+
+	queue_foreach(gatt_devices, create_server_connections,
+						find_app_by_id(ev.server_if));
 }
 
 static void handle_server_unregister(const void *buf, uint16_t len)
-- 
2.0.0

[RFC 12/13] android/gatt: Verify client and server disconnect callers

[Jakub Tyszkowski]

This checks if connection with provided ID actually belongs to the
client or server apps. Previously server_disconnect could be called with
clients connection ID and vice versa.
---
 android/gatt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 9c2f3ba..9607416 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1587,7 +1587,7 @@ static void handle_client_disconnect(const void *buf, uint16_t len)
 
 	/* TODO: should we care to match also bdaddr when conn_id is unique? */
 	conn = find_connection_by_id(cmd->conn_id);
-	if (conn)
+	if (conn && conn->app->type == APP_CLIENT)
 		trigger_disconnection(conn);
 
 	status = HAL_STATUS_SUCCESS;
@@ -3725,7 +3725,7 @@ static void handle_server_disconnect(const void *buf, uint16_t len)
 
 	/* TODO: should we care to match also bdaddr when conn_id is unique? */
 	conn = find_connection_by_id(cmd->conn_id);
-	if (conn)
+	if (conn && conn->app->type == APP_SERVER)
 		trigger_disconnection(conn);
 
 	status = HAL_STATUS_SUCCESS;
-- 
2.0.0

[RFC 13/13] android/gatt: Postpone disconnect notifications sent to server apps

[Jakub Tyszkowski]

Bluedroid postpones server app disconnection callbacks until last
connection owner has disconnected instead of sending it when server app
calls disconnect.
---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 9607416..678d1b2 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3725,7 +3725,7 @@ static void handle_server_disconnect(const void *buf, uint16_t len)
 
 	/* TODO: should we care to match also bdaddr when conn_id is unique? */
 	conn = find_connection_by_id(cmd->conn_id);
-	if (conn && conn->app->type == APP_SERVER)
+	if (conn && conn->owner && conn->app->type == APP_SERVER)
 		trigger_disconnection(conn);
 
 	status = HAL_STATUS_SUCCESS;
-- 
2.0.0

Re: [RFC 00/13] Connection and request handling improvements

[Szymon Janc]

Hi Jakub,

On Friday 06 of June 2014 15:46:13 Jakub Tyszkowski wrote:
> Patches from 1 to 9 adds subtle improvements to the existing functionality to
> prepare ground for patches 10 to 13, which adds proper handling of server app
> connections.
> 
> Server app connections should exist for all devices currently connected and for
> each server app registered so we properly propagate requests for reads and
> writes. Such connections are not owned by server apps so they cannot disconnect
> connections owned by other. But they should be notified of any connection state
> changes.
> 
> For this kind of functionality app_connection received owner flag to control
> disconnection permissions. Ownerless connections are created automatically for
> every device if server app registers. and for every server app on device
> connecting for proper event propagation purpose.
> 
> This fixes some issues spotted on the UPF.
> 
> Jakub Tyszkowski (13):
>   android/gatt: Rename app connection handling function
>   android/gatt: Always check for complete response queue
>   android/gatt: Dont process pending requests if queue is empty
>   android/gatt: Fix double processing of pending responses list
>   android/gatt: Improve response filling function
>   android/gatt: Move response filling functions up
>   android/gatt: Use fill_gatt_response in response processing
>   android/gatt: Use common code for server and client apps unregister
>   android/gatt: Rename app unregister function
>   android/gatt: Factor out device disconn. part from app disconn.
>   android/gatt: Create server connections on register
>   android/gatt: Verify client and server disconnect callers
>   android/gatt: Postpone disconnect notifications sent to server apps
> 
>  android/gatt.c | 399 +++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 217 insertions(+), 182 deletions(-)
> 

Patches 1-9 are now applied, thanks.

-- 
Best regards, 
Szymon Janc