[PATCH 1/5] android/gatt: Fix write confirm callback being not set

[PATCH 1/5] android/gatt: Fix write confirm callback being not set

[Jakub Tyszkowski]

Currently this callback is required or previously registered write
callback function wont be called.
---
 android/gatt.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7d1115e..4ad0ce9 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6178,6 +6178,15 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
 	return 0;
 }
 
+static void write_confirm(struct gatt_db_attribute *attrib,
+						int err, void *user_data)
+{
+	if (!err)
+		return;
+
+	error("Error writting attribute %p", attrib);
+}
+
 static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 						struct gatt_device *dev)
 {
@@ -6206,7 +6215,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 		return;
 
 	gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
-								NULL, NULL);
+							write_confirm, NULL);
 }
 
 static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
@@ -6278,7 +6287,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 		/* Signature OK, proceed with write */
 		bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
 		gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
-						&dev->bdaddr, NULL, NULL);
+					&dev->bdaddr, write_confirm, NULL);
 	}
 }
 
@@ -6640,15 +6649,6 @@ static void device_name_read_cb(struct gatt_db_attribute *attrib,
 								strlen(name));
 }
 
-static void write_confirm(struct gatt_db_attribute *attrib,
-						int err, void *user_data)
-{
-	if (!err)
-		return;
-
-	error("Error writting attribute %p", attrib);
-}
-
 static void register_gap_service(void)
 {
 	uint16_t start, end;
-- 
1.9.1

[PATCH 2/5] android/gatt: Fix not confirming write commands in database

[Jakub Tyszkowski]

App will never confirm write command as commands dont need one but we
still need to confirm in databasse. We confirm immediately.
---
 android/gatt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 4ad0ce9..cb87810 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4904,6 +4904,8 @@ static void write_cb(struct gatt_db_attribute *attrib, unsigned int id,
 
 	if (opcode == ATT_OP_WRITE_REQ || opcode == ATT_OP_PREP_WRITE_REQ)
 		ev->need_rsp = 0x01;
+	else
+		gatt_db_attribute_write_result(attrib, id, 0);
 
 	ev->length = len;
 	memcpy(ev->value, value, len);
-- 
1.9.1

[PATCH 3/5] android/gatt: Fix write commands being not handled

[Jakub Tyszkowski]

Registering for GATTRIB_ALL_REQS now means only requests and not commands
or other type of att operations. Those needs their own handlers to be
registered.
---
 android/gatt.c | 475 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 245 insertions(+), 230 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index cb87810..766aae9 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -161,7 +161,9 @@ struct gatt_device {
 	bool partial_srvc_search;
 
 	guint watch_id;
-	guint server_id;
+	guint req_handler_id;
+	guint write_cmd_handler_id;
+	guint sign_write_cmd_handler_id;
 
 	int ref;
 	int conn_cnt;
@@ -630,8 +632,15 @@ static void connection_cleanup(struct gatt_device *device)
 	if (device->attrib) {
 		GAttrib *attrib = device->attrib;
 
-		if (device->server_id > 0)
-			g_attrib_unregister(device->attrib, device->server_id);
+		if (device->req_handler_id > 0)
+			g_attrib_unregister(device->attrib,
+							device->req_handler_id);
+		if (device->write_cmd_handler_id > 0)
+			g_attrib_unregister(device->attrib,
+						device->write_cmd_handler_id);
+		if (device->sign_write_cmd_handler_id > 0)
+			g_attrib_unregister(device->attrib,
+					device->sign_write_cmd_handler_id);
 
 		device->attrib = NULL;
 		g_attrib_cancel_all(attrib);
@@ -943,7 +952,8 @@ static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
 	return FALSE;
 }
 
-static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data);
+static void att_req_handler(const uint8_t *ipdu, uint16_t len,
+							gpointer user_data);
 
 static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
 							gpointer user_data)
@@ -1431,6 +1441,222 @@ static void create_app_connection(void *data, void *user_data)
 		create_connection(dev, app);
 }
 
+static void write_confirm(struct gatt_db_attribute *attrib,
+						int err, void *user_data)
+{
+	if (!err)
+		return;
+
+	error("Error writting attribute %p", attrib);
+}
+
+static uint8_t check_device_permissions(struct gatt_device *device,
+					uint8_t opcode, uint32_t permissions)
+{
+	GIOChannel *io;
+	int sec_level;
+
+	io = g_attrib_get_channel(device->attrib);
+
+	if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
+							BT_IO_OPT_INVALID))
+		return ATT_ECODE_UNLIKELY;
+
+	DBG("opcode %u permissions %u sec_level %u", opcode, permissions,
+								sec_level);
+
+	switch (opcode) {
+	case ATT_OP_SIGNED_WRITE_CMD:
+		if (!(permissions & GATT_PERM_WRITE_SIGNED))
+				return ATT_ECODE_WRITE_NOT_PERM;
+
+		if ((permissions & GATT_PERM_WRITE_SIGNED_MITM) &&
+						sec_level < BT_SECURITY_HIGH)
+			return ATT_ECODE_AUTHENTICATION;
+		break;
+	case ATT_OP_READ_BY_TYPE_REQ:
+	case ATT_OP_READ_REQ:
+	case ATT_OP_READ_BLOB_REQ:
+	case ATT_OP_READ_MULTI_REQ:
+	case ATT_OP_READ_BY_GROUP_REQ:
+	case ATT_OP_FIND_BY_TYPE_REQ:
+	case ATT_OP_FIND_INFO_REQ:
+		if (!(permissions & GATT_PERM_READ))
+			return ATT_ECODE_READ_NOT_PERM;
+
+		if ((permissions & GATT_PERM_READ_MITM) &&
+						sec_level < BT_SECURITY_HIGH)
+			return ATT_ECODE_AUTHENTICATION;
+
+		if ((permissions & GATT_PERM_READ_ENCRYPTED) &&
+						sec_level < BT_SECURITY_MEDIUM)
+			return ATT_ECODE_INSUFF_ENC;
+
+		if (permissions & GATT_PERM_READ_AUTHORIZATION)
+			return ATT_ECODE_AUTHORIZATION;
+		break;
+	case ATT_OP_WRITE_REQ:
+	case ATT_OP_WRITE_CMD:
+	case ATT_OP_PREP_WRITE_REQ:
+	case ATT_OP_EXEC_WRITE_REQ:
+		if (!(permissions & GATT_PERM_WRITE))
+			return ATT_ECODE_WRITE_NOT_PERM;
+
+		if ((permissions & GATT_PERM_WRITE_MITM) &&
+						sec_level < BT_SECURITY_HIGH)
+			return ATT_ECODE_AUTHENTICATION;
+
+		if ((permissions & GATT_PERM_WRITE_ENCRYPTED) &&
+						sec_level < BT_SECURITY_MEDIUM)
+			return ATT_ECODE_INSUFF_ENC;
+
+		if (permissions & GATT_PERM_WRITE_AUTHORIZATION)
+			return ATT_ECODE_AUTHORIZATION;
+		break;
+	default:
+		return ATT_ECODE_UNLIKELY;
+	}
+
+	return 0;
+}
+
+static int get_cid(struct gatt_device *dev)
+{
+	GIOChannel *io;
+	uint16_t cid;
+
+	io = g_attrib_get_channel(dev->attrib);
+
+	if (!bt_io_get(io, NULL, BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID)) {
+		error("gatt: Failed to get CID");
+		return -1;
+	}
+
+	return cid;
+}
+
+static int get_sec_level(struct gatt_device *dev)
+{
+	GIOChannel *io;
+	int sec_level;
+
+	io = g_attrib_get_channel(dev->attrib);
+
+	if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
+							BT_IO_OPT_INVALID)) {
+		error("gatt: Failed to get sec_level");
+		return -1;
+	}
+
+	return sec_level;
+}
+
+static void att_write_cmd_handler(const uint8_t *cmd, uint16_t cmd_len,
+							gpointer user_data)
+{
+	struct gatt_device *dev = user_data;
+	uint8_t value[cmd_len];
+	struct gatt_db_attribute *attrib;
+	uint32_t permissions;
+	uint16_t handle;
+	uint16_t len;
+	size_t vlen;
+
+	len = dec_write_cmd(cmd, cmd_len, &handle, value, &vlen);
+	if (!len)
+		return;
+
+	if (handle == 0)
+		return;
+
+	attrib = gatt_db_get_attribute(gatt_db, handle);
+	if (!attrib)
+		return;
+
+	if (!gatt_db_attribute_get_permissions(attrib, &permissions))
+		return;
+
+	if (check_device_permissions(dev, cmd[0], permissions))
+		return;
+
+	gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
+							write_confirm, NULL);
+}
+
+static void att_sign_write_cmd_handler(const uint8_t *cmd, uint16_t cmd_len,
+							gpointer user_data)
+{
+	struct gatt_device *dev = user_data;
+	uint8_t value[cmd_len];
+	uint8_t s[ATT_SIGNATURE_LEN];
+	struct gatt_db_attribute *attrib;
+	uint32_t permissions;
+	uint16_t handle;
+	uint16_t len;
+	size_t vlen;
+	uint8_t csrk[16];
+	uint32_t sign_cnt;
+
+	if (get_cid(dev) != ATT_CID) {
+		error("gatt: Remote tries write signed on BR/EDR bearer");
+		connection_cleanup(dev);
+		return;
+	}
+
+	if (get_sec_level(dev) != BT_SECURITY_LOW) {
+		error("gatt: Remote tries write signed on encrypted link");
+		connection_cleanup(dev);
+		return;
+	}
+
+	if (!bt_get_csrk(&dev->bdaddr, REMOTE_CSRK, csrk, &sign_cnt)) {
+		error("gatt: No valid csrk from remote device");
+		return;
+	}
+
+	len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
+
+	if (handle == 0)
+		return;
+
+	attrib = gatt_db_get_attribute(gatt_db, handle);
+	if (!attrib)
+		return;
+
+	gatt_db_attribute_get_permissions(attrib, &permissions);
+
+	if (check_device_permissions(dev, cmd[0], permissions))
+		return;
+
+	if (len) {
+		uint8_t t[ATT_SIGNATURE_LEN];
+		uint32_t r_sign_cnt = get_le32(s);
+
+		if (r_sign_cnt < sign_cnt) {
+			error("gatt: Invalid sign counter (%d<%d)",
+							r_sign_cnt, sign_cnt);
+			return;
+		}
+
+		/* Generate signature and verify it */
+		if (!bt_crypto_sign_att(crypto, csrk, cmd,
+						cmd_len - ATT_SIGNATURE_LEN,
+						r_sign_cnt, t)) {
+			error("gatt: Error when generating att signature");
+			return;
+		}
+
+		if (memcmp(t, s, ATT_SIGNATURE_LEN)) {
+			error("gatt: signature does not match");
+			return;
+		}
+		/* Signature OK, proceed with write */
+		bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
+		gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
+					&dev->bdaddr, write_confirm, NULL);
+	}
+}
+
 static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 {
 	struct gatt_device *dev = user_data;
@@ -1475,10 +1701,20 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	dev->watch_id = g_io_add_watch(io, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
 							disconnected_cb, dev);
 
-	dev->server_id = g_attrib_register(attrib, GATTRIB_ALL_REQS,
+	dev->req_handler_id = g_attrib_register(attrib, GATTRIB_ALL_REQS,
+						GATTRIB_ALL_HANDLES,
+						att_req_handler, dev, NULL);
+	dev->write_cmd_handler_id = g_attrib_register(attrib, ATT_OP_WRITE_CMD,
+							GATTRIB_ALL_HANDLES,
+							att_write_cmd_handler,
+							dev, NULL);
+	dev->sign_write_cmd_handler_id = g_attrib_register(attrib,
+						ATT_OP_SIGNED_WRITE_CMD,
 						GATTRIB_ALL_HANDLES,
-						att_handler, dev, NULL);
-	if (dev->server_id == 0)
+						att_sign_write_cmd_handler,
+						dev, NULL);
+	if ((dev->req_handler_id && dev->write_cmd_handler_id &&
+					dev->sign_write_cmd_handler_id) == 0)
 		error("gatt: Could not attach to server");
 
 	device_set_state(dev, DEVICE_CONNECTED);
@@ -2989,37 +3225,6 @@ static void read_char_cb(guint8 status, const guint8 *pdu, guint16 len,
 	free(data);
 }
 
-static int get_cid(struct gatt_device *dev)
-{
-	GIOChannel *io;
-	uint16_t cid;
-
-	io = g_attrib_get_channel(dev->attrib);
-
-	if (!bt_io_get(io, NULL, BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID)) {
-		error("gatt: Failed to get CID");
-		return -1;
-	}
-
-	return cid;
-}
-
-static int get_sec_level(struct gatt_device *dev)
-{
-	GIOChannel *io;
-	int sec_level;
-
-	io = g_attrib_get_channel(dev->attrib);
-
-	if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
-							BT_IO_OPT_INVALID)) {
-		error("gatt: Failed to get sec_level");
-		return -1;
-	}
-
-	return sec_level;
-}
-
 static bool set_security(struct gatt_device *device, int req_sec_level)
 {
 	int sec_level;
@@ -4612,76 +4817,6 @@ struct request_processing_data {
 	struct gatt_device *device;
 };
 
-static uint8_t check_device_permissions(struct gatt_device *device,
-					uint8_t opcode, uint32_t permissions)
-{
-	GIOChannel *io;
-	int sec_level;
-
-	io = g_attrib_get_channel(device->attrib);
-
-	if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
-							BT_IO_OPT_INVALID))
-		return ATT_ECODE_UNLIKELY;
-
-	DBG("opcode %u permissions %u sec_level %u", opcode, permissions,
-								sec_level);
-
-	switch (opcode) {
-	case ATT_OP_SIGNED_WRITE_CMD:
-		if (!(permissions & GATT_PERM_WRITE_SIGNED))
-				return ATT_ECODE_WRITE_NOT_PERM;
-
-		if ((permissions & GATT_PERM_WRITE_SIGNED_MITM) &&
-						sec_level < BT_SECURITY_HIGH)
-			return ATT_ECODE_AUTHENTICATION;
-		break;
-	case ATT_OP_READ_BY_TYPE_REQ:
-	case ATT_OP_READ_REQ:
-	case ATT_OP_READ_BLOB_REQ:
-	case ATT_OP_READ_MULTI_REQ:
-	case ATT_OP_READ_BY_GROUP_REQ:
-	case ATT_OP_FIND_BY_TYPE_REQ:
-	case ATT_OP_FIND_INFO_REQ:
-		if (!(permissions & GATT_PERM_READ))
-			return ATT_ECODE_READ_NOT_PERM;
-
-		if ((permissions & GATT_PERM_READ_MITM) &&
-						sec_level < BT_SECURITY_HIGH)
-			return ATT_ECODE_AUTHENTICATION;
-
-		if ((permissions & GATT_PERM_READ_ENCRYPTED) &&
-						sec_level < BT_SECURITY_MEDIUM)
-			return ATT_ECODE_INSUFF_ENC;
-
-		if (permissions & GATT_PERM_READ_AUTHORIZATION)
-			return ATT_ECODE_AUTHORIZATION;
-		break;
-	case ATT_OP_WRITE_REQ:
-	case ATT_OP_WRITE_CMD:
-	case ATT_OP_PREP_WRITE_REQ:
-	case ATT_OP_EXEC_WRITE_REQ:
-		if (!(permissions & GATT_PERM_WRITE))
-			return ATT_ECODE_WRITE_NOT_PERM;
-
-		if ((permissions & GATT_PERM_WRITE_MITM) &&
-						sec_level < BT_SECURITY_HIGH)
-			return ATT_ECODE_AUTHENTICATION;
-
-		if ((permissions & GATT_PERM_WRITE_ENCRYPTED) &&
-						sec_level < BT_SECURITY_MEDIUM)
-			return ATT_ECODE_INSUFF_ENC;
-
-		if (permissions & GATT_PERM_WRITE_AUTHORIZATION)
-			return ATT_ECODE_AUTHORIZATION;
-		break;
-	default:
-		return ATT_ECODE_UNLIKELY;
-	}
-
-	return 0;
-}
-
 static uint8_t err_to_att(int err)
 {
 	if (!err || (err > 0 && err < UINT8_MAX))
@@ -6180,119 +6315,6 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
 	return 0;
 }
 
-static void write_confirm(struct gatt_db_attribute *attrib,
-						int err, void *user_data)
-{
-	if (!err)
-		return;
-
-	error("Error writting attribute %p", attrib);
-}
-
-static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
-						struct gatt_device *dev)
-{
-	uint8_t value[cmd_len];
-	struct gatt_db_attribute *attrib;
-	uint32_t permissions;
-	uint16_t handle;
-	uint16_t len;
-	size_t vlen;
-
-	len = dec_write_cmd(cmd, cmd_len, &handle, value, &vlen);
-	if (!len)
-		return;
-
-	if (handle == 0)
-		return;
-
-	attrib = gatt_db_get_attribute(gatt_db, handle);
-	if (!attrib)
-		return;
-
-	if (!gatt_db_attribute_get_permissions(attrib, &permissions))
-		return;
-
-	if (check_device_permissions(dev, cmd[0], permissions))
-		return;
-
-	gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
-							write_confirm, NULL);
-}
-
-static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
-						struct gatt_device *dev)
-{
-	uint8_t value[cmd_len];
-	uint8_t s[ATT_SIGNATURE_LEN];
-	struct gatt_db_attribute *attrib;
-	uint32_t permissions;
-	uint16_t handle;
-	uint16_t len;
-	size_t vlen;
-	uint8_t csrk[16];
-	uint32_t sign_cnt;
-
-	if (get_cid(dev) != ATT_CID) {
-		error("gatt: Remote tries write signed on BR/EDR bearer");
-		connection_cleanup(dev);
-		return;
-	}
-
-	if (get_sec_level(dev) != BT_SECURITY_LOW) {
-		error("gatt: Remote tries write signed on encrypted link");
-		connection_cleanup(dev);
-		return;
-	}
-
-	if (!bt_get_csrk(&dev->bdaddr, REMOTE_CSRK, csrk, &sign_cnt)) {
-		error("gatt: No valid csrk from remote device");
-		return;
-	}
-
-	len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
-
-	if (handle == 0)
-		return;
-
-	attrib = gatt_db_get_attribute(gatt_db, handle);
-	if (!attrib)
-		return;
-
-	gatt_db_attribute_get_permissions(attrib, &permissions);
-
-	if (check_device_permissions(dev, cmd[0], permissions))
-		return;
-
-	if (len) {
-		uint8_t t[ATT_SIGNATURE_LEN];
-		uint32_t r_sign_cnt = get_le32(s);
-
-		if (r_sign_cnt < sign_cnt) {
-			error("gatt: Invalid sign counter (%d<%d)",
-							r_sign_cnt, sign_cnt);
-			return;
-		}
-
-		/* Generate signature and verify it */
-		if (!bt_crypto_sign_att(crypto, csrk, cmd,
-						cmd_len - ATT_SIGNATURE_LEN,
-						r_sign_cnt, t)) {
-			error("gatt: Error when generating att signature");
-			return;
-		}
-
-		if (memcmp(t, s, ATT_SIGNATURE_LEN)) {
-			error("gatt: signature does not match");
-			return;
-		}
-		/* Signature OK, proceed with write */
-		bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
-		gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
-					&dev->bdaddr, write_confirm, NULL);
-	}
-}
-
 static void attribute_write_cb(struct gatt_db_attribute *attrib, int err,
 								void *user_data)
 {
@@ -6481,7 +6503,8 @@ static uint8_t write_execute_request(const uint8_t *cmd, uint16_t cmd_len,
 	return 0;
 }
 
-static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
+static void att_req_handler(const uint8_t *ipdu, uint16_t len,
+							gpointer user_data)
 {
 	struct gatt_device *dev = user_data;
 	uint8_t status;
@@ -6518,14 +6541,6 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 		if (!status)
 			return;
 		break;
-	case ATT_OP_WRITE_CMD:
-		write_cmd_request(ipdu, len, dev);
-		/* No response on write cmd */
-		return;
-	case ATT_OP_SIGNED_WRITE_CMD:
-		write_signed_cmd_request(ipdu, len, dev);
-		/* No response on write signed cmd */
-		return;
 	case ATT_OP_PREP_WRITE_REQ:
 		status = write_prep_request(ipdu, len, dev);
 		if (!status)
-- 
1.9.1

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Luiz Augusto von Dentz]

Hi Jakub,

On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
<jakub.tyszkowski@tieto.com> wrote:
> Registering for GATTRIB_ALL_REQS now means only requests and not commands
> or other type of att operations. Those needs their own handlers to be
> registered.

Is this something we break while with the internal changes to use
bt_gatt* internally? Maybe we should have it fixed there instead since
it perhaps breaks the core daemon as well. Going forward this code
will transition to use bt_gatt* directly.

> ---
>  android/gatt.c | 475 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 245 insertions(+), 230 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index cb87810..766aae9 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -161,7 +161,9 @@ struct gatt_device {
>         bool partial_srvc_search;
>
>         guint watch_id;
> -       guint server_id;
> +       guint req_handler_id;
> +       guint write_cmd_handler_id;
> +       guint sign_write_cmd_handler_id;
>
>         int ref;
>         int conn_cnt;
> @@ -630,8 +632,15 @@ static void connection_cleanup(struct gatt_device *device)
>         if (device->attrib) {
>                 GAttrib *attrib = device->attrib;
>
> -               if (device->server_id > 0)
> -                       g_attrib_unregister(device->attrib, device->server_id);
> +               if (device->req_handler_id > 0)
> +                       g_attrib_unregister(device->attrib,
> +                                                       device->req_handler_id);
> +               if (device->write_cmd_handler_id > 0)
> +                       g_attrib_unregister(device->attrib,
> +                                               device->write_cmd_handler_id);
> +               if (device->sign_write_cmd_handler_id > 0)
> +                       g_attrib_unregister(device->attrib,
> +                                       device->sign_write_cmd_handler_id);
>
>                 device->attrib = NULL;
>                 g_attrib_cancel_all(attrib);
> @@ -943,7 +952,8 @@ static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
>         return FALSE;
>  }
>
> -static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data);
> +static void att_req_handler(const uint8_t *ipdu, uint16_t len,
> +                                                       gpointer user_data);
>
>  static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
>                                                         gpointer user_data)
> @@ -1431,6 +1441,222 @@ static void create_app_connection(void *data, void *user_data)
>                 create_connection(dev, app);
>  }
>
> +static void write_confirm(struct gatt_db_attribute *attrib,
> +                                               int err, void *user_data)
> +{
> +       if (!err)
> +               return;
> +
> +       error("Error writting attribute %p", attrib);
> +}
> +
> +static uint8_t check_device_permissions(struct gatt_device *device,
> +                                       uint8_t opcode, uint32_t permissions)
> +{
> +       GIOChannel *io;
> +       int sec_level;
> +
> +       io = g_attrib_get_channel(device->attrib);
> +
> +       if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
> +                                                       BT_IO_OPT_INVALID))
> +               return ATT_ECODE_UNLIKELY;
> +
> +       DBG("opcode %u permissions %u sec_level %u", opcode, permissions,
> +                                                               sec_level);
> +
> +       switch (opcode) {
> +       case ATT_OP_SIGNED_WRITE_CMD:
> +               if (!(permissions & GATT_PERM_WRITE_SIGNED))
> +                               return ATT_ECODE_WRITE_NOT_PERM;
> +
> +               if ((permissions & GATT_PERM_WRITE_SIGNED_MITM) &&
> +                                               sec_level < BT_SECURITY_HIGH)
> +                       return ATT_ECODE_AUTHENTICATION;
> +               break;
> +       case ATT_OP_READ_BY_TYPE_REQ:
> +       case ATT_OP_READ_REQ:
> +       case ATT_OP_READ_BLOB_REQ:
> +       case ATT_OP_READ_MULTI_REQ:
> +       case ATT_OP_READ_BY_GROUP_REQ:
> +       case ATT_OP_FIND_BY_TYPE_REQ:
> +       case ATT_OP_FIND_INFO_REQ:
> +               if (!(permissions & GATT_PERM_READ))
> +                       return ATT_ECODE_READ_NOT_PERM;
> +
> +               if ((permissions & GATT_PERM_READ_MITM) &&
> +                                               sec_level < BT_SECURITY_HIGH)
> +                       return ATT_ECODE_AUTHENTICATION;
> +
> +               if ((permissions & GATT_PERM_READ_ENCRYPTED) &&
> +                                               sec_level < BT_SECURITY_MEDIUM)
> +                       return ATT_ECODE_INSUFF_ENC;
> +
> +               if (permissions & GATT_PERM_READ_AUTHORIZATION)
> +                       return ATT_ECODE_AUTHORIZATION;
> +               break;
> +       case ATT_OP_WRITE_REQ:
> +       case ATT_OP_WRITE_CMD:
> +       case ATT_OP_PREP_WRITE_REQ:
> +       case ATT_OP_EXEC_WRITE_REQ:
> +               if (!(permissions & GATT_PERM_WRITE))
> +                       return ATT_ECODE_WRITE_NOT_PERM;
> +
> +               if ((permissions & GATT_PERM_WRITE_MITM) &&
> +                                               sec_level < BT_SECURITY_HIGH)
> +                       return ATT_ECODE_AUTHENTICATION;
> +
> +               if ((permissions & GATT_PERM_WRITE_ENCRYPTED) &&
> +                                               sec_level < BT_SECURITY_MEDIUM)
> +                       return ATT_ECODE_INSUFF_ENC;
> +
> +               if (permissions & GATT_PERM_WRITE_AUTHORIZATION)
> +                       return ATT_ECODE_AUTHORIZATION;
> +               break;
> +       default:
> +               return ATT_ECODE_UNLIKELY;
> +       }
> +
> +       return 0;
> +}
> +
> +static int get_cid(struct gatt_device *dev)
> +{
> +       GIOChannel *io;
> +       uint16_t cid;
> +
> +       io = g_attrib_get_channel(dev->attrib);
> +
> +       if (!bt_io_get(io, NULL, BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID)) {
> +               error("gatt: Failed to get CID");
> +               return -1;
> +       }
> +
> +       return cid;
> +}
> +
> +static int get_sec_level(struct gatt_device *dev)
> +{
> +       GIOChannel *io;
> +       int sec_level;
> +
> +       io = g_attrib_get_channel(dev->attrib);
> +
> +       if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
> +                                                       BT_IO_OPT_INVALID)) {
> +               error("gatt: Failed to get sec_level");
> +               return -1;
> +       }
> +
> +       return sec_level;
> +}
> +
> +static void att_write_cmd_handler(const uint8_t *cmd, uint16_t cmd_len,
> +                                                       gpointer user_data)
> +{
> +       struct gatt_device *dev = user_data;
> +       uint8_t value[cmd_len];
> +       struct gatt_db_attribute *attrib;
> +       uint32_t permissions;
> +       uint16_t handle;
> +       uint16_t len;
> +       size_t vlen;
> +
> +       len = dec_write_cmd(cmd, cmd_len, &handle, value, &vlen);
> +       if (!len)
> +               return;
> +
> +       if (handle == 0)
> +               return;
> +
> +       attrib = gatt_db_get_attribute(gatt_db, handle);
> +       if (!attrib)
> +               return;
> +
> +       if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> +               return;
> +
> +       if (check_device_permissions(dev, cmd[0], permissions))
> +               return;
> +
> +       gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
> +                                                       write_confirm, NULL);
> +}
> +
> +static void att_sign_write_cmd_handler(const uint8_t *cmd, uint16_t cmd_len,
> +                                                       gpointer user_data)
> +{
> +       struct gatt_device *dev = user_data;
> +       uint8_t value[cmd_len];
> +       uint8_t s[ATT_SIGNATURE_LEN];
> +       struct gatt_db_attribute *attrib;
> +       uint32_t permissions;
> +       uint16_t handle;
> +       uint16_t len;
> +       size_t vlen;
> +       uint8_t csrk[16];
> +       uint32_t sign_cnt;
> +
> +       if (get_cid(dev) != ATT_CID) {
> +               error("gatt: Remote tries write signed on BR/EDR bearer");
> +               connection_cleanup(dev);
> +               return;
> +       }
> +
> +       if (get_sec_level(dev) != BT_SECURITY_LOW) {
> +               error("gatt: Remote tries write signed on encrypted link");
> +               connection_cleanup(dev);
> +               return;
> +       }
> +
> +       if (!bt_get_csrk(&dev->bdaddr, REMOTE_CSRK, csrk, &sign_cnt)) {
> +               error("gatt: No valid csrk from remote device");
> +               return;
> +       }
> +
> +       len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
> +
> +       if (handle == 0)
> +               return;
> +
> +       attrib = gatt_db_get_attribute(gatt_db, handle);
> +       if (!attrib)
> +               return;
> +
> +       gatt_db_attribute_get_permissions(attrib, &permissions);
> +
> +       if (check_device_permissions(dev, cmd[0], permissions))
> +               return;
> +
> +       if (len) {
> +               uint8_t t[ATT_SIGNATURE_LEN];
> +               uint32_t r_sign_cnt = get_le32(s);
> +
> +               if (r_sign_cnt < sign_cnt) {
> +                       error("gatt: Invalid sign counter (%d<%d)",
> +                                                       r_sign_cnt, sign_cnt);
> +                       return;
> +               }
> +
> +               /* Generate signature and verify it */
> +               if (!bt_crypto_sign_att(crypto, csrk, cmd,
> +                                               cmd_len - ATT_SIGNATURE_LEN,
> +                                               r_sign_cnt, t)) {
> +                       error("gatt: Error when generating att signature");
> +                       return;
> +               }
> +
> +               if (memcmp(t, s, ATT_SIGNATURE_LEN)) {
> +                       error("gatt: signature does not match");
> +                       return;
> +               }
> +               /* Signature OK, proceed with write */
> +               bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
> +               gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
> +                                       &dev->bdaddr, write_confirm, NULL);
> +       }
> +}
> +
>  static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>  {
>         struct gatt_device *dev = user_data;
> @@ -1475,10 +1701,20 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>         dev->watch_id = g_io_add_watch(io, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
>                                                         disconnected_cb, dev);
>
> -       dev->server_id = g_attrib_register(attrib, GATTRIB_ALL_REQS,
> +       dev->req_handler_id = g_attrib_register(attrib, GATTRIB_ALL_REQS,
> +                                               GATTRIB_ALL_HANDLES,
> +                                               att_req_handler, dev, NULL);
> +       dev->write_cmd_handler_id = g_attrib_register(attrib, ATT_OP_WRITE_CMD,
> +                                                       GATTRIB_ALL_HANDLES,
> +                                                       att_write_cmd_handler,
> +                                                       dev, NULL);
> +       dev->sign_write_cmd_handler_id = g_attrib_register(attrib,
> +                                               ATT_OP_SIGNED_WRITE_CMD,
>                                                 GATTRIB_ALL_HANDLES,
> -                                               att_handler, dev, NULL);
> -       if (dev->server_id == 0)
> +                                               att_sign_write_cmd_handler,
> +                                               dev, NULL);
> +       if ((dev->req_handler_id && dev->write_cmd_handler_id &&
> +                                       dev->sign_write_cmd_handler_id) == 0)
>                 error("gatt: Could not attach to server");
>
>         device_set_state(dev, DEVICE_CONNECTED);
> @@ -2989,37 +3225,6 @@ static void read_char_cb(guint8 status, const guint8 *pdu, guint16 len,
>         free(data);
>  }
>
> -static int get_cid(struct gatt_device *dev)
> -{
> -       GIOChannel *io;
> -       uint16_t cid;
> -
> -       io = g_attrib_get_channel(dev->attrib);
> -
> -       if (!bt_io_get(io, NULL, BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID)) {
> -               error("gatt: Failed to get CID");
> -               return -1;
> -       }
> -
> -       return cid;
> -}
> -
> -static int get_sec_level(struct gatt_device *dev)
> -{
> -       GIOChannel *io;
> -       int sec_level;
> -
> -       io = g_attrib_get_channel(dev->attrib);
> -
> -       if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
> -                                                       BT_IO_OPT_INVALID)) {
> -               error("gatt: Failed to get sec_level");
> -               return -1;
> -       }
> -
> -       return sec_level;
> -}
> -
>  static bool set_security(struct gatt_device *device, int req_sec_level)
>  {
>         int sec_level;
> @@ -4612,76 +4817,6 @@ struct request_processing_data {
>         struct gatt_device *device;
>  };
>
> -static uint8_t check_device_permissions(struct gatt_device *device,
> -                                       uint8_t opcode, uint32_t permissions)
> -{
> -       GIOChannel *io;
> -       int sec_level;
> -
> -       io = g_attrib_get_channel(device->attrib);
> -
> -       if (!bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
> -                                                       BT_IO_OPT_INVALID))
> -               return ATT_ECODE_UNLIKELY;
> -
> -       DBG("opcode %u permissions %u sec_level %u", opcode, permissions,
> -                                                               sec_level);
> -
> -       switch (opcode) {
> -       case ATT_OP_SIGNED_WRITE_CMD:
> -               if (!(permissions & GATT_PERM_WRITE_SIGNED))
> -                               return ATT_ECODE_WRITE_NOT_PERM;
> -
> -               if ((permissions & GATT_PERM_WRITE_SIGNED_MITM) &&
> -                                               sec_level < BT_SECURITY_HIGH)
> -                       return ATT_ECODE_AUTHENTICATION;
> -               break;
> -       case ATT_OP_READ_BY_TYPE_REQ:
> -       case ATT_OP_READ_REQ:
> -       case ATT_OP_READ_BLOB_REQ:
> -       case ATT_OP_READ_MULTI_REQ:
> -       case ATT_OP_READ_BY_GROUP_REQ:
> -       case ATT_OP_FIND_BY_TYPE_REQ:
> -       case ATT_OP_FIND_INFO_REQ:
> -               if (!(permissions & GATT_PERM_READ))
> -                       return ATT_ECODE_READ_NOT_PERM;
> -
> -               if ((permissions & GATT_PERM_READ_MITM) &&
> -                                               sec_level < BT_SECURITY_HIGH)
> -                       return ATT_ECODE_AUTHENTICATION;
> -
> -               if ((permissions & GATT_PERM_READ_ENCRYPTED) &&
> -                                               sec_level < BT_SECURITY_MEDIUM)
> -                       return ATT_ECODE_INSUFF_ENC;
> -
> -               if (permissions & GATT_PERM_READ_AUTHORIZATION)
> -                       return ATT_ECODE_AUTHORIZATION;
> -               break;
> -       case ATT_OP_WRITE_REQ:
> -       case ATT_OP_WRITE_CMD:
> -       case ATT_OP_PREP_WRITE_REQ:
> -       case ATT_OP_EXEC_WRITE_REQ:
> -               if (!(permissions & GATT_PERM_WRITE))
> -                       return ATT_ECODE_WRITE_NOT_PERM;
> -
> -               if ((permissions & GATT_PERM_WRITE_MITM) &&
> -                                               sec_level < BT_SECURITY_HIGH)
> -                       return ATT_ECODE_AUTHENTICATION;
> -
> -               if ((permissions & GATT_PERM_WRITE_ENCRYPTED) &&
> -                                               sec_level < BT_SECURITY_MEDIUM)
> -                       return ATT_ECODE_INSUFF_ENC;
> -
> -               if (permissions & GATT_PERM_WRITE_AUTHORIZATION)
> -                       return ATT_ECODE_AUTHORIZATION;
> -               break;
> -       default:
> -               return ATT_ECODE_UNLIKELY;
> -       }
> -
> -       return 0;
> -}
> -
>  static uint8_t err_to_att(int err)
>  {
>         if (!err || (err > 0 && err < UINT8_MAX))
> @@ -6180,119 +6315,6 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
>         return 0;
>  }
>
> -static void write_confirm(struct gatt_db_attribute *attrib,
> -                                               int err, void *user_data)
> -{
> -       if (!err)
> -               return;
> -
> -       error("Error writting attribute %p", attrib);
> -}
> -
> -static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> -                                               struct gatt_device *dev)
> -{
> -       uint8_t value[cmd_len];
> -       struct gatt_db_attribute *attrib;
> -       uint32_t permissions;
> -       uint16_t handle;
> -       uint16_t len;
> -       size_t vlen;
> -
> -       len = dec_write_cmd(cmd, cmd_len, &handle, value, &vlen);
> -       if (!len)
> -               return;
> -
> -       if (handle == 0)
> -               return;
> -
> -       attrib = gatt_db_get_attribute(gatt_db, handle);
> -       if (!attrib)
> -               return;
> -
> -       if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> -               return;
> -
> -       if (check_device_permissions(dev, cmd[0], permissions))
> -               return;
> -
> -       gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
> -                                                       write_confirm, NULL);
> -}
> -
> -static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> -                                               struct gatt_device *dev)
> -{
> -       uint8_t value[cmd_len];
> -       uint8_t s[ATT_SIGNATURE_LEN];
> -       struct gatt_db_attribute *attrib;
> -       uint32_t permissions;
> -       uint16_t handle;
> -       uint16_t len;
> -       size_t vlen;
> -       uint8_t csrk[16];
> -       uint32_t sign_cnt;
> -
> -       if (get_cid(dev) != ATT_CID) {
> -               error("gatt: Remote tries write signed on BR/EDR bearer");
> -               connection_cleanup(dev);
> -               return;
> -       }
> -
> -       if (get_sec_level(dev) != BT_SECURITY_LOW) {
> -               error("gatt: Remote tries write signed on encrypted link");
> -               connection_cleanup(dev);
> -               return;
> -       }
> -
> -       if (!bt_get_csrk(&dev->bdaddr, REMOTE_CSRK, csrk, &sign_cnt)) {
> -               error("gatt: No valid csrk from remote device");
> -               return;
> -       }
> -
> -       len = dec_signed_write_cmd(cmd, cmd_len, &handle, value, &vlen, s);
> -
> -       if (handle == 0)
> -               return;
> -
> -       attrib = gatt_db_get_attribute(gatt_db, handle);
> -       if (!attrib)
> -               return;
> -
> -       gatt_db_attribute_get_permissions(attrib, &permissions);
> -
> -       if (check_device_permissions(dev, cmd[0], permissions))
> -               return;
> -
> -       if (len) {
> -               uint8_t t[ATT_SIGNATURE_LEN];
> -               uint32_t r_sign_cnt = get_le32(s);
> -
> -               if (r_sign_cnt < sign_cnt) {
> -                       error("gatt: Invalid sign counter (%d<%d)",
> -                                                       r_sign_cnt, sign_cnt);
> -                       return;
> -               }
> -
> -               /* Generate signature and verify it */
> -               if (!bt_crypto_sign_att(crypto, csrk, cmd,
> -                                               cmd_len - ATT_SIGNATURE_LEN,
> -                                               r_sign_cnt, t)) {
> -                       error("gatt: Error when generating att signature");
> -                       return;
> -               }
> -
> -               if (memcmp(t, s, ATT_SIGNATURE_LEN)) {
> -                       error("gatt: signature does not match");
> -                       return;
> -               }
> -               /* Signature OK, proceed with write */
> -               bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
> -               gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
> -                                       &dev->bdaddr, write_confirm, NULL);
> -       }
> -}
> -
>  static void attribute_write_cb(struct gatt_db_attribute *attrib, int err,
>                                                                 void *user_data)
>  {
> @@ -6481,7 +6503,8 @@ static uint8_t write_execute_request(const uint8_t *cmd, uint16_t cmd_len,
>         return 0;
>  }
>
> -static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
> +static void att_req_handler(const uint8_t *ipdu, uint16_t len,
> +                                                       gpointer user_data)
>  {
>         struct gatt_device *dev = user_data;
>         uint8_t status;
> @@ -6518,14 +6541,6 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
>                 if (!status)
>                         return;
>                 break;
> -       case ATT_OP_WRITE_CMD:
> -               write_cmd_request(ipdu, len, dev);
> -               /* No response on write cmd */
> -               return;
> -       case ATT_OP_SIGNED_WRITE_CMD:
> -               write_signed_cmd_request(ipdu, len, dev);
> -               /* No response on write signed cmd */
> -               return;
>         case ATT_OP_PREP_WRITE_REQ:
>                 status = write_prep_request(ipdu, len, dev);
>                 if (!status)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Luiz Augusto von Dentz]

Hi Jakub,

On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Jakub,
>
> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
> <jakub.tyszkowski@tieto.com> wrote:
>> Registering for GATTRIB_ALL_REQS now means only requests and not commands
>> or other type of att operations. Those needs their own handlers to be
>> registered.
>
> Is this something we break while with the internal changes to use
> bt_gatt* internally? Maybe we should have it fixed there instead since
> it perhaps breaks the core daemon as well. Going forward this code
> will transition to use bt_gatt* directly.

Looks like this code is to blame:

opcode_match:

if (opcode == BT_ATT_ALL_REQUESTS &&
   get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
return true;

Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
since, and looking at gatt-server.c you can clearly see how many
handlers we end up doing, perhaps we could something like this:

diff --git a/src/shared/att.c b/src/shared/att.c
index bc01827..3b52607 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -661,7 +661,7 @@ struct notify_data {
 static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
 {
        if (opcode == BT_ATT_ALL_REQUESTS &&
-                                   get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
+                                   get_op_type(test_opcode) != ATT_OP_TYPE_RSP)
                return true;

        return opcode == test_opcode;

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Tyszkowski Jakub]

Hi Luiz,

On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
> Hi Jakub,
>
> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Jakub,
>>
>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>> <jakub.tyszkowski@tieto.com> wrote:
>>> Registering for GATTRIB_ALL_REQS now means only requests and not commands
>>> or other type of att operations. Those needs their own handlers to be
>>> registered.
>>
>> Is this something we break while with the internal changes to use
>> bt_gatt* internally? Maybe we should have it fixed there instead since
>> it perhaps breaks the core daemon as well. Going forward this code
>> will transition to use bt_gatt* directly.
>
> Looks like this code is to blame:
>
> opcode_match:
>
> if (opcode == BT_ATT_ALL_REQUESTS &&
>     get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
> return true;
>
> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
> since, and looking at gatt-server.c you can clearly see how many
> handlers we end up doing, perhaps we could something like this:
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index bc01827..3b52607 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -661,7 +661,7 @@ struct notify_data {
>   static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>   {
>          if (opcode == BT_ATT_ALL_REQUESTS &&
> -                                   get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
> +                                   get_op_type(test_opcode) != ATT_OP_TYPE_RSP)
>                  return true;
>
>          return opcode == test_opcode;
>
Yeap, that's the line. I assumed that this change was intended and we 
should make the daemon compatible with this new semantic.
I've looked through the code where ALL_REQUESTS is used and I think 
there is no other place affected by this.

Regards

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Tyszkowski Jakub]

Hi,

On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
> Hi Luiz,
>
> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>> Hi Jakub,
>>
>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Jakub,
>>>
>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>> <jakub.tyszkowski@tieto.com> wrote:
>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>> commands
>>>> or other type of att operations. Those needs their own handlers to be
>>>> registered.
>>>
>>> Is this something we break while with the internal changes to use
>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>> it perhaps breaks the core daemon as well. Going forward this code
>>> will transition to use bt_gatt* directly.
>>
>> Looks like this code is to blame:
>>
>> opcode_match:
>>
>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>     get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>> return true;
>>
>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>> since, and looking at gatt-server.c you can clearly see how many
>> handlers we end up doing, perhaps we could something like this:
>>
>> diff --git a/src/shared/att.c b/src/shared/att.c
>> index bc01827..3b52607 100644
>> --- a/src/shared/att.c
>> +++ b/src/shared/att.c
>> @@ -661,7 +661,7 @@ struct notify_data {
>>   static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>   {
>>          if (opcode == BT_ATT_ALL_REQUESTS &&
>> -                                   get_op_type(test_opcode) ==
>> ATT_OP_TYPE_REQ)
>> +                                   get_op_type(test_opcode) !=
>> ATT_OP_TYPE_RSP)
>>                  return true;

If we do that, we are still left with other stuff like indications or 
even confirmations which would still be quite quirky for using 
*ALL_REQESTS in name.

>>
>>          return opcode == test_opcode;
>>
> Yeap, that's the line. I assumed that this change was intended and we
> should make the daemon compatible with this new semantic.
> I've looked through the code where ALL_REQUESTS is used and I think
> there is no other place affected by this.
>
> Regards

Regards

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Luiz Augusto von Dentz]

Hi Jakub,

On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
<jakub.tyszkowski@tieto.com> wrote:
> Hi,
>
>
> On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
>>
>> Hi Luiz,
>>
>> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>>>
>>> Hi Jakub,
>>>
>>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>>
>>>> Hi Jakub,
>>>>
>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>> <jakub.tyszkowski@tieto.com> wrote:
>>>>>
>>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>>> commands
>>>>> or other type of att operations. Those needs their own handlers to be
>>>>> registered.
>>>>
>>>>
>>>> Is this something we break while with the internal changes to use
>>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>>> it perhaps breaks the core daemon as well. Going forward this code
>>>> will transition to use bt_gatt* directly.
>>>
>>>
>>> Looks like this code is to blame:
>>>
>>> opcode_match:
>>>
>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>     get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>>> return true;
>>>
>>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>>> since, and looking at gatt-server.c you can clearly see how many
>>> handlers we end up doing, perhaps we could something like this:
>>>
>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>> index bc01827..3b52607 100644
>>> --- a/src/shared/att.c
>>> +++ b/src/shared/att.c
>>> @@ -661,7 +661,7 @@ struct notify_data {
>>>   static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>>   {
>>>          if (opcode == BT_ATT_ALL_REQUESTS &&
>>> -                                   get_op_type(test_opcode) ==
>>> ATT_OP_TYPE_REQ)
>>> +                                   get_op_type(test_opcode) !=
>>> ATT_OP_TYPE_RSP)
>>>                  return true;
>
>
> If we do that, we are still left with other stuff like indications or even
> confirmations which would still be quite quirky for using *ALL_REQESTS in
> name.

Perhaps we check ATT_OP_TYPE_CMD in addition to ATT_OP_TYPE_REQ.

>>>
>>>          return opcode == test_opcode;
>>>
>> Yeap, that's the line. I assumed that this change was intended and we
>> should make the daemon compatible with this new semantic.
>> I've looked through the code where ALL_REQUESTS is used and I think
>> there is no other place affected by this.
>>
>> Regards
>
>
> Regards
>



-- 
Luiz Augusto von Dentz

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Arman Uguray]

Hi Luiz & Jakub,

> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Jakub,
>
> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
> <jakub.tyszkowski@tieto.com> wrote:
>> Hi,
>>
>>
>> On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
>>>
>>> Hi Luiz,
>>>
>>> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>>>>
>>>> Hi Jakub,
>>>>
>>>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>>
>>>>> Hi Jakub,
>>>>>
>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>>> <jakub.tyszkowski@tieto.com> wrote:
>>>>>>
>>>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>>>> commands
>>>>>> or other type of att operations. Those needs their own handlers to be
>>>>>> registered.
>>>>>
>>>>>
>>>>> Is this something we break while with the internal changes to use
>>>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>>>> it perhaps breaks the core daemon as well. Going forward this code
>>>>> will transition to use bt_gatt* directly.
>>>>
>>>>
>>>> Looks like this code is to blame:
>>>>
>>>> opcode_match:
>>>>
>>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>     get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>>>> return true;
>>>>
>>>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>>>> since, and looking at gatt-server.c you can clearly see how many
>>>> handlers we end up doing, perhaps we could something like this:
>>>>
>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>> index bc01827..3b52607 100644
>>>> --- a/src/shared/att.c
>>>> +++ b/src/shared/att.c
>>>> @@ -661,7 +661,7 @@ struct notify_data {
>>>>   static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>>>   {
>>>>          if (opcode == BT_ATT_ALL_REQUESTS &&
>>>> -                                   get_op_type(test_opcode) ==
>>>> ATT_OP_TYPE_REQ)
>>>> +                                   get_op_type(test_opcode) !=
>>>> ATT_OP_TYPE_RSP)
>>>>                  return true;
>>
>>
>> If we do that, we are still left with other stuff like indications or even
>> confirmations which would still be quite quirky for using *ALL_REQESTS in
>> name.
>
> Perhaps we check ATT_OP_TYPE_CMD in addition to ATT_OP_TYPE_REQ.
>
>>>>
>>>>          return opcode == test_opcode;
>>>>
>>> Yeap, that's the line. I assumed that this change was intended and we
>>> should make the daemon compatible with this new semantic.
>>> I've looked through the code where ALL_REQUESTS is used and I think
>>> there is no other place affected by this.
>>>

The change in bt_att was really done to make GAttrib's ALL_REQUESTS
behavior work, otherwise we wouldn't have added it. In general, new
code should just register individual handler functions for the
specific opcodes it wants to handle. This leads to cleaner code, and
avoids the kind of huge switch/if statements you end up needing with
an ALL_* handler.

So for notifications/indications, you should really just register a
handler with the associated opcodes.

As a note: bt_att doesn't allow registering handlers for incoming
response PDUs. Instead, those are propagated to the upper layer via
the callback passed to bt_att_send when the request was originally
sent out. Basically, you don't get to know about a response unless you
sent out the request in the first place (and AFAIK this is the case
with GAttrib as well).

Cheers,
Arman

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Luiz Augusto von Dentz]

Hi Arman,

On Mon, Dec 8, 2014 at 9:50 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz & Jakub,
>
>> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> Hi Jakub,
>>
>> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
>> <jakub.tyszkowski@tieto.com> wrote:
>>> Hi,
>>>
>>>
>>> On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
>>>>
>>>> Hi Luiz,
>>>>
>>>> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>>>>>
>>>>> Hi Jakub,
>>>>>
>>>>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>>
>>>>>> Hi Jakub,
>>>>>>
>>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>>>> <jakub.tyszkowski@tieto.com> wrote:
>>>>>>>
>>>>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>>>>> commands
>>>>>>> or other type of att operations. Those needs their own handlers to be
>>>>>>> registered.
>>>>>>
>>>>>>
>>>>>> Is this something we break while with the internal changes to use
>>>>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>>>>> it perhaps breaks the core daemon as well. Going forward this code
>>>>>> will transition to use bt_gatt* directly.
>>>>>
>>>>>
>>>>> Looks like this code is to blame:
>>>>>
>>>>> opcode_match:
>>>>>
>>>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>     get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>>>>> return true;
>>>>>
>>>>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>>>>> since, and looking at gatt-server.c you can clearly see how many
>>>>> handlers we end up doing, perhaps we could something like this:
>>>>>
>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>> index bc01827..3b52607 100644
>>>>> --- a/src/shared/att.c
>>>>> +++ b/src/shared/att.c
>>>>> @@ -661,7 +661,7 @@ struct notify_data {
>>>>>   static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>>>>   {
>>>>>          if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>> -                                   get_op_type(test_opcode) ==
>>>>> ATT_OP_TYPE_REQ)
>>>>> +                                   get_op_type(test_opcode) !=
>>>>> ATT_OP_TYPE_RSP)
>>>>>                  return true;
>>>
>>>
>>> If we do that, we are still left with other stuff like indications or even
>>> confirmations which would still be quite quirky for using *ALL_REQESTS in
>>> name.
>>
>> Perhaps we check ATT_OP_TYPE_CMD in addition to ATT_OP_TYPE_REQ.
>>
>>>>>
>>>>>          return opcode == test_opcode;
>>>>>
>>>> Yeap, that's the line. I assumed that this change was intended and we
>>>> should make the daemon compatible with this new semantic.
>>>> I've looked through the code where ALL_REQUESTS is used and I think
>>>> there is no other place affected by this.
>>>>
>
> The change in bt_att was really done to make GAttrib's ALL_REQUESTS
> behavior work, otherwise we wouldn't have added it. In general, new
> code should just register individual handler functions for the
> specific opcodes it wants to handle. This leads to cleaner code, and
> avoids the kind of huge switch/if statements you end up needing with
> an ALL_* handler.

I would classify a command as a request as well which apparently is
not the case anymore, anyway the problem is that ALL_REQUESTS was
create to map to GAttrib but we end up with something different thus
breaking Android code. For new code you actually should not register
handler yourself, except as a client for notifications and
indications, since bt_gatt_server should take care of those but I
agree that is probably cleaner to have each handler registered
separately.

> So for notifications/indications, you should really just register a
> handler with the associated opcodes.

Yep, that should really be the only handlers outside bt_gatt_server scope.

> As a note: bt_att doesn't allow registering handlers for incoming
> response PDUs. Instead, those are propagated to the upper layer via
> the callback passed to bt_att_send when the request was originally
> sent out. Basically, you don't get to know about a response unless you
> sent out the request in the first place (and AFAIK this is the case
> with GAttrib as well).

I guess we were discussing how to get the same behavior as in GAttrib
ALL_REQUESTS, this would probably be removed once we transition to
bt_gatt_server.


-- 
Luiz Augusto von Dentz

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Arman Uguray]

Hi Luiz,

On Mon, Dec 8, 2014 at 2:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Mon, Dec 8, 2014 at 9:50 PM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Luiz & Jakub,
>>
>>> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>> Hi Jakub,
>>>
>>> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
>>> <jakub.tyszkowski@tieto.com> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
>>>>>
>>>>> Hi Luiz,
>>>>>
>>>>> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>>>>>>
>>>>>> Hi Jakub,
>>>>>>
>>>>>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi Jakub,
>>>>>>>
>>>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>>>>> <jakub.tyszkowski@tieto.com> wrote:
>>>>>>>>
>>>>>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>>>>>> commands
>>>>>>>> or other type of att operations. Those needs their own handlers to be
>>>>>>>> registered.
>>>>>>>
>>>>>>>
>>>>>>> Is this something we break while with the internal changes to use
>>>>>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>>>>>> it perhaps breaks the core daemon as well. Going forward this code
>>>>>>> will transition to use bt_gatt* directly.
>>>>>>
>>>>>>
>>>>>> Looks like this code is to blame:
>>>>>>
>>>>>> opcode_match:
>>>>>>
>>>>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>>     get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>>>>>> return true;
>>>>>>
>>>>>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>>>>>> since, and looking at gatt-server.c you can clearly see how many
>>>>>> handlers we end up doing, perhaps we could something like this:
>>>>>>
>>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>>> index bc01827..3b52607 100644
>>>>>> --- a/src/shared/att.c
>>>>>> +++ b/src/shared/att.c
>>>>>> @@ -661,7 +661,7 @@ struct notify_data {
>>>>>>   static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>>>>>   {
>>>>>>          if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>> -                                   get_op_type(test_opcode) ==
>>>>>> ATT_OP_TYPE_REQ)
>>>>>> +                                   get_op_type(test_opcode) !=
>>>>>> ATT_OP_TYPE_RSP)
>>>>>>                  return true;
>>>>
>>>>
>>>> If we do that, we are still left with other stuff like indications or even
>>>> confirmations which would still be quite quirky for using *ALL_REQESTS in
>>>> name.
>>>
>>> Perhaps we check ATT_OP_TYPE_CMD in addition to ATT_OP_TYPE_REQ.
>>>
>>>>>>
>>>>>>          return opcode == test_opcode;
>>>>>>
>>>>> Yeap, that's the line. I assumed that this change was intended and we
>>>>> should make the daemon compatible with this new semantic.
>>>>> I've looked through the code where ALL_REQUESTS is used and I think
>>>>> there is no other place affected by this.
>>>>>
>>
>> The change in bt_att was really done to make GAttrib's ALL_REQUESTS
>> behavior work, otherwise we wouldn't have added it. In general, new
>> code should just register individual handler functions for the
>> specific opcodes it wants to handle. This leads to cleaner code, and
>> avoids the kind of huge switch/if statements you end up needing with
>> an ALL_* handler.
>
> I would classify a command as a request as well which apparently is
> not the case anymore, anyway the problem is that ALL_REQUESTS was
> create to map to GAttrib but we end up with something different thus
> breaking Android code.

Oh OK, if the GAttrib code groups commands under the ALL_REQUESTS flag
then bt_att should respect that for compatibility, at least until we
remove the dependencies. So feel free to make the fix to bt_att so
that it works with commands as well.

> ... For new code you actually should not register
> handler yourself, except as a client for notifications and
> indications, since bt_gatt_server should take care of those but I
> agree that is probably cleaner to have each handler registered
> separately.
>

Sorry, that's exactly what I meant. In general if new code needs to
handle specific opcodes then they should register a special handler
for it, except they won't need to for most things since bt_gatt_server
already does it as you said :)

>> So for notifications/indications, you should really just register a
>> handler with the associated opcodes.
>
> Yep, that should really be the only handlers outside bt_gatt_server scope.
>

Right. Actually, in the future, for incoming notifications/indications
new code should use bt_gatt_client_register_notify. This automatically
writes to the remote CCC (does reference counting so that
notifications don't accidentally get disabled), sets its own handler
with bt_att for notifications and indications and automatically sends
out a confirmation for the latter. So I guess new code generally won't
need to ever directly call bt_att_register as long as they use
bt_gatt_client/bt_gatt_server.

>> As a note: bt_att doesn't allow registering handlers for incoming
>> response PDUs. Instead, those are propagated to the upper layer via
>> the callback passed to bt_att_send when the request was originally
>> sent out. Basically, you don't get to know about a response unless you
>> sent out the request in the first place (and AFAIK this is the case
>> with GAttrib as well).
>
> I guess we were discussing how to get the same behavior as in GAttrib
> ALL_REQUESTS, this would probably be removed once we transition to
> bt_gatt_server.
>

I think so. I think it's basically just for backwards compatibility to
keep GAttrib working and we can remove it in the future. We should
probably add a note in shared/att-types.h to discourage new code from
using it.

Thanks,
Arman

Re: [PATCH 3/5] android/gatt: Fix write commands being not handled

[Tyszkowski Jakub]

Hi Arman, Luiz,

On 12/08/2014 11:20 PM, Arman Uguray wrote:
> Hi Luiz,
>
> On Mon, Dec 8, 2014 at 2:08 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Arman,
>>
>> On Mon, Dec 8, 2014 at 9:50 PM, Arman Uguray <armansito@chromium.org> wrote:
>>> Hi Luiz & Jakub,
>>>
>>>> On Mon, Dec 8, 2014 at 3:02 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>>> Hi Jakub,
>>>>
>>>> On Mon, Dec 8, 2014 at 12:17 PM, Tyszkowski Jakub
>>>> <jakub.tyszkowski@tieto.com> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 12/08/2014 10:31 AM, Tyszkowski Jakub wrote:
>>>>>>
>>>>>> Hi Luiz,
>>>>>>
>>>>>> On 12/08/2014 10:22 AM, Luiz Augusto von Dentz wrote:
>>>>>>>
>>>>>>> Hi Jakub,
>>>>>>>
>>>>>>> On Mon, Dec 8, 2014 at 10:55 AM, Luiz Augusto von Dentz
>>>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Hi Jakub,
>>>>>>>>
>>>>>>>> On Mon, Dec 8, 2014 at 10:22 AM, Jakub Tyszkowski
>>>>>>>> <jakub.tyszkowski@tieto.com> wrote:
>>>>>>>>>
>>>>>>>>> Registering for GATTRIB_ALL_REQS now means only requests and not
>>>>>>>>> commands
>>>>>>>>> or other type of att operations. Those needs their own handlers to be
>>>>>>>>> registered.
>>>>>>>>
>>>>>>>>
>>>>>>>> Is this something we break while with the internal changes to use
>>>>>>>> bt_gatt* internally? Maybe we should have it fixed there instead since
>>>>>>>> it perhaps breaks the core daemon as well. Going forward this code
>>>>>>>> will transition to use bt_gatt* directly.
>>>>>>>
>>>>>>>
>>>>>>> Looks like this code is to blame:
>>>>>>>
>>>>>>> opcode_match:
>>>>>>>
>>>>>>> if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>>>      get_op_type(test_opcode) == ATT_OP_TYPE_REQ)
>>>>>>> return true;
>>>>>>>
>>>>>>> Id say this turn BT_ATT_ALL_REQUESTS not that convenient for server
>>>>>>> since, and looking at gatt-server.c you can clearly see how many
>>>>>>> handlers we end up doing, perhaps we could something like this:
>>>>>>>
>>>>>>> diff --git a/src/shared/att.c b/src/shared/att.c
>>>>>>> index bc01827..3b52607 100644
>>>>>>> --- a/src/shared/att.c
>>>>>>> +++ b/src/shared/att.c
>>>>>>> @@ -661,7 +661,7 @@ struct notify_data {
>>>>>>>    static bool opcode_match(uint8_t opcode, uint8_t test_opcode)
>>>>>>>    {
>>>>>>>           if (opcode == BT_ATT_ALL_REQUESTS &&
>>>>>>> -                                   get_op_type(test_opcode) ==
>>>>>>> ATT_OP_TYPE_REQ)
>>>>>>> +                                   get_op_type(test_opcode) !=
>>>>>>> ATT_OP_TYPE_RSP)
>>>>>>>                   return true;
>>>>>
>>>>>
>>>>> If we do that, we are still left with other stuff like indications or even
>>>>> confirmations which would still be quite quirky for using *ALL_REQESTS in
>>>>> name.
>>>>
>>>> Perhaps we check ATT_OP_TYPE_CMD in addition to ATT_OP_TYPE_REQ.
>>>>
>>>>>>>
>>>>>>>           return opcode == test_opcode;
>>>>>>>
>>>>>> Yeap, that's the line. I assumed that this change was intended and we
>>>>>> should make the daemon compatible with this new semantic.
>>>>>> I've looked through the code where ALL_REQUESTS is used and I think
>>>>>> there is no other place affected by this.
>>>>>>
>>>
>>> The change in bt_att was really done to make GAttrib's ALL_REQUESTS
>>> behavior work, otherwise we wouldn't have added it. In general, new
>>> code should just register individual handler functions for the
>>> specific opcodes it wants to handle. This leads to cleaner code, and
>>> avoids the kind of huge switch/if statements you end up needing with
>>> an ALL_* handler.
>>
>> I would classify a command as a request as well which apparently is
>> not the case anymore, anyway the problem is that ALL_REQUESTS was
>> create to map to GAttrib but we end up with something different thus
>> breaking Android code.
>
> Oh OK, if the GAttrib code groups commands under the ALL_REQUESTS flag
> then bt_att should respect that for compatibility, at least until we
> remove the dependencies. So feel free to make the fix to bt_att so
> that it works with commands as well.
>
>> ... For new code you actually should not register
>> handler yourself, except as a client for notifications and
>> indications, since bt_gatt_server should take care of those but I
>> agree that is probably cleaner to have each handler registered
>> separately.
>>
>
> Sorry, that's exactly what I meant. In general if new code needs to
> handle specific opcodes then they should register a special handler
> for it, except they won't need to for most things since bt_gatt_server
> already does it as you said :)
>
>>> So for notifications/indications, you should really just register a
>>> handler with the associated opcodes.
>>
>> Yep, that should really be the only handlers outside bt_gatt_server scope.
>>
>
> Right. Actually, in the future, for incoming notifications/indications
> new code should use bt_gatt_client_register_notify. This automatically
> writes to the remote CCC (does reference counting so that
> notifications don't accidentally get disabled), sets its own handler
> with bt_att for notifications and indications and automatically sends
> out a confirmation for the latter. So I guess new code generally won't
> need to ever directly call bt_att_register as long as they use
> bt_gatt_client/bt_gatt_server.

Sounds great. :)

>
>>> As a note: bt_att doesn't allow registering handlers for incoming
>>> response PDUs. Instead, those are propagated to the upper layer via
>>> the callback passed to bt_att_send when the request was originally
>>> sent out. Basically, you don't get to know about a response unless you
>>> sent out the request in the first place (and AFAIK this is the case
>>> with GAttrib as well).
>>
>> I guess we were discussing how to get the same behavior as in GAttrib
>> ALL_REQUESTS, this would probably be removed once we transition to
>> bt_gatt_server.
>>
>
> I think so. I think it's basically just for backwards compatibility to
> keep GAttrib working and we can remove it in the future. We should
> probably add a note in shared/att-types.h to discourage new code from
> using it.

Right. I'll send patch enabling commands under ALL_REQUEST type, as 
attrib-server.c looks to also be affected. I'll redo this set so that 
write commands stays in the same handler as requests and only 
indications are handled separately.

>
> Thanks,
> Arman
>

Regards,
Jakub

[PATCH 4/5] android/gatt: Extract indication support from att_req_handler

[Jakub Tyszkowski]

This fixes indication being not confirmed as this handler is called
only for requests.
---
 android/gatt.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 766aae9..8f0864f 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -164,6 +164,7 @@ struct gatt_device {
 	guint req_handler_id;
 	guint write_cmd_handler_id;
 	guint sign_write_cmd_handler_id;
+	guint ind_handler_id;
 
 	int ref;
 	int conn_cnt;
@@ -641,6 +642,9 @@ static void connection_cleanup(struct gatt_device *device)
 		if (device->sign_write_cmd_handler_id > 0)
 			g_attrib_unregister(device->attrib,
 					device->sign_write_cmd_handler_id);
+		if (device->ind_handler_id > 0)
+			g_attrib_unregister(device->attrib,
+							device->ind_handler_id);
 
 		device->attrib = NULL;
 		g_attrib_cancel_all(attrib);
@@ -1657,6 +1661,22 @@ static void att_sign_write_cmd_handler(const uint8_t *cmd, uint16_t cmd_len,
 	}
 }
 
+static void ind_handler(const uint8_t *cmd, uint16_t cmd_len,
+							gpointer user_data)
+{
+	struct gatt_device *dev = user_data;
+	uint16_t resp_length = 0;
+	size_t length;
+
+	uint8_t *opdu = g_attrib_get_buffer(dev->attrib, &length);
+
+	resp_length = enc_confirmation(opdu, length);
+
+	if (resp_length)
+		g_attrib_send(dev->attrib, 0, opdu, resp_length, NULL, NULL,
+									NULL);
+}
+
 static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 {
 	struct gatt_device *dev = user_data;
@@ -1713,8 +1733,11 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 						GATTRIB_ALL_HANDLES,
 						att_sign_write_cmd_handler,
 						dev, NULL);
+	dev->ind_handler_id = g_attrib_register(attrib, ATT_OP_HANDLE_IND,
+						GATTRIB_ALL_HANDLES,
+						ind_handler, dev, NULL);
 	if ((dev->req_handler_id && dev->write_cmd_handler_id &&
-					dev->sign_write_cmd_handler_id) == 0)
+			dev->ind_handler_id && dev->ind_handler_id) == 0)
 		error("gatt: Could not attach to server");
 
 	device_set_state(dev, DEVICE_CONNECTED);
@@ -6549,15 +6572,6 @@ static void att_req_handler(const uint8_t *ipdu, uint16_t len,
 	case ATT_OP_FIND_BY_TYPE_REQ:
 		status = find_by_type_request(ipdu, len, dev);
 		break;
-	case ATT_OP_HANDLE_IND:
-		/*
-		 * We have to send confirmation here. If some client is
-		 * registered for this indication, event will be send in
-		 * handle_notification
-		 */
-		resp_length = enc_confirmation(opdu, length);
-		status = 0;
-		break;
 	case ATT_OP_HANDLE_NOTIFY:
 		/* Client will handle this */
 		return;
-- 
1.9.1

[PATCH 5/5] android/gatt: Remove not handled att ops in request handler

[Jakub Tyszkowski]

For those att operations callback will never be called as its registered
for ATT_OP_TYPE_REQ.
---
 android/gatt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 8f0864f..7d1348c 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6572,15 +6572,11 @@ static void att_req_handler(const uint8_t *ipdu, uint16_t len,
 	case ATT_OP_FIND_BY_TYPE_REQ:
 		status = find_by_type_request(ipdu, len, dev);
 		break;
-	case ATT_OP_HANDLE_NOTIFY:
-		/* Client will handle this */
-		return;
 	case ATT_OP_EXEC_WRITE_REQ:
 		status = write_execute_request(ipdu, len, dev);
 		if (!status)
 			return;
 		break;
-	case ATT_OP_HANDLE_CNF:
 	case ATT_OP_READ_MULTI_REQ:
 	default:
 		DBG("Unsupported request 0x%02x", ipdu[0]);
-- 
1.9.1