[PATCH 01/10] android/gatt: Replace request state with boolean

[PATCH 01/10] android/gatt: Replace request state with boolean

[Jakub Tyszkowski]

As there are two states we can instead use boolean value.
---
 android/gatt.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 8e58e41..b0ce1a8 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -690,11 +690,6 @@ static void destroy_gatt_app(void *data)
 	free(app);
 }
 
-enum pend_req_state {
-	REQUEST_PENDING,
-	REQUEST_DONE,
-};
-
 struct pending_request {
 	struct gatt_db_attribute *attrib;
 	int length;
@@ -704,7 +699,7 @@ struct pending_request {
 	uint8_t *filter_value;
 	uint16_t filter_vlen;
 
-	enum pend_req_state state;
+	bool completed;
 	uint8_t error;
 };
 
@@ -4475,7 +4470,7 @@ 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;
+	return !pending_request->completed;
 }
 
 static void send_dev_complete_response(struct gatt_device *device,
@@ -4830,7 +4825,7 @@ static void attribute_read_cb(struct gatt_db_attribute *attrib, int err,
 	resp_data->length = length;
 	resp_data->error = error;
 
-	resp_data->state = REQUEST_DONE;
+	resp_data->completed = true;
 
 	if (!length)
 		return;
@@ -4856,7 +4851,7 @@ static void read_requested_attributes(void *data, void *user_data)
 	attrib = resp_data->attrib;
 	if (!attrib) {
 		resp_data->error = ATT_ECODE_ATTR_NOT_FOUND;
-		resp_data->state = REQUEST_DONE;
+		resp_data->completed = true;
 		return;
 	}
 
@@ -4874,7 +4869,7 @@ static void read_requested_attributes(void *data, void *user_data)
 							permissions);
 	if (error != 0) {
 		resp_data->error = error;
-		resp_data->state = REQUEST_DONE;
+		resp_data->completed = true;
 		return;
 	}
 
@@ -5650,7 +5645,7 @@ static void handle_server_send_response(const void *buf, uint16_t len)
 
 		/* Cast status to uint8_t, due to (byte) cast in java layer. */
 		req->error = err_to_att((uint8_t) cmd->status);
-		req->state = REQUEST_DONE;
+		req->completed = true;
 
 		/*
 		 * FIXME: Handle situation when not all server applications
@@ -6493,7 +6488,7 @@ static void attribute_write_cb(struct gatt_db_attribute *attrib, int err,
 	data->attrib = attrib;
 	data->error = error;
 
-	data->state = REQUEST_DONE;
+	data->completed = true;
 }
 
 static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
-- 
1.9.1

[PATCH 02/10] android/gatt: Destroy app connections before destroying apps

[Jakub Tyszkowski]

This is the right destruction order to avoid dangling pointers.
And it will actually matter when we make app connection destructor
automatically notify apps about disconnection.
---
 android/gatt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index b0ce1a8..f9cef4f 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -7284,12 +7284,12 @@ void bt_gatt_unregister(void)
 	ipc_unregister(hal_ipc, HAL_SERVICE_ID_GATT);
 	hal_ipc = NULL;
 
-	queue_destroy(gatt_apps, destroy_gatt_app);
-	gatt_apps = NULL;
-
 	queue_destroy(app_connections, destroy_connection);
 	app_connections = NULL;
 
+	queue_destroy(gatt_apps, destroy_gatt_app);
+	gatt_apps = NULL;
+
 	queue_destroy(gatt_devices, destroy_device);
 	gatt_devices = NULL;
 
-- 
1.9.1

[PATCH 03/10] android/gatt: Simplify app disconnection function call flow

[Jakub Tyszkowski]

We do not keep app_connections in disconnected state and disconnecting
is equal to app_connection destruction. Thus we can use more generic
'queue_remove()' with proper destructor instead of custom functions like
device_disconnect_client and app_disconnect_devices. This destructor will
notify app about disconnection.
---
 android/gatt.c | 78 +++++++++++++++-------------------------------------------
 1 file changed, 20 insertions(+), 58 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index f9cef4f..33eeba9 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -879,15 +879,14 @@ static void notify_app_connect_status(struct app_connection *conn,
 		send_server_connection_state_notify(conn, !status);
 }
 
-static void notify_app_disconnect_status_by_device(void *data, void *user_data)
+static void destroy_connection(void *data)
 {
 	struct app_connection *conn = data;
-	struct gatt_device *dev = user_data;
 
-	if (dev != conn->device || !conn->app)
-		return;
+	if (conn->timeout_id > 0)
+		g_source_remove(conn->timeout_id);
 
-	switch (dev->state) {
+	switch (conn->device->state) {
 	case DEVICE_CONNECTED:
 		notify_app_disconnect_status(conn, GATT_SUCCESS);
 		break;
@@ -898,14 +897,6 @@ static void notify_app_disconnect_status_by_device(void *data, void *user_data)
 	case DEVICE_DISCONNECTED:
 		break;
 	}
-}
-
-static void destroy_connection(void *data)
-{
-	struct app_connection *conn = data;
-
-	if (conn->timeout_id > 0)
-		g_source_remove(conn->timeout_id);
 
 	if (!queue_find(app_connections, match_connection_by_device,
 							conn->device))
@@ -916,17 +907,6 @@ static void destroy_connection(void *data)
 	free(conn);
 }
 
-static void device_disconnect_clients(struct gatt_device *dev)
-{
-	/* Notify disconnection to all clients */
-	queue_foreach(app_connections, notify_app_disconnect_status_by_device,
-									dev);
-
-	/* Remove all clients by given device's */
-	queue_remove_all(app_connections, match_connection_by_device, dev,
-							destroy_connection);
-}
-
 static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
 							gpointer user_data)
 {
@@ -939,7 +919,8 @@ 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);
+	queue_remove_all(app_connections, match_connection_by_device, dev,
+							destroy_connection);
 
 	return FALSE;
 }
@@ -1900,30 +1881,6 @@ reply:
 									status);
 }
 
-static void trigger_disconnection(struct app_connection *connection)
-{
-	/* Notify client */
-	if (queue_remove(app_connections, connection))
-			notify_app_disconnect_status(connection, GATT_SUCCESS);
-
-	destroy_connection(connection);
-}
-
-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);
-	while (conn) {
-		trigger_disconnection(conn);
-
-		conn = queue_remove_if(app_connections,
-					match_connection_by_app, client);
-	}
-}
-
 static int connect_bredr(struct gatt_device *dev)
 {
 	BtIOSecLevel sec_level;
@@ -2039,7 +1996,8 @@ static uint8_t unregister_app(int client_if)
 	}
 
 	/* Destroy app connections with proper notifications for this app. */
-	app_disconnect_devices(cl);
+	queue_remove_all(app_connections, match_connection_by_app, cl,
+							destroy_connection);
 	destroy_gatt_app(cl);
 
 	return HAL_STATUS_SUCCESS;
@@ -2207,9 +2165,10 @@ static void handle_client_disconnect(const void *buf, uint16_t len)
 	DBG("");
 
 	/* TODO: should we care to match also bdaddr when conn_id is unique? */
-	conn = find_connection_by_id(cmd->conn_id);
+	conn = queue_remove_if(app_connections, match_connection_by_id,
+						INT_TO_PTR(cmd->conn_id));
 	if (conn)
-		trigger_disconnection(conn);
+		destroy_connection(conn);
 
 	status = HAL_STATUS_SUCCESS;
 
@@ -4249,7 +4208,9 @@ static void handle_client_test_command(const void *buf, uint16_t len)
 		app = queue_find(gatt_apps, match_app_by_id,
 						INT_TO_PTR(test_client_if));
 		if (app)
-			app_disconnect_devices(app);
+			queue_remove_all(app_connections,
+						match_connection_by_app, app,
+						destroy_connection);
 
 		status = HAL_STATUS_SUCCESS;
 		break;
@@ -4342,9 +4303,10 @@ static void handle_server_disconnect(const void *buf, uint16_t len)
 	DBG("");
 
 	/* TODO: should we care to match also bdaddr when conn_id is unique? */
-	conn = find_connection_by_id(cmd->conn_id);
+	conn = queue_remove_if(app_connections, match_connection_by_id,
+						INT_TO_PTR(cmd->conn_id));
 	if (conn)
-		trigger_disconnection(conn);
+		destroy_connection(conn);
 
 	status = HAL_STATUS_SUCCESS;
 
@@ -7387,12 +7349,12 @@ bool bt_gatt_disconnect_app(unsigned int id, const bdaddr_t *addr)
 	match.device = device;
 	match.app = app;
 
-	conn = queue_find(app_connections, match_connection_by_device_and_app,
-									&match);
+	conn = queue_remove_if(app_connections,
+				match_connection_by_device_and_app, &match);
 	if (!conn)
 		return false;
 
-	trigger_disconnection(conn);
+	destroy_connection(conn);
 
 	return true;
 }
-- 
1.9.1

[PATCH 04/10] android/gatt: Make struct destructors NULL proof

[Jakub Tyszkowski]

Some destructor functions do check for NULL and for some we need to
check manually. In few cases we forgot to check at all, which may lead to
NULL dereference. With this patch we are consistent about where this
check should be done.
---
 android/gatt.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 33eeba9..7f95226 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -543,6 +543,9 @@ static void destroy_notification(void *data)
 	struct notification_data *notification = data;
 	struct gatt_app *app;
 
+	if (!notification)
+		return;
+
 	if (--notification->ref)
 		return;
 
@@ -670,6 +673,9 @@ static void destroy_gatt_app(void *data)
 {
 	struct gatt_app *app = data;
 
+	if (!app)
+		return;
+
 	/*
 	 * First we want to get all notifications and unregister them.
 	 * We don't pass unregister_notification to queue_destroy,
@@ -707,6 +713,9 @@ static void destroy_pending_request(void *data)
 {
 	struct pending_request *entry = data;
 
+	if (!entry)
+		return;
+
 	free(entry->value);
 	free(entry->filter_value);
 	free(entry);
@@ -883,6 +892,9 @@ static void destroy_connection(void *data)
 {
 	struct app_connection *conn = data;
 
+	if (!conn)
+		return;
+
 	if (conn->timeout_id > 0)
 		g_source_remove(conn->timeout_id);
 
@@ -2167,8 +2179,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 = queue_remove_if(app_connections, match_connection_by_id,
 						INT_TO_PTR(cmd->conn_id));
-	if (conn)
-		destroy_connection(conn);
+	destroy_connection(conn);
 
 	status = HAL_STATUS_SUCCESS;
 
@@ -4207,10 +4218,8 @@ static void handle_client_test_command(const void *buf, uint16_t len)
 	case GATT_CLIENT_TEST_CMD_DISCONNECT:
 		app = queue_find(gatt_apps, match_app_by_id,
 						INT_TO_PTR(test_client_if));
-		if (app)
-			queue_remove_all(app_connections,
-						match_connection_by_app, app,
-						destroy_connection);
+		queue_remove_all(app_connections, match_connection_by_app, app,
+							destroy_connection);
 
 		status = HAL_STATUS_SUCCESS;
 		break;
@@ -4305,8 +4314,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 = queue_remove_if(app_connections, match_connection_by_id,
 						INT_TO_PTR(cmd->conn_id));
-	if (conn)
-		destroy_connection(conn);
+	destroy_connection(conn);
 
 	status = HAL_STATUS_SUCCESS;
 
@@ -4488,8 +4496,7 @@ static void send_dev_complete_response(struct gatt_device *device,
 		adl = att_data_list_alloc(queue_length(temp),
 						sizeof(uint16_t) + length);
 
-		if (val)
-			destroy_pending_request(val);
+		destroy_pending_request(val);
 
 		val = queue_pop_head(temp);
 		while (val) {
-- 
1.9.1

[PATCH 05/10] android/gatt: Use g_attrib_send consistently

[Jakub Tyszkowski]

'g_attrib_send' do checks the 'length' parameter so there is no need
for us to do this (and in most cases we don't). We can test the
returned value in case we want to report error.
---
 android/gatt.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7f95226..81b0283 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1068,9 +1068,7 @@ static void notify_att_range_change(struct gatt_device *dev,
 		break;
 	}
 
-	if (length)
-		g_attrib_send(dev->attrib, 0, pdu, length,
-						confirmation_cb, NULL, NULL);
+	g_attrib_send(dev->attrib, 0, pdu, length, confirmation_cb, NULL, NULL);
 }
 
 static struct app_connection *create_connection(struct gatt_device *device,
@@ -1480,9 +1478,7 @@ static void ind_handler(const uint8_t *cmd, uint16_t cmd_len,
 	 */
 
 	resp_length = enc_confirmation(opdu, length);
-	if (resp_length)
-		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
-									NULL);
+	g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL, NULL);
 }
 
 static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
@@ -4157,12 +4153,10 @@ static uint8_t test_read_write(bdaddr_t *bdaddr, bt_uuid_t *uuid, uint16_t op,
 		return HAL_STATUS_UNSUPPORTED;
 	}
 
-	if (!length)
+	if (!g_attrib_send(dev->attrib, 0, pdu, length, test_command_result,
+								NULL, NULL))
 		return HAL_STATUS_FAILED;
 
-	g_attrib_send(dev->attrib, 0, pdu, length, test_command_result, NULL,
-									NULL);
-
 	return HAL_STATUS_SUCCESS;
 }
 
@@ -5531,12 +5525,11 @@ static void handle_server_send_indication(const void *buf, uint16_t len)
 						cmd->len, pdu, mtu);
 	}
 
-	if (length == 0) {
-		error("gatt: Failed to encode indication");
+	if (!g_attrib_send(conn->device->attrib, 0, pdu, length,
+				confirmation_cb, UINT_TO_PTR(conn->id), NULL)) {
+		error("gatt: Failed to send indication");
 		status = HAL_STATUS_FAILED;
 	} else {
-		g_attrib_send(conn->device->attrib, 0, pdu, length,
-				confirmation_cb, UINT_TO_PTR(conn->id), NULL);
 		status = HAL_STATUS_SUCCESS;
 	}
 
@@ -6143,13 +6136,13 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
 
 	rsp = g_attrib_get_buffer(dev->attrib, &length);
 
+		return ATT_ECODE_UNLIKELY;
+
 	/* Respond with our MTU */
 	len = enc_mtu_resp(mtu, rsp, length);
-	if (!len)
+	if (!g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL))
 		return ATT_ECODE_UNLIKELY;
 
-	g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL);
-
 	return 0;
 }
 
@@ -6703,9 +6696,7 @@ done:
 		resp_length = enc_error_resp(ipdu[0], 0x0000, status, opdu,
 									length);
 
-	if (resp_length)
-		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
-									NULL);
+	g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL, NULL);
 }
 
 static void connect_confirm(GIOChannel *io, void *user_data)
-- 
1.9.1

Re: [PATCH 05/10] android/gatt: Use g_attrib_send consistently

[Szymon Janc]

Hi Jakub,

On Wednesday 11 of February 2015 15:34:03 Jakub Tyszkowski wrote:
> 'g_attrib_send' do checks the 'length' parameter so there is no need
> for us to do this (and in most cases we don't). We can test the
> returned value in case we want to report error.
> ---
>  android/gatt.c | 31 +++++++++++--------------------
>  1 file changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 7f95226..81b0283 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1068,9 +1068,7 @@ static void notify_att_range_change(struct gatt_device *dev,
>  		break;
>  	}
>  
> -	if (length)
> -		g_attrib_send(dev->attrib, 0, pdu, length,
> -						confirmation_cb, NULL, NULL);
> +	g_attrib_send(dev->attrib, 0, pdu, length, confirmation_cb, NULL, NULL);
>  }
>  
>  static struct app_connection *create_connection(struct gatt_device *device,
> @@ -1480,9 +1478,7 @@ static void ind_handler(const uint8_t *cmd, uint16_t cmd_len,
>  	 */
>  
>  	resp_length = enc_confirmation(opdu, length);
> -	if (resp_length)
> -		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
> -									NULL);
> +	g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL, NULL);
>  }
>  
>  static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> @@ -4157,12 +4153,10 @@ static uint8_t test_read_write(bdaddr_t *bdaddr, bt_uuid_t *uuid, uint16_t op,
>  		return HAL_STATUS_UNSUPPORTED;
>  	}
>  
> -	if (!length)
> +	if (!g_attrib_send(dev->attrib, 0, pdu, length, test_command_result,
> +								NULL, NULL))
>  		return HAL_STATUS_FAILED;
>  
> -	g_attrib_send(dev->attrib, 0, pdu, length, test_command_result, NULL,
> -									NULL);
> -
>  	return HAL_STATUS_SUCCESS;
>  }
>  
> @@ -5531,12 +5525,11 @@ static void handle_server_send_indication(const void *buf, uint16_t len)
>  						cmd->len, pdu, mtu);
>  	}
>  
> -	if (length == 0) {
> -		error("gatt: Failed to encode indication");
> +	if (!g_attrib_send(conn->device->attrib, 0, pdu, length,
> +				confirmation_cb, UINT_TO_PTR(conn->id), NULL)) {
> +		error("gatt: Failed to send indication");
>  		status = HAL_STATUS_FAILED;
>  	} else {
> -		g_attrib_send(conn->device->attrib, 0, pdu, length,
> -				confirmation_cb, UINT_TO_PTR(conn->id), NULL);
>  		status = HAL_STATUS_SUCCESS;
>  	}
>  
> @@ -6143,13 +6136,13 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
>  
>  	rsp = g_attrib_get_buffer(dev->attrib, &length);
>  
> +		return ATT_ECODE_UNLIKELY;
> +

This looks strange, rebase artifact perhaps?

>  	/* Respond with our MTU */
>  	len = enc_mtu_resp(mtu, rsp, length);
> -	if (!len)
> +	if (!g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL))
>  		return ATT_ECODE_UNLIKELY;
>  
> -	g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL);
> -
>  	return 0;
>  }
>  
> @@ -6703,9 +6696,7 @@ done:
>  		resp_length = enc_error_resp(ipdu[0], 0x0000, status, opdu,
>  									length);
>  
> -	if (resp_length)
> -		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
> -									NULL);
> +	g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL, NULL);
>  }
>  
>  static void connect_confirm(GIOChannel *io, void *user_data)
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH 05/10] android/gatt: Use g_attrib_send consistently

[Tyszkowski Jakub]

Hi Szymon,

On 13.02.2015 12:31, Szymon Janc wrote:
> Hi Jakub,
>
> On Wednesday 11 of February 2015 15:34:03 Jakub Tyszkowski wrote:
>> 'g_attrib_send' do checks the 'length' parameter so there is no need
>> for us to do this (and in most cases we don't). We can test the
>> returned value in case we want to report error.
>> ---
>>   android/gatt.c | 31 +++++++++++--------------------
>>   1 file changed, 11 insertions(+), 20 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index 7f95226..81b0283 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -1068,9 +1068,7 @@ static void notify_att_range_change(struct gatt_device *dev,
>>   		break;
>>   	}
>>
>> -	if (length)
>> -		g_attrib_send(dev->attrib, 0, pdu, length,
>> -						confirmation_cb, NULL, NULL);
>> +	g_attrib_send(dev->attrib, 0, pdu, length, confirmation_cb, NULL, NULL);
>>   }
>>
>>   static struct app_connection *create_connection(struct gatt_device *device,
>> @@ -1480,9 +1478,7 @@ static void ind_handler(const uint8_t *cmd, uint16_t cmd_len,
>>   	 */
>>
>>   	resp_length = enc_confirmation(opdu, length);
>> -	if (resp_length)
>> -		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
>> -									NULL);
>> +	g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL, NULL);
>>   }
>>
>>   static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>> @@ -4157,12 +4153,10 @@ static uint8_t test_read_write(bdaddr_t *bdaddr, bt_uuid_t *uuid, uint16_t op,
>>   		return HAL_STATUS_UNSUPPORTED;
>>   	}
>>
>> -	if (!length)
>> +	if (!g_attrib_send(dev->attrib, 0, pdu, length, test_command_result,
>> +								NULL, NULL))
>>   		return HAL_STATUS_FAILED;
>>
>> -	g_attrib_send(dev->attrib, 0, pdu, length, test_command_result, NULL,
>> -									NULL);
>> -
>>   	return HAL_STATUS_SUCCESS;
>>   }
>>
>> @@ -5531,12 +5525,11 @@ static void handle_server_send_indication(const void *buf, uint16_t len)
>>   						cmd->len, pdu, mtu);
>>   	}
>>
>> -	if (length == 0) {
>> -		error("gatt: Failed to encode indication");
>> +	if (!g_attrib_send(conn->device->attrib, 0, pdu, length,
>> +				confirmation_cb, UINT_TO_PTR(conn->id), NULL)) {
>> +		error("gatt: Failed to send indication");
>>   		status = HAL_STATUS_FAILED;
>>   	} else {
>> -		g_attrib_send(conn->device->attrib, 0, pdu, length,
>> -				confirmation_cb, UINT_TO_PTR(conn->id), NULL);
>>   		status = HAL_STATUS_SUCCESS;
>>   	}
>>
>> @@ -6143,13 +6136,13 @@ static uint8_t mtu_att_handle(const uint8_t *cmd, uint16_t cmd_len,
>>
>>   	rsp = g_attrib_get_buffer(dev->attrib, &length);
>>
>> +		return ATT_ECODE_UNLIKELY;
>> +
>
> This looks strange, rebase artifact perhaps?

Yeap, I'll resend this patch.

>
>>   	/* Respond with our MTU */
>>   	len = enc_mtu_resp(mtu, rsp, length);
>> -	if (!len)
>> +	if (!g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL))
>>   		return ATT_ECODE_UNLIKELY;
>>
>> -	g_attrib_send(dev->attrib, 0, rsp, len, NULL, NULL, NULL);
>> -
>>   	return 0;
>>   }
>>
>> @@ -6703,9 +6696,7 @@ done:
>>   		resp_length = enc_error_resp(ipdu[0], 0x0000, status, opdu,
>>   									length);
>>
>> -	if (resp_length)
>> -		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
>> -									NULL);
>> +	g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL, NULL);
>>   }
>>
>>   static void connect_confirm(GIOChannel *io, void *user_data)
>>
>

BR,
Jakub

[PATCH 06/10] android/gatt: Improve send_dev_complete_response

[Jakub Tyszkowski]

For simple reads or writes there is no need to take the request out
of the queue as it is destroyed at the end anyway. We can peek and
check for errors even before switch statement.
---
 android/gatt.c | 47 +++++++++++------------------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 81b0283..19232f7 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4455,6 +4455,17 @@ static void send_dev_complete_response(struct gatt_device *device,
 		return;
 	}
 
+	val = queue_peek_head(device->pending_requests);
+	if (!val) {
+		error = ATT_ECODE_ATTR_NOT_FOUND;
+		goto done;
+	}
+
+	if (val->error) {
+		error = val->error;
+		goto done;
+	}
+
 	switch (opcode) {
 	case ATT_OP_READ_BY_TYPE_REQ: {
 		struct att_data_list *adl;
@@ -4514,25 +4525,11 @@ static void send_dev_complete_response(struct gatt_device *device,
 		break;
 	}
 	case ATT_OP_READ_BLOB_REQ:
-		val = queue_pop_head(device->pending_requests);
-		if (val->error) {
-			error = val->error;
-			goto done;
-		}
-
 		len = enc_read_blob_resp(val->value, val->length, val->offset,
 								rsp, mtu);
-		destroy_pending_request(val);
 		break;
 	case ATT_OP_READ_REQ:
-		val = queue_pop_head(device->pending_requests);
-		if (val->error) {
-			error = val->error;
-			goto done;
-		}
-
 		len = enc_read_resp(val->value, val->length, rsp, mtu);
-		destroy_pending_request(val);
 		break;
 	case ATT_OP_READ_BY_GROUP_REQ: {
 		struct att_data_list *adl;
@@ -4638,39 +4635,17 @@ static void send_dev_complete_response(struct gatt_device *device,
 		break;
 	}
 	case ATT_OP_EXEC_WRITE_REQ:
-		val = queue_pop_head(device->pending_requests);
-		if (val->error) {
-			error = val->error;
-			goto done;
-		}
-
 		len = enc_exec_write_resp(rsp);
-		destroy_pending_request(val);
 		break;
 	case ATT_OP_WRITE_REQ:
-		val = queue_pop_head(device->pending_requests);
-		if (val->error) {
-			error = val->error;
-			goto done;
-		}
-
 		len = enc_write_resp(rsp);
-		destroy_pending_request(val);
 		break;
 	case ATT_OP_PREP_WRITE_REQ: {
 		uint16_t handle;
 
-		val = queue_pop_head(device->pending_requests);
-		if (val->error) {
-			error = val->error;
-			goto done;
-		}
-
 		handle = gatt_db_attribute_get_handle(val->attrib);
-
 		len = enc_prep_write_resp(handle, val->offset, val->value,
 							val->length, rsp, mtu);
-		destroy_pending_request(val);
 		break;
 	}
 	default:
-- 
1.9.1

[PATCH 07/10] android/gatt: Improve send_dev_complete_response even further

[Jakub Tyszkowski]

We can use 'queue_remove_all' for response filtering instead of manually
repacking them to yet another queue.
---
 android/gatt.c | 104 ++++++++++++++++++++++++---------------------------------
 1 file changed, 43 insertions(+), 61 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 19232f7..49bcda2 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4437,6 +4437,22 @@ static bool match_pending_dev_request(const void *data, const void *user_data)
 	return !pending_request->completed;
 }
 
+struct invalid_response_match {
+	int length;
+	bool *remove;
+};
+
+static bool remove_invalid_requests(const void *data, const void *match_data)
+{
+	const struct pending_request *val = data;
+	const struct invalid_response_match *match = match_data;
+
+	if (val->length != match->length || val->error)
+		*match->remove = true;
+
+	return *match->remove;
+}
+
 static void send_dev_complete_response(struct gatt_device *device,
 								uint8_t opcode)
 {
@@ -4470,40 +4486,20 @@ static void send_dev_complete_response(struct gatt_device *device,
 	case ATT_OP_READ_BY_TYPE_REQ: {
 		struct att_data_list *adl;
 		int iterator = 0;
-		int length;
-		struct queue *temp;
-
-		temp = queue_new();
-		if (!temp)
-			goto done;
-
-		val = queue_pop_head(device->pending_requests);
-		if (!val) {
-			queue_destroy(temp, NULL);
-			error = ATT_ECODE_ATTR_NOT_FOUND;
-			goto done;
-		}
-
-		if (val->error) {
-			queue_destroy(temp, NULL);
-			error = val->error;
-			destroy_pending_request(val);
-			goto done;
-		}
-
-		length = val->length;
-
-		while (val && val->length == length && val->error == 0) {
-			queue_push_tail(temp, val);
-			val = queue_pop_head(device->pending_requests);
-		}
-
-		adl = att_data_list_alloc(queue_length(temp),
-						sizeof(uint16_t) + length);
+		bool remove = false;
+		struct invalid_response_match match;
+		struct queue *requests = device->pending_requests;
+
+		/* Remove all after error or value length change. */
+		match.remove = &remove;
+		match.length = val->length;
+		queue_remove_all(requests, remove_invalid_requests, &match,
+						destroy_pending_request);
 
-		destroy_pending_request(val);
+		adl = att_data_list_alloc(queue_length(requests),
+						sizeof(uint16_t) + val->length);
 
-		val = queue_pop_head(temp);
+		val = queue_pop_head(requests);
 		while (val) {
 			uint8_t *value = adl->data[iterator++];
 			uint16_t handle;
@@ -4514,14 +4510,12 @@ static void send_dev_complete_response(struct gatt_device *device,
 			memcpy(&value[2], val->value, val->length);
 
 			destroy_pending_request(val);
-			val = queue_pop_head(temp);
+			val = queue_pop_head(requests);
 		}
 
 		len = enc_read_by_type_resp(adl, rsp, mtu);
 
 		att_data_list_free(adl);
-		queue_destroy(temp, destroy_pending_request);
-
 		break;
 	}
 	case ATT_OP_READ_BLOB_REQ:
@@ -4534,31 +4528,21 @@ static void send_dev_complete_response(struct gatt_device *device,
 	case ATT_OP_READ_BY_GROUP_REQ: {
 		struct att_data_list *adl;
 		int iterator = 0;
-		int length;
-		struct queue *temp;
-
-		temp = queue_new();
-		if (!temp)
-			goto done;
-
-		val = queue_pop_head(device->pending_requests);
-		if (!val) {
-			queue_destroy(temp, NULL);
-			error = ATT_ECODE_ATTR_NOT_FOUND;
-			goto done;
-		}
-
-		length = val->length;
-
-		while (val && val->length == length) {
-			queue_push_tail(temp, val);
-			val = queue_pop_head(device->pending_requests);
-		}
+		bool remove = false;
+		struct invalid_response_match match;
+		struct queue *requests = device->pending_requests;
+
+		/* Remove all after error or value length change. */
+		match.remove = &remove;
+		match.length = val->length;
+		queue_remove_all(requests, remove_invalid_requests, &match,
+						destroy_pending_request);
 
-		adl = att_data_list_alloc(queue_length(temp),
-						2 * sizeof(uint16_t) + length);
+		adl = att_data_list_alloc(queue_length(requests),
+							2 * sizeof(uint16_t) +
+							val->length);
 
-		val = queue_pop_head(temp);
+		val = queue_pop_head(requests);
 		while (val) {
 			uint8_t *value = adl->data[iterator++];
 			uint16_t start_handle, end_handle;
@@ -4572,14 +4556,12 @@ static void send_dev_complete_response(struct gatt_device *device,
 			memcpy(&value[4], val->value, val->length);
 
 			destroy_pending_request(val);
-			val = queue_pop_head(temp);
+			val = queue_pop_head(requests);
 		}
 
 		len = enc_read_by_grp_resp(adl, rsp, mtu);
 
 		att_data_list_free(adl);
-		queue_destroy(temp, destroy_pending_request);
-
 		break;
 	}
 	case ATT_OP_FIND_BY_TYPE_REQ: {
-- 
1.9.1

Re: [PATCH 07/10] android/gatt: Improve send_dev_complete_response even further

[Szymon Janc]

Hi Jakub,

On Wednesday 11 of February 2015 15:34:05 Jakub Tyszkowski wrote:
> We can use 'queue_remove_all' for response filtering instead of manually
> repacking them to yet another queue.
> ---
>  android/gatt.c | 104 ++++++++++++++++++++++++---------------------------------
>  1 file changed, 43 insertions(+), 61 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 19232f7..49bcda2 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4437,6 +4437,22 @@ static bool match_pending_dev_request(const void *data, const void *user_data)
>  	return !pending_request->completed;
>  }
>  
> +struct invalid_response_match {
> +	int length;
> +	bool *remove;

I'd prefer to avoid such hacks.

> +};
> +
> +static bool remove_invalid_requests(const void *data, const void *match_data)
> +{
> +	const struct pending_request *val = data;
> +	const struct invalid_response_match *match = match_data;
> +
> +	if (val->length != match->length || val->error)
> +		*match->remove = true;
> +
> +	return *match->remove;
> +}
> +
>  static void send_dev_complete_response(struct gatt_device *device,
>  								uint8_t opcode)
>  {
> @@ -4470,40 +4486,20 @@ static void send_dev_complete_response(struct gatt_device *device,
>  	case ATT_OP_READ_BY_TYPE_REQ: {
>  		struct att_data_list *adl;
>  		int iterator = 0;
> -		int length;
> -		struct queue *temp;
> -
> -		temp = queue_new();
> -		if (!temp)
> -			goto done;
> -
> -		val = queue_pop_head(device->pending_requests);
> -		if (!val) {
> -			queue_destroy(temp, NULL);
> -			error = ATT_ECODE_ATTR_NOT_FOUND;
> -			goto done;
> -		}
> -
> -		if (val->error) {
> -			queue_destroy(temp, NULL);
> -			error = val->error;
> -			destroy_pending_request(val);
> -			goto done;
> -		}
> -
> -		length = val->length;
> -
> -		while (val && val->length == length && val->error == 0) {
> -			queue_push_tail(temp, val);
> -			val = queue_pop_head(device->pending_requests);
> -		}
> -
> -		adl = att_data_list_alloc(queue_length(temp),
> -						sizeof(uint16_t) + length);
> +		bool remove = false;
> +		struct invalid_response_match match;
> +		struct queue *requests = device->pending_requests;
> +
> +		/* Remove all after error or value length change. */
> +		match.remove = &remove;
> +		match.length = val->length;
> +		queue_remove_all(requests, remove_invalid_requests, &match,
> +						destroy_pending_request);

Please check if queue_remove_if() was more suitable here.

>  
> -		destroy_pending_request(val);
> +		adl = att_data_list_alloc(queue_length(requests),
> +						sizeof(uint16_t) + val->length);
>  
> -		val = queue_pop_head(temp);
> +		val = queue_pop_head(requests);
>  		while (val) {
>  			uint8_t *value = adl->data[iterator++];
>  			uint16_t handle;
> @@ -4514,14 +4510,12 @@ static void send_dev_complete_response(struct gatt_device *device,
>  			memcpy(&value[2], val->value, val->length);
>  
>  			destroy_pending_request(val);
> -			val = queue_pop_head(temp);
> +			val = queue_pop_head(requests);
>  		}
>  
>  		len = enc_read_by_type_resp(adl, rsp, mtu);
>  
>  		att_data_list_free(adl);
> -		queue_destroy(temp, destroy_pending_request);
> -
>  		break;
>  	}
>  	case ATT_OP_READ_BLOB_REQ:
> @@ -4534,31 +4528,21 @@ static void send_dev_complete_response(struct gatt_device *device,
>  	case ATT_OP_READ_BY_GROUP_REQ: {
>  		struct att_data_list *adl;
>  		int iterator = 0;
> -		int length;
> -		struct queue *temp;
> -
> -		temp = queue_new();
> -		if (!temp)
> -			goto done;
> -
> -		val = queue_pop_head(device->pending_requests);
> -		if (!val) {
> -			queue_destroy(temp, NULL);
> -			error = ATT_ECODE_ATTR_NOT_FOUND;
> -			goto done;
> -		}
> -
> -		length = val->length;
> -
> -		while (val && val->length == length) {
> -			queue_push_tail(temp, val);
> -			val = queue_pop_head(device->pending_requests);
> -		}
> +		bool remove = false;
> +		struct invalid_response_match match;
> +		struct queue *requests = device->pending_requests;
> +
> +		/* Remove all after error or value length change. */
> +		match.remove = &remove;
> +		match.length = val->length;
> +		queue_remove_all(requests, remove_invalid_requests, &match,
> +						destroy_pending_request);
>  
> -		adl = att_data_list_alloc(queue_length(temp),
> -						2 * sizeof(uint16_t) + length);
> +		adl = att_data_list_alloc(queue_length(requests),
> +							2 * sizeof(uint16_t) +
> +							val->length);
>  
> -		val = queue_pop_head(temp);
> +		val = queue_pop_head(requests);
>  		while (val) {
>  			uint8_t *value = adl->data[iterator++];
>  			uint16_t start_handle, end_handle;
> @@ -4572,14 +4556,12 @@ static void send_dev_complete_response(struct gatt_device *device,
>  			memcpy(&value[4], val->value, val->length);
>  
>  			destroy_pending_request(val);
> -			val = queue_pop_head(temp);
> +			val = queue_pop_head(requests);
>  		}
>  
>  		len = enc_read_by_grp_resp(adl, rsp, mtu);
>  
>  		att_data_list_free(adl);
> -		queue_destroy(temp, destroy_pending_request);
> -
>  		break;
>  	}
>  	case ATT_OP_FIND_BY_TYPE_REQ: {
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH 07/10] android/gatt: Improve send_dev_complete_response even further

[Tyszkowski Jakub]

Hi Szymon,

On 13.02.2015 12:35, Szymon Janc wrote:
> Hi Jakub,
>
> On Wednesday 11 of February 2015 15:34:05 Jakub Tyszkowski wrote:
>> We can use 'queue_remove_all' for response filtering instead of manually
>> repacking them to yet another queue.
>> ---
>>   android/gatt.c | 104 ++++++++++++++++++++++++---------------------------------
>>   1 file changed, 43 insertions(+), 61 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index 19232f7..49bcda2 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -4437,6 +4437,22 @@ static bool match_pending_dev_request(const void *data, const void *user_data)
>>   	return !pending_request->completed;
>>   }
>>
>> +struct invalid_response_match {
>> +	int length;
>> +	bool *remove;
>
> I'd prefer to avoid such hacks.

Then I can't provide You a better solution unless we extend queue's API 
somehow.

>
>> +};
>> +
>> +static bool remove_invalid_requests(const void *data, const void *match_data)
>> +{
>> +	const struct pending_request *val = data;
>> +	const struct invalid_response_match *match = match_data;
>> +
>> +	if (val->length != match->length || val->error)
>> +		*match->remove = true;
>> +
>> +	return *match->remove;
>> +}
>> +
>>   static void send_dev_complete_response(struct gatt_device *device,
>>   								uint8_t opcode)
>>   {
>> @@ -4470,40 +4486,20 @@ static void send_dev_complete_response(struct gatt_device *device,
>>   	case ATT_OP_READ_BY_TYPE_REQ: {
>>   		struct att_data_list *adl;
>>   		int iterator = 0;
>> -		int length;
>> -		struct queue *temp;
>> -
>> -		temp = queue_new();
>> -		if (!temp)
>> -			goto done;
>> -
>> -		val = queue_pop_head(device->pending_requests);
>> -		if (!val) {
>> -			queue_destroy(temp, NULL);
>> -			error = ATT_ECODE_ATTR_NOT_FOUND;
>> -			goto done;
>> -		}
>> -
>> -		if (val->error) {
>> -			queue_destroy(temp, NULL);
>> -			error = val->error;
>> -			destroy_pending_request(val);
>> -			goto done;
>> -		}
>> -
>> -		length = val->length;
>> -
>> -		while (val && val->length == length && val->error == 0) {
>> -			queue_push_tail(temp, val);
>> -			val = queue_pop_head(device->pending_requests);
>> -		}
>> -
>> -		adl = att_data_list_alloc(queue_length(temp),
>> -						sizeof(uint16_t) + length);
>> +		bool remove = false;
>> +		struct invalid_response_match match;
>> +		struct queue *requests = device->pending_requests;
>> +
>> +		/* Remove all after error or value length change. */
>> +		match.remove = &remove;
>> +		match.length = val->length;
>> +		queue_remove_all(requests, remove_invalid_requests, &match,
>> +						destroy_pending_request);
>
> Please check if queue_remove_if() was more suitable here.

Nope. That's where this 'hack' was needed to remove also all elements 
after first match. queue_remove_if() works only for the first matching 
element.

>
>>
>> -		destroy_pending_request(val);
>> +		adl = att_data_list_alloc(queue_length(requests),
>> +						sizeof(uint16_t) + val->length);
>>
>> -		val = queue_pop_head(temp);
>> +		val = queue_pop_head(requests);
>>   		while (val) {
>>   			uint8_t *value = adl->data[iterator++];
>>   			uint16_t handle;
>> @@ -4514,14 +4510,12 @@ static void send_dev_complete_response(struct gatt_device *device,
>>   			memcpy(&value[2], val->value, val->length);
>>
>>   			destroy_pending_request(val);
>> -			val = queue_pop_head(temp);
>> +			val = queue_pop_head(requests);
>>   		}
>>
>>   		len = enc_read_by_type_resp(adl, rsp, mtu);
>>
>>   		att_data_list_free(adl);
>> -		queue_destroy(temp, destroy_pending_request);
>> -
>>   		break;
>>   	}
>>   	case ATT_OP_READ_BLOB_REQ:
>> @@ -4534,31 +4528,21 @@ static void send_dev_complete_response(struct gatt_device *device,
>>   	case ATT_OP_READ_BY_GROUP_REQ: {
>>   		struct att_data_list *adl;
>>   		int iterator = 0;
>> -		int length;
>> -		struct queue *temp;
>> -
>> -		temp = queue_new();
>> -		if (!temp)
>> -			goto done;
>> -
>> -		val = queue_pop_head(device->pending_requests);
>> -		if (!val) {
>> -			queue_destroy(temp, NULL);
>> -			error = ATT_ECODE_ATTR_NOT_FOUND;
>> -			goto done;
>> -		}
>> -
>> -		length = val->length;
>> -
>> -		while (val && val->length == length) {
>> -			queue_push_tail(temp, val);
>> -			val = queue_pop_head(device->pending_requests);
>> -		}
>> +		bool remove = false;
>> +		struct invalid_response_match match;
>> +		struct queue *requests = device->pending_requests;
>> +
>> +		/* Remove all after error or value length change. */
>> +		match.remove = &remove;
>> +		match.length = val->length;
>> +		queue_remove_all(requests, remove_invalid_requests, &match,
>> +						destroy_pending_request);
>>
>> -		adl = att_data_list_alloc(queue_length(temp),
>> -						2 * sizeof(uint16_t) + length);
>> +		adl = att_data_list_alloc(queue_length(requests),
>> +							2 * sizeof(uint16_t) +
>> +							val->length);
>>
>> -		val = queue_pop_head(temp);
>> +		val = queue_pop_head(requests);
>>   		while (val) {
>>   			uint8_t *value = adl->data[iterator++];
>>   			uint16_t start_handle, end_handle;
>> @@ -4572,14 +4556,12 @@ static void send_dev_complete_response(struct gatt_device *device,
>>   			memcpy(&value[4], val->value, val->length);
>>
>>   			destroy_pending_request(val);
>> -			val = queue_pop_head(temp);
>> +			val = queue_pop_head(requests);
>>   		}
>>
>>   		len = enc_read_by_grp_resp(adl, rsp, mtu);
>>
>>   		att_data_list_free(adl);
>> -		queue_destroy(temp, destroy_pending_request);
>> -
>>   		break;
>>   	}
>>   	case ATT_OP_FIND_BY_TYPE_REQ: {
>>
>

BR,
Jakub

[PATCH 08/10] android/gatt: Remove not needed checks and jumps

[Jakub Tyszkowski]

In the end we check the status anyway so its safe to break out of switch
statement. There is also no point in calling 'goto label' when the 'label'
is right after switch statement closing bracket.
---
 android/gatt.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 49bcda2..c518d27 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6617,8 +6617,6 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 		break;
 	case ATT_OP_WRITE_REQ:
 		status = write_req_request(ipdu, len, dev);
-		if (!status)
-			return;
 		break;
 	case ATT_OP_WRITE_CMD:
 		write_cmd_request(ipdu, len, dev);
@@ -6630,22 +6628,18 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 		return;
 	case ATT_OP_PREP_WRITE_REQ:
 		status = write_prep_request(ipdu, len, dev);
-		if (!status)
-			return;
 		break;
 	case ATT_OP_FIND_BY_TYPE_REQ:
 		status = find_by_type_request(ipdu, len, dev);
 		break;
 	case ATT_OP_EXEC_WRITE_REQ:
 		status = write_execute_request(ipdu, len, dev);
-		if (!status)
-			return;
 		break;
 	case ATT_OP_READ_MULTI_REQ:
 	default:
 		DBG("Unsupported request 0x%02x", ipdu[0]);
 		status = ATT_ECODE_REQ_NOT_SUPP;
-		goto done;
+		break;
 	}
 
 done:
-- 
1.9.1

[PATCH 09/10] android/gatt: Remove redundant matching function

[Jakub Tyszkowski]

If NULL is passed as matching function, queue uses its internal
'direct_match()' thus there is no need to duplicate this function.
---
 android/gatt.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index c518d27..cd0825a 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -347,11 +347,6 @@ static struct gatt_app *find_app_by_id(int32_t id)
 	return queue_find(gatt_apps, match_app_by_id, INT_TO_PTR(id));
 }
 
-static bool match_by_value(const void *data, const void *user_data)
-{
-	return data == user_data;
-}
-
 static bool match_device_by_bdaddr(const void *data, const void *user_data)
 {
 	const struct gatt_device *dev = data;
@@ -564,7 +559,7 @@ static void unregister_notification(void *data)
 	 * triggered afterwards, but once client unregisters, device stays if
 	 * used by others. Then just unregister single handle.
 	 */
-	if (!queue_find(gatt_devices, match_by_value, dev))
+	if (!queue_find(gatt_devices, NULL, dev))
 		return;
 
 	if (notification->notif_id && dev)
@@ -2072,7 +2067,7 @@ static void handle_client_unregister(const void *buf, uint16_t len)
 
 	DBG("");
 
-	listening_client = queue_find(listen_apps, match_by_value,
+	listening_client = queue_find(listen_apps, NULL,
 						INT_TO_PTR(cmd->client_if));
 
 	if (listening_client) {
@@ -2199,7 +2194,7 @@ static void handle_client_listen(const void *buf, uint16_t len)
 		goto reply;
 	}
 
-	listening_client = queue_find(listen_apps, match_by_value,
+	listening_client = queue_find(listen_apps, NULL,
 						INT_TO_PTR(cmd->client_if));
 	/* Start listening */
 	if (cmd->start) {
@@ -7331,7 +7326,7 @@ bool bt_gatt_add_autoconnect(unsigned int id, const bdaddr_t *addr)
 	if (queue_isempty(dev->autoconnect_apps))
 		device_ref(dev);
 
-	if (!queue_find(dev->autoconnect_apps, match_by_value, INT_TO_PTR(id)))
+	if (!queue_find(dev->autoconnect_apps, NULL, INT_TO_PTR(id)))
 		return queue_push_head(dev->autoconnect_apps, INT_TO_PTR(id));
 
 	return true;
-- 
1.9.1

[PATCH 10/10] android/gatt: Avoid copying pointers in matching functions

[Jakub Tyszkowski]

This removes casting void* to struct pointers when there is no need to
do so (pointer comparison).
---
 android/gatt.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index cd0825a..3076a02 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -350,9 +350,8 @@ static struct gatt_app *find_app_by_id(int32_t id)
 static bool match_device_by_bdaddr(const void *data, const void *user_data)
 {
 	const struct gatt_device *dev = data;
-	const bdaddr_t *addr = user_data;
 
-	return !bacmp(&dev->bdaddr, addr);
+	return !bacmp(&dev->bdaddr, user_data);
 }
 
 static bool match_device_by_state(const void *data, const void *user_data)
@@ -408,17 +407,15 @@ static struct app_connection *find_connection_by_id(int32_t conn_id)
 static bool match_connection_by_device(const void *data, const void *user_data)
 {
 	const struct app_connection *conn = data;
-	const struct gatt_device *dev = user_data;
 
-	return conn->device == dev;
+	return conn->device == user_data;
 }
 
 static bool match_connection_by_app(const void *data, const void *user_data)
 {
 	const struct app_connection *conn = data;
-	const struct gatt_app *app = user_data;
 
-	return conn->app == app;
+	return conn->app == user_data;
 }
 
 static struct gatt_device *find_device_by_addr(const bdaddr_t *addr)
@@ -469,9 +466,8 @@ static bool match_srvc_by_bt_uuid(const void *data, const void *user_data)
 static bool match_srvc_by_range(const void *data, const void *user_data)
 {
 	const struct service *srvc = data;
-	const struct att_range *range = user_data;
 
-	return !memcmp(&srvc->prim.range, range, sizeof(srvc->prim.range));
+	return !memcmp(&srvc->prim.range, user_data, sizeof(srvc->prim.range));
 }
 
 static bool match_char_by_higher_inst_id(const void *data,
-- 
1.9.1

Re: [PATCH 10/10] android/gatt: Avoid copying pointers in matching functions

[Szymon Janc]

Hi Jakub,

On Wednesday 11 of February 2015 15:34:08 Jakub Tyszkowski wrote:
> This removes casting void* to struct pointers when there is no need to
> do so (pointer comparison).
> ---
>  android/gatt.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index cd0825a..3076a02 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -350,9 +350,8 @@ static struct gatt_app *find_app_by_id(int32_t id)
>  static bool match_device_by_bdaddr(const void *data, const void *user_data)
>  {
>  	const struct gatt_device *dev = data;
> -	const bdaddr_t *addr = user_data;
>  
> -	return !bacmp(&dev->bdaddr, addr);
> +	return !bacmp(&dev->bdaddr, user_data);
>  }
>  
>  static bool match_device_by_state(const void *data, const void *user_data)
> @@ -408,17 +407,15 @@ static struct app_connection *find_connection_by_id(int32_t conn_id)
>  static bool match_connection_by_device(const void *data, const void *user_data)
>  {
>  	const struct app_connection *conn = data;
> -	const struct gatt_device *dev = user_data;
>  
> -	return conn->device == dev;
> +	return conn->device == user_data;
>  }
>  
>  static bool match_connection_by_app(const void *data, const void *user_data)
>  {
>  	const struct app_connection *conn = data;
> -	const struct gatt_app *app = user_data;
>  
> -	return conn->app == app;
> +	return conn->app == user_data;
>  }
>  
>  static struct gatt_device *find_device_by_addr(const bdaddr_t *addr)
> @@ -469,9 +466,8 @@ static bool match_srvc_by_bt_uuid(const void *data, const void *user_data)
>  static bool match_srvc_by_range(const void *data, const void *user_data)
>  {
>  	const struct service *srvc = data;
> -	const struct att_range *range = user_data;
>  
> -	return !memcmp(&srvc->prim.range, range, sizeof(srvc->prim.range));
> +	return !memcmp(&srvc->prim.range, user_data, sizeof(srvc->prim.range));
>  }
>  
>  static bool match_char_by_higher_inst_id(const void *data,

I prefer to have those as is so that one can easily see what the userdata
is expected to be. And compiler will most likely optimize those assignments
anyway.

-- 
Best regards, 
Szymon Janc

Re: [PATCH 10/10] android/gatt: Avoid copying pointers in matching functions

[Tyszkowski Jakub]

Hi Szymon,

On 13.02.2015 12:48, Szymon Janc wrote:
> Hi Jakub,
>
> On Wednesday 11 of February 2015 15:34:08 Jakub Tyszkowski wrote:
>> This removes casting void* to struct pointers when there is no need to
>> do so (pointer comparison).
>> ---
>>   android/gatt.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index cd0825a..3076a02 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -350,9 +350,8 @@ static struct gatt_app *find_app_by_id(int32_t id)
>>   static bool match_device_by_bdaddr(const void *data, const void *user_data)
>>   {
>>   	const struct gatt_device *dev = data;
>> -	const bdaddr_t *addr = user_data;
>>
>> -	return !bacmp(&dev->bdaddr, addr);
>> +	return !bacmp(&dev->bdaddr, user_data);
>>   }
>>
>>   static bool match_device_by_state(const void *data, const void *user_data)
>> @@ -408,17 +407,15 @@ static struct app_connection *find_connection_by_id(int32_t conn_id)
>>   static bool match_connection_by_device(const void *data, const void *user_data)
>>   {
>>   	const struct app_connection *conn = data;
>> -	const struct gatt_device *dev = user_data;
>>
>> -	return conn->device == dev;
>> +	return conn->device == user_data;
>>   }
>>
>>   static bool match_connection_by_app(const void *data, const void *user_data)
>>   {
>>   	const struct app_connection *conn = data;
>> -	const struct gatt_app *app = user_data;
>>
>> -	return conn->app == app;
>> +	return conn->app == user_data;
>>   }
>>
>>   static struct gatt_device *find_device_by_addr(const bdaddr_t *addr)
>> @@ -469,9 +466,8 @@ static bool match_srvc_by_bt_uuid(const void *data, const void *user_data)
>>   static bool match_srvc_by_range(const void *data, const void *user_data)
>>   {
>>   	const struct service *srvc = data;
>> -	const struct att_range *range = user_data;
>>
>> -	return !memcmp(&srvc->prim.range, range, sizeof(srvc->prim.range));
>> +	return !memcmp(&srvc->prim.range, user_data, sizeof(srvc->prim.range));
>>   }
>>
>>   static bool match_char_by_higher_inst_id(const void *data,
>
> I prefer to have those as is so that one can easily see what the userdata
> is expected to be. And compiler will most likely optimize those assignments
> anyway.
>

Ok.

BR,
Jakub

Re: [PATCH 01/10] android/gatt: Replace request state with boolean

[Szymon Janc]

Hi Jakub,

On Wednesday 11 of February 2015 15:33:59 Jakub Tyszkowski wrote:
> As there are two states we can instead use boolean value.
> ---
>  android/gatt.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 8e58e41..b0ce1a8 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -690,11 +690,6 @@ static void destroy_gatt_app(void *data)
>  	free(app);
>  }
>  
> -enum pend_req_state {
> -	REQUEST_PENDING,
> -	REQUEST_DONE,
> -};
> -
>  struct pending_request {
>  	struct gatt_db_attribute *attrib;
>  	int length;
> @@ -704,7 +699,7 @@ struct pending_request {
>  	uint8_t *filter_value;
>  	uint16_t filter_vlen;
>  
> -	enum pend_req_state state;
> +	bool completed;
>  	uint8_t error;
>  };
>  
> @@ -4475,7 +4470,7 @@ 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;
> +	return !pending_request->completed;
>  }
>  
>  static void send_dev_complete_response(struct gatt_device *device,
> @@ -4830,7 +4825,7 @@ static void attribute_read_cb(struct gatt_db_attribute *attrib, int err,
>  	resp_data->length = length;
>  	resp_data->error = error;
>  
> -	resp_data->state = REQUEST_DONE;
> +	resp_data->completed = true;
>  
>  	if (!length)
>  		return;
> @@ -4856,7 +4851,7 @@ static void read_requested_attributes(void *data, void *user_data)
>  	attrib = resp_data->attrib;
>  	if (!attrib) {
>  		resp_data->error = ATT_ECODE_ATTR_NOT_FOUND;
> -		resp_data->state = REQUEST_DONE;
> +		resp_data->completed = true;
>  		return;
>  	}
>  
> @@ -4874,7 +4869,7 @@ static void read_requested_attributes(void *data, void *user_data)
>  							permissions);
>  	if (error != 0) {
>  		resp_data->error = error;
> -		resp_data->state = REQUEST_DONE;
> +		resp_data->completed = true;
>  		return;
>  	}
>  
> @@ -5650,7 +5645,7 @@ static void handle_server_send_response(const void *buf, uint16_t len)
>  
>  		/* Cast status to uint8_t, due to (byte) cast in java layer. */
>  		req->error = err_to_att((uint8_t) cmd->status);
> -		req->state = REQUEST_DONE;
> +		req->completed = true;
>  
>  		/*
>  		 * FIXME: Handle situation when not all server applications
> @@ -6493,7 +6488,7 @@ static void attribute_write_cb(struct gatt_db_attribute *attrib, int err,
>  	data->attrib = attrib;
>  	data->error = error;
>  
> -	data->state = REQUEST_DONE;
> +	data->completed = true;
>  }
>  
>  static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,

Patches 1-4, 6, 8 and 9 are now applied. Thanks.

-- 
Best regards, 
Szymon Janc