[PATCH BlueZ 0/2] shared/gatt: Bug fixes.

[PATCH BlueZ 0/2] shared/gatt: Bug fixes.

[Arman Uguray]

This patch includes small bug fixes for shared/att and
shared/gatt-client.

Arman Uguray (2):
  shared/att: Directly close fd on ATT violations.
  shared/gatt-client: Call ready_handler only in init.

 src/shared/att.c         | 17 +++++++++++------
 src/shared/gatt-client.c |  3 ++-
 2 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ 1/2] shared/att: Directly close fd on ATT violations.

[Arman Uguray]

The existing code handles ATT protocol request timeouts and invalid
incoming request PDUs by cleaning up the io structure via io_destroy.
This doesn't always work, since this only terminates the connection if
io_set_close_on_destroy has been set. Calling io_destroy to force a
disconnect also has the added side-effect of not invoking the disconnect
callback registered with struct io.

This patch fixes these by calling close directly on the file descriptor
when required by the ATT protocol specification. This both cleans up the
connection regardless of whether or not close_on_unref has been set, and
it triggers the disconnect callback via an EPOLLHUP event.
---
 src/shared/att.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 2bc7682..7ea86e5 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -416,9 +416,6 @@ static bool timeout_cb(void *user_data)
 	if (!op)
 		return false;
 
-	io_destroy(att->io);
-	att->io = NULL;
-
 	util_debug(att->debug_callback, att->debug_data,
 				"Operation timed out: 0x%02x", op->opcode);
 
@@ -428,6 +425,13 @@ static bool timeout_cb(void *user_data)
 	op->timeout_id = 0;
 	destroy_att_send_op(op);
 
+	/*
+	 * Directly terminate the connection as required by the ATT protocol.
+	 * This should trigger an io disconnect event which will clean up the
+	 * io and notify the upper layer.
+	 */
+	close(att->fd);
+
 	return false;
 }
 
@@ -765,14 +769,15 @@ static bool can_read_data(struct io *io, void *user_data)
 	case ATT_OP_TYPE_REQ:
 		/*
 		 * If a request is currently pending, then the sequential
-		 * protocol was violated. Disconnect the bearer and notify the
-		 * upper-layer.
+		 * protocol was violated. Disconnect the bearer, which will
+		 * promptly notify the upper layer via disconnect handlers.
 		 */
 		if (att->in_req) {
 			util_debug(att->debug_callback, att->debug_data,
 					"Received request while another is "
 					"pending: 0x%02x", opcode);
-			disconnect_cb(att->io, att);
+			close(att->fd);
+
 			return false;
 		}
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ 2/2] shared/gatt-client: Call ready_handler only in init.

[Arman Uguray]

Currently disconnect_cb calls ready_handler to notify the upper layer
if a disconnect occurred during initialization. This patch fixes it so
that the handler is only called if in_init is set.
---
 src/shared/gatt-client.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 4a645eb..033cba1 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1455,6 +1455,7 @@ static void bt_gatt_client_free(struct bt_gatt_client *client)
 static void att_disconnect_cb(void *user_data)
 {
 	struct bt_gatt_client *client = user_data;
+	bool in_init = client->in_init;
 
 	client->disc_id = 0;
 
@@ -1464,7 +1465,7 @@ static void att_disconnect_cb(void *user_data)
 	client->in_init = false;
 	client->ready = false;
 
-	if (client->ready_callback)
+	if (in_init && client->ready_callback)
 		client->ready_callback(false, 0, client->ready_data);
 }
 
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH BlueZ 1/2] shared/att: Directly close fd on ATT violations.

[Johan Hedberg]

Hi Arman,

On Fri, Nov 21, 2014, Arman Uguray wrote:
> +	/*
> +	 * Directly terminate the connection as required by the ATT protocol.
> +	 * This should trigger an io disconnect event which will clean up the
> +	 * io and notify the upper layer.
> +	 */
> +	close(att->fd);
> +
>  	return false;
>  }

Shouldn't you also set att->fd to -1 since its old value is now invalid?
(similar to setting a pointer to NULL after freeing it)

However, wouldn't the more correct thing to do be to use shutdown()
instead of close()? That would guarantee that even in the case of
duplicated fd's the connection would get terminated - close() can be
considered to behave similar to an unref(), i.e. it invalidates the fd
value but only does something to the connection (the same as shutdown)
if there are no other references left.

Johan