[PATCH v3 0/4] bt_att initial implementation.

[PATCH v3 0/4] bt_att initial implementation.

[Arman Uguray]

v3:
  - Split the first patch "Introduce struct bt_att" into 4 different patches.
  - Removed bt_att_send_sequential. Instead, there is now one bt_att_send that
  correctly returns an error if an invalid combination of opcode and callbacks
  is given.
  - Added a table of ATT protocol opcodes. Send operations and read events now
  route the PDU to the correct handler after explicitly checking the opcode
  type.
  - Added three separate write queues for requests, indications, and writes that
  don't require a control flow.
  - Replaced pending_list with two separate pointers for the currently pending
  request and indication.

Arman Uguray (4):
  src/shared/att: Introduce struct bt_att.
  src/shared/att: Implement basic boilerplate.
  src/shared/att: Implement write handler and bt_att_send.
  src/shared/att: Handle incoming response PDUs

 Makefile.am      |   3 +-
 src/shared/att.c | 786 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/att.h | 261 ++++++++++++++++++
 3 files changed, 1049 insertions(+), 1 deletion(-)
 create mode 100644 src/shared/att.c
 create mode 100644 src/shared/att.h

-- 
1.8.3.2

[PATCH v3 1/4] src/shared/att: Introduce struct bt_att.

[Arman Uguray]

This patch introduces struct bt_att, which handles the transport and
encoding/decoding for the ATT protocol. The structure of the code
follows that of src/shared/mgmt and lib/mgmt.h, where individual
parameter structures are defined for all ATT protocol requests, responses,
commands, indications, and notifications. The serialization and
endianness conversion for all parameters are handled by bt_att.

struct bt_att is based around struct io and operates on a raw file
descriptor.
---
 Makefile.am      |   3 +-
 src/shared/att.c | 167 +++++++++++++++++++++++++++++++++++
 src/shared/att.h | 261 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+), 1 deletion(-)
 create mode 100644 src/shared/att.c
 create mode 100644 src/shared/att.h

diff --git a/Makefile.am b/Makefile.am
index dc88816..9612170 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -156,7 +156,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
 			src/shared/timeout.h src/shared/timeout-glib.c \
 			src/shared/queue.h src/shared/queue.c \
 			src/shared/util.h src/shared/util.c \
-			src/shared/mgmt.h src/shared/mgmt.c
+			src/shared/mgmt.h src/shared/mgmt.c \
+			src/shared/att.h src/shared/att.c
 src_bluetoothd_LDADD = lib/libbluetooth-internal.la gdbus/libgdbus-internal.la \
 			@GLIB_LIBS@ @DBUS_LIBS@ -ldl -lrt
 src_bluetoothd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic \
diff --git a/src/shared/att.c b/src/shared/att.c
new file mode 100644
index 0000000..6398ca7
--- /dev/null
+++ b/src/shared/att.c
@@ -0,0 +1,167 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+
+#include "src/shared/io.h"
+#include "src/shared/queue.h"
+#include "lib/uuid.h"
+#include "src/shared/att.h"
+
+#define ATT_DEFAULT_LE_MTU		23
+#define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
+
+struct att_send_op;
+
+struct bt_att {
+	int ref_count;
+	int fd;
+	bool close_on_unref;
+	struct io *io;
+	bool destroyed;
+
+	struct queue *req_queue;	/* Queued ATT protocol requests */
+	struct att_send_op *pending_req;
+	struct queue *ind_queue;	/* Queued ATT protocol indications */
+	struct att_send_op *pending_ind;
+	struct queue *write_queue;	/* Queue of PDUs ready to send */
+	bool writer_active;
+
+	/* TODO Add queues for registered request and notification handlers */
+
+	uint8_t *buf;
+	uint16_t mtu;
+
+	uint8_t auth_sig[12];
+
+	unsigned int next_send_id;	/* IDs for "send" ops */
+	unsigned int next_reg_id;	/* IDs for registered callbacks */
+
+	bt_att_debug_func_t debug_callback;
+	bt_att_destroy_func_t debug_destroy;
+	void *debug_data;
+};
+
+struct att_send_op {
+	unsigned int id;
+	uint16_t opcode;
+	void *pdu;
+	uint16_t len;
+	bt_att_request_func_t callback;
+	bt_att_destroy_func_t destroy;
+	void *user_data;
+};
+
+struct bt_att *bt_att_new(int fd)
+{
+	/* TODO */
+	return NULL;
+}
+
+struct bt_att *bt_att_ref(struct bt_att *att)
+{
+	/* TODO */
+	return NULL;
+}
+
+void bt_att_unref(struct bt_att *att)
+{
+	/* TODO */
+}
+
+bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close)
+{
+	/* TODO */
+	return false;
+}
+
+bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
+				void *user_data, bt_att_destroy_func_t destroy)
+{
+	/* TODO */
+	return false;
+}
+
+uint16_t bt_att_get_mtu(struct bt_att *att)
+{
+	/* TODO */
+	return 0;
+}
+
+bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu)
+{
+	/* TODO */
+	return false;
+}
+
+void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12])
+{
+	/* TODO */
+}
+
+unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
+				const void *param, uint16_t length,
+				bt_att_request_func_t callback, void *user_data,
+				bt_att_destroy_func_t destroy)
+{
+	/* TODO */
+	return 0;
+}
+
+bool bt_att_cancel_all(struct bt_att *att)
+{
+	/* TODO */
+	return false;
+}
+
+unsigned int bt_att_register_request(struct bt_att *att,
+				bt_att_request_func_t callback,
+				void *user_data, bt_att_destroy_func_t destroy)
+{
+	/* TODO */
+	return 0;
+}
+
+unsigned int bt_att_register_notify(struct bt_att *att,
+				bt_att_notify_func_t callback,
+				void *user_data, bt_att_destroy_func_t destroy)
+{
+	/* TODO */
+	return 0;
+}
+
+bool bt_att_unregister(struct bt_att *att, unsigned int id)
+{
+	/* TODO */
+	return false;
+}
+
+bool bt_att_unregister_all(struct bt_att *att)
+{
+	/* TODO */
+	return false;
+}
diff --git a/src/shared/att.h b/src/shared/att.h
new file mode 100644
index 0000000..b358d8f
--- /dev/null
+++ b/src/shared/att.h
@@ -0,0 +1,261 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <stdbool.h>
+#include <stdint.h>
+
+/* Error response */
+#define BT_ATT_OP_ERROR_RSP	      		0x01
+struct bt_att_error_rsp_param {
+	uint8_t request_opcode;
+	uint16_t handle;
+	uint8_t	error_code;
+};
+
+/* Exchange MTU */
+#define BT_ATT_OP_MTU_REQ			0x02
+struct bt_att_mtu_req_param {
+	uint16_t client_rx_mtu;
+};
+
+#define BT_ATT_OP_MTU_RSP			0x03
+struct bt_att_mtu_rsp_param {
+	uint16_t server_rx_mtu;
+};
+
+/* Find Information */
+#define BT_ATT_OP_FIND_INFO_REQ			0x04
+struct bt_att_find_info_req_param {
+	uint16_t start_handle;
+	uint16_t end_handle;
+};
+
+#define BT_ATT_OP_FIND_INFO_RSP			0x05
+struct bt_att_find_info_rsp_param {
+	uint8_t format;
+	const uint8_t *info_data;
+	uint16_t length;
+};
+
+/* Find By Type Value */
+#define BT_ATT_OP_FIND_BY_TYPE_VAL_REQ		0x06
+struct bt_att_find_by_type_value_req_param {
+	uint16_t start_handle;
+	uint16_t end_handle;
+	uint16_t type;  /* 2 octet UUID */
+	const uint8_t *value;
+	uint16_t length;  /* MAX length: (ATT_MTU - 7) */
+};
+
+#define BT_ATT_OP_FIND_BY_TYPE_VAL_RSP		0x07
+struct bt_att_find_by_type_value_rsp_param {
+	const uint8_t *handles_info_list;
+	uint16_t length;
+};
+
+/* Read By Type */
+#define BT_ATT_OP_READ_BY_TYPE_REQ		0x08
+struct bt_att_read_by_type_req_param {
+	uint16_t start_handle;
+	uint16_t end_handle;
+	bt_uuid_t type;  /* 2 or 16 octet UUID */
+};
+
+#define BT_ATT_OP_READ_BY_TYPE_RSP		0x09
+struct bt_att_read_by_type_rsp_param {
+	uint8_t length;
+	const uint8_t *attr_data_list;
+	uint16_t list_length;  /* Length of "attr_data_list" */
+};
+
+/* Read */
+#define BT_ATT_OP_READ_REQ			0x0a
+struct bt_att_read_req_param {
+	uint16_t handle;
+};
+
+#define BT_ATT_OP_READ_RSP			0x0b
+struct bt_att_read_rsp_param {
+	const uint8_t *value;
+	uint16_t length;
+};
+
+/* Read Blob */
+#define BT_ATT_OP_READ_BLOB_REQ			0x0c
+struct bt_att_read_blob_req_param {
+	uint16_t handle;
+	uint16_t offset;
+};
+
+#define BT_ATT_OP_READ_BLOB_RSP			0x0d
+struct bt_att_read_blob_rsp_param {
+	const uint8_t *part_value;
+	uint16_t length;
+};
+
+/* Read Multiple */
+#define BT_ATT_OP_READ_MULT_REQ			0x0e
+struct bt_att_read_multiple_req_param {
+	const uint16_t *handles;
+	uint16_t num_handles;
+};
+
+#define BT_ATT_OP_READ_MULT_RSP			0x0f
+struct bt_att_read_multiple_rsp_param {
+	const uint8_t *values;
+	uint16_t length;
+};
+
+/* Read By Group Type */
+#define BT_ATT_OP_READ_BY_GRP_TYPE_REQ		0x10
+struct bt_att_read_by_group_type_req_param {
+	uint16_t start_handle;
+	uint16_t end_handle;
+	bt_uuid_t type;
+};
+
+#define BT_ATT_OP_READ_BY_GRP_TYPE_RSP		0x11
+struct bt_att_read_by_group_type_rsp_param {
+	uint8_t length;
+	const uint8_t *attr_data_list;
+	uint16_t list_length;  /* Length of "attr_data_list" */
+};
+
+/* Write Request */
+#define BT_ATT_OP_WRITE_REQ			0x12
+/*
+ * bt_att_write_param is used for write request and signed and unsigned write
+ * command.
+ */
+struct bt_att_write_param {
+	uint16_t handle;
+	const uint8_t *value;
+	uint16_t length;
+};
+
+#define BT_ATT_OP_WRITE_RSP			0x13  /* No parameters */
+
+/* Write Command */
+#define BT_ATT_OP_WRITE_CMD			0x52
+
+/* Signed Write Command */
+#define BT_ATT_OP_SIGNED_WRITE_CMD		0xD2
+
+/* Prepare Write */
+#define BT_ATT_OP_PREP_WRITE_REQ		0x16
+struct bt_att_prepare_write_req_param {
+	uint16_t handle;
+	uint16_t offset;
+	const uint8_t *part_value;
+	uint16_t length;
+};
+
+#define BT_ATT_OP_PREP_WRITE_RSP		0x17
+struct bt_att_prepare_write_rsp_param {
+	uint16_t handle;
+	uint16_t offset;
+	const uint8_t *part_value;
+	uint16_t length;
+};
+
+/* Execute Write */
+#define BT_ATT_OP_EXEC_WRITE_REQ		0x18
+typedef enum {
+	BT_ATT_EXEC_WRITE_FLAG_CANCEL	= 0x00,
+	BT_ATT_EXEC_WRITE_FLAG_WRITE	= 0x01,
+} bt_att_exec_write_flag_t;
+
+struct bt_att_exec_write_req_param {
+	bt_att_exec_write_flag_t flags;
+};
+
+#define BT_ATT_OP_EXEC_WRITE_RSP		0x19
+
+/* Handle Value Notification/Indication */
+#define BT_ATT_OP_HANDLE_VAL_NOT		0x1B
+#define BT_ATT_OP_HANDLE_VAL_IND		0x1D
+struct bt_att_notify_param {
+	uint16_t handle;
+	const uint8_t *value;
+	uint16_t length;
+};
+
+/* Handle Value Confirmation */
+#define BT_ATT_OP_HANDLE_VAL_CONF		0x1E
+
+/* Error codes for Error response PDU */
+#define BT_ATT_ERROR_INVALID_HANDLE			0x01
+#define BT_ATT_ERROR_READ_NOT_PERMITTED			0x02
+#define BT_ATT_ERROR_WRITE_NOT_PERMITTED		0x03
+#define BT_ATT_ERROR_INVALID_PDU			0x04
+#define BT_ATT_ERROR_AUTHENTICATION			0x05
+#define BT_ATT_ERROR_REQUEST_NOT_SUPPORTED		0x06
+#define BT_ATT_ERROR_INVALID_OFFSET			0x07
+#define BT_ATT_ERROR_AUTHORIZATION			0x08
+#define BT_ATT_ERROR_PREPARE_QUEUE_FULL			0x09
+#define BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND		0x0A
+#define BT_ATT_ERROR_ATTRIBUTE_NOT_LONG			0x0B
+#define BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION_KEY_SIZE	0x0C
+#define BT_ATT_ERROR_INVALID_ATTRIBUTE_VALUE_LEN	0x0D
+#define BT_ATT_ERROR_UNLIKELY				0x0E
+#define BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION		0x0F
+#define BT_ATT_ERROR_UNSUPPORTED_GROUP_TYPE		0x10
+#define BT_ATT_ERROR_INSUFFICIENT_RESOURCES		0x11
+
+struct bt_att;
+
+struct bt_att *bt_att_new(int fd);
+struct bt_att *bt_att_ref(struct bt_att *att);
+void bt_att_unref(struct bt_att *att);
+bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
+
+typedef void (*bt_att_request_func_t)(uint8_t opcode, const void *param,
+					uint16_t length, void *user_data);
+typedef void (*bt_att_destroy_func_t)(void *user_data);
+typedef void (*bt_att_debug_func_t)(const char *str, void *user_data);
+typedef void (*bt_att_notify_func_t)(uint8_t opcode,
+					const struct bt_att_notify_param *param,
+					void *user_data);
+
+bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
+				void *user_data, bt_att_destroy_func_t destroy);
+
+uint16_t bt_att_get_mtu(struct bt_att *att);
+bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu);
+
+void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12]);
+
+unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
+				const void *param, uint16_t length,
+				bt_att_request_func_t callback, void *user_data,
+				bt_att_destroy_func_t destroy);
+bool bt_att_cancel_all(struct bt_att *att);
+
+unsigned int bt_att_register_request(struct bt_att *att,
+				bt_att_request_func_t callback,
+				void *user_data, bt_att_destroy_func_t destroy);
+unsigned int bt_att_register_notify(struct bt_att *att,
+				bt_att_notify_func_t callback,
+				void *user_data, bt_att_destroy_func_t destroy);
+bool bt_att_unregister(struct bt_att *att, unsigned int id);
+bool bt_att_unregister_all(struct bt_att *att);
-- 
1.8.3.2

[PATCH v3 2/4] src/shared/att: Implement basic boilerplate.

[Arman Uguray]

This patch implements the getters, setters, creation, ref, and unref
functions for struct bt_att. Also added is a simple table for
determining the ATT op type given an opcode and the io read handler that
currently does nothing.
---
 src/shared/att.c | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 264 insertions(+), 16 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 6398ca7..270a9df 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -34,6 +34,8 @@
 
 #define ATT_DEFAULT_LE_MTU		23
 #define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
+#define ATT_OP_CMD_MASK			0x40
+#define ATT_OP_SIGNED_MASK		0x80
 
 struct att_send_op;
 
@@ -51,12 +53,14 @@ struct bt_att {
 	struct queue *write_queue;	/* Queue of PDUs ready to send */
 	bool writer_active;
 
-	/* TODO Add queues for registered request and notification handlers */
+	/* TODO: Add queues for registered request and notification handlers */
+	/* TODO: Add a timeout mechanism for pending ops */
 
 	uint8_t *buf;
 	uint16_t mtu;
 
 	uint8_t auth_sig[12];
+	bool sig_set;
 
 	unsigned int next_send_id;	/* IDs for "send" ops */
 	unsigned int next_reg_id;	/* IDs for registered callbacks */
@@ -66,8 +70,68 @@ struct bt_att {
 	void *debug_data;
 };
 
+typedef enum {
+	ATT_OP_TYPE_REQ,
+	ATT_OP_TYPE_RSP,
+	ATT_OP_TYPE_CMD,
+	ATT_OP_TYPE_IND,
+	ATT_OP_TYPE_NOT,
+	ATT_OP_TYPE_CONF,
+	ATT_OP_TYPE_UNKNOWN,
+} att_op_type_t;
+
+static att_op_type_t get_op_type(uint8_t opcode)
+{
+	switch (opcode) {
+	/* Requests */
+	case BT_ATT_OP_MTU_REQ:
+	case BT_ATT_OP_FIND_INFO_REQ:
+	case BT_ATT_OP_FIND_BY_TYPE_VAL_REQ:
+	case BT_ATT_OP_READ_BY_TYPE_REQ:
+	case BT_ATT_OP_READ_REQ:
+	case BT_ATT_OP_READ_BLOB_REQ:
+	case BT_ATT_OP_READ_MULT_REQ:
+	case BT_ATT_OP_READ_BY_GRP_TYPE_REQ:
+	case BT_ATT_OP_WRITE_REQ:
+	case BT_ATT_OP_PREP_WRITE_REQ:
+	case BT_ATT_OP_EXEC_WRITE_REQ:
+		return ATT_OP_TYPE_REQ;
+	/* Commands */
+	case BT_ATT_OP_SIGNED_WRITE_CMD:
+	case BT_ATT_OP_WRITE_CMD:
+		return ATT_OP_TYPE_CMD;
+	/* Responses */
+	case BT_ATT_OP_ERROR_RSP:
+	case BT_ATT_OP_MTU_RSP:
+	case BT_ATT_OP_FIND_INFO_RSP:
+	case BT_ATT_OP_FIND_BY_TYPE_VAL_RSP:
+	case BT_ATT_OP_READ_BY_TYPE_RSP:
+	case BT_ATT_OP_READ_RSP:
+	case BT_ATT_OP_READ_BLOB_RSP:
+	case BT_ATT_OP_READ_MULT_RSP:
+	case BT_ATT_OP_READ_BY_GRP_TYPE_RSP:
+	case BT_ATT_OP_WRITE_RSP:
+	case BT_ATT_OP_PREP_WRITE_RSP:
+	case BT_ATT_OP_EXEC_WRITE_RSP:
+		return ATT_OP_TYPE_RSP;
+	/* Notifications */
+	case BT_ATT_OP_HANDLE_VAL_NOT:
+		return ATT_OP_TYPE_NOT;
+	/* Indications */
+	case BT_ATT_OP_HANDLE_VAL_IND:
+		return ATT_OP_TYPE_IND;
+	/* Confirmations */
+	case BT_ATT_OP_HANDLE_VAL_CONF:
+		return ATT_OP_TYPE_CONF;
+	default:
+		return ATT_OP_TYPE_UNKNOWN;
+	}
+	return ATT_OP_TYPE_UNKNOWN;
+}
+
 struct att_send_op {
 	unsigned int id;
+	att_op_type_t op_type;
 	uint16_t opcode;
 	void *pdu;
 	uint16_t len;
@@ -76,51 +140,229 @@ struct att_send_op {
 	void *user_data;
 };
 
+static void destroy_att_send_op(void *data)
+{
+	struct att_send_op *op = data;
+
+	if (op->destroy)
+		op->destroy(op->user_data);
+
+	free(op->pdu);
+	free(op);
+}
+
+static void read_watch_destroy(void *user_data)
+{
+	struct bt_att *att = user_data;
+
+	if (!att->destroyed)
+		return;
+
+	if (att->pending_req)
+		destroy_att_send_op(att->pending_req);
+
+	if (att->pending_ind)
+		destroy_att_send_op(att->pending_ind);
+
+	free(att);
+}
+
+static bool can_read_data(struct io *io, void *user_data)
+{
+	struct bt_att *att = user_data;
+	uint8_t *pdu;
+	ssize_t bytes_read;
+
+	bytes_read = read(att->fd, att->buf, att->mtu);
+	if (bytes_read < 0)
+		return false;
+
+	util_hexdump('>', att->buf, bytes_read,
+					att->debug_callback, att->debug_data);
+
+	if (bytes_read < ATT_MIN_PDU_LEN)
+		goto done;
+
+	/* TODO: Handle different types of PDUs here */
+
+done:
+	if (att->destroyed)
+		return false;
+
+	return true;
+}
+
 struct bt_att *bt_att_new(int fd)
 {
-	/* TODO */
+	struct bt_att *att;
+
+	if (fd < 0)
+		return NULL;
+
+	att = new0(struct bt_att, 1);
+	if (!att)
+		return NULL;
+
+	att->fd = fd;
+
+	att->mtu = ATT_DEFAULT_LE_MTU;
+	att->buf = malloc(att->mtu);
+	if (!att->buf)
+		goto fail;
+
+	att->io = io_new(fd);
+	if (!att->io)
+		goto fail;
+
+	att->req_queue = queue_new();
+	if (!att->req_queue)
+		goto fail;
+
+	att->ind_queue = queue_new();
+	if (!att->ind_queue)
+		goto fail;
+
+	att->write_queue = queue_new();
+	if (!att->write_queue)
+		goto fail;
+
+	if (!io_set_read_handler(att->io, can_read_data, att,
+							read_watch_destroy))
+		goto fail;
+
+	return bt_att_ref(att);
+
+fail:
+	queue_destroy(att->req_queue, NULL);
+	queue_destroy(att->ind_queue, NULL);
+	queue_destroy(att->write_queue, NULL);
+	io_destroy(att->io);
+	free(att->buf);
+	free(att);
+
 	return NULL;
 }
 
 struct bt_att *bt_att_ref(struct bt_att *att)
 {
-	/* TODO */
-	return NULL;
+	if (!att)
+		return NULL;
+
+	__sync_fetch_and_add(&att->ref_count, 1);
+
+	return att;
 }
 
 void bt_att_unref(struct bt_att *att)
 {
-	/* TODO */
+	if (!att)
+		return;
+
+	if (__sync_sub_and_fetch(&att->ref_count, 1))
+		return;
+
+	bt_att_cancel_all(att);
+
+	io_set_write_handler(att->io, NULL, NULL, NULL);
+	io_set_read_handler(att->io, NULL, NULL, NULL);
+
+	queue_destroy(att->req_queue, NULL);
+	queue_destroy(att->ind_queue, NULL);
+	queue_destroy(att->write_queue, NULL);
+	att->req_queue = NULL;
+	att->ind_queue = NULL;
+	att->write_queue = NULL;
+
+	io_destroy(att->io);
+	att->io = NULL;
+
+	if (att->close_on_unref)
+		close(att->fd);
+
+	if (att->debug_destroy)
+		att->debug_destroy(att->debug_data);
+
+	free(att->buf);
+	att->buf = NULL;
+
+	/*
+	 * Defer freeing the bt_att structure here; if this call to unref
+	 * happened during a callback called from the io read handler, we can
+	 * end up freeing the structure prematurely, in which case the clean up
+	 * should occur in read_watch_destroy.
+	 */
+	if (att->pending_req)
+		destroy_att_send_op(att->pending_req);
+
+	if (att->pending_ind)
+		destroy_att_send_op(att->pending_ind);
+
+	att->destroyed = true;
 }
 
 bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close)
 {
-	/* TODO */
-	return false;
+	if (!att)
+		return false;
+
+	att->close_on_unref = do_close;
+
+	return true;
 }
 
 bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
 				void *user_data, bt_att_destroy_func_t destroy)
 {
-	/* TODO */
-	return false;
+	if (!att)
+		return false;
+
+	if (att->debug_destroy)
+		att->debug_destroy(att->debug_data);
+
+	att->debug_callback = callback;
+	att->debug_destroy = destroy;
+	att->debug_data = user_data;
+
+	return true;
 }
 
 uint16_t bt_att_get_mtu(struct bt_att *att)
 {
-	/* TODO */
-	return 0;
+	if (!att)
+		return 0;
+
+	return att->mtu;
 }
 
 bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu)
 {
-	/* TODO */
-	return false;
+	char *buf;
+
+	if (!att)
+		return false;
+
+	if (mtu < ATT_DEFAULT_LE_MTU)
+		return false;
+
+	buf = malloc(mtu);
+	if (!buf)
+		return false;
+
+	free(att->buf);
+
+	att->mtu = mtu;
+	att->buf = buf;
+
+	return true;
 }
 
 void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12])
 {
-	/* TODO */
+	if (!att)
+		return;
+
+	memcpy(att->auth_sig, signature, sizeof(signature));
+	att->sig_set = true;
 }
 
 unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
@@ -134,8 +376,14 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
 
 bool bt_att_cancel_all(struct bt_att *att)
 {
-	/* TODO */
-	return false;
+	if (!att)
+		return false;
+
+	queue_remove_all(att->req_queue, NULL, NULL, destroy_att_send_op);
+	queue_remove_all(att->ind_queue, NULL, NULL, destroy_att_send_op);
+	queue_remove_all(att->write_queue, NULL, NULL, destroy_att_send_op);
+
+	return true;
 }
 
 unsigned int bt_att_register_request(struct bt_att *att,
-- 
1.8.3.2

[PATCH v3 3/4] src/shared/att: Implement write handler and bt_att_send.

[Arman Uguray]

This patch implements the write handler logic, including the way send
operations are process from the various internal queues. Added PDU
encoding for the Exchange MTU request.
---
 src/shared/att.c | 261 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 258 insertions(+), 3 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 270a9df..f3ece02 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -26,9 +26,11 @@
 #endif
 
 #include <unistd.h>
+#include <errno.h>
 
 #include "src/shared/io.h"
 #include "src/shared/queue.h"
+#include "src/shared/util.h"
 #include "lib/uuid.h"
 #include "src/shared/att.h"
 
@@ -133,13 +135,155 @@ struct att_send_op {
 	unsigned int id;
 	att_op_type_t op_type;
 	uint16_t opcode;
-	void *pdu;
+	uint8_t *pdu;
 	uint16_t len;
 	bt_att_request_func_t callback;
 	bt_att_destroy_func_t destroy;
 	void *user_data;
 };
 
+static bool encode_mtu_req(struct att_send_op *op, const void *param,
+						uint16_t length, uint16_t mtu)
+{
+	const struct bt_att_mtu_req_param *p = param;
+	const uint16_t len = 3;
+
+	if (length != sizeof(*p))
+		return false;
+
+	if (len > mtu)
+		return false;
+
+	op->pdu = malloc(len);
+	if (!op->pdu)
+		return false;
+
+	op->pdu[0] = op->opcode;
+	put_le16(p->client_rx_mtu, op->pdu + 1);
+	op->len = len;
+
+	return true;
+}
+
+static bool encode_pdu(struct att_send_op *op, const void *param,
+						uint16_t length, uint16_t mtu)
+{
+	/* If no parameters are given, simply set the PDU to consist of the
+	 * opcode.
+	 */
+	if (length == 0) {
+		op->len = 1;
+		op->pdu = malloc(1);
+		if (!op->pdu)
+			return false;
+
+		op->pdu[0] = op->opcode;
+		return true;
+	}
+
+	/* TODO: If the opcode has the "signed" bit set, make sure that the
+	 * resulting PDU contains the authentication signature. Return an error,
+	 * if the provided parameters structure is such that it leaves no room
+	 * for an authentication signature in the PDU.
+	 */
+
+	switch (op->opcode) {
+	case BT_ATT_OP_MTU_REQ:
+		return encode_mtu_req(op, param, length, mtu);
+	dafault:
+		break;
+	}
+
+	return false;
+}
+
+static struct att_send_op *create_att_send_op(uint8_t opcode, const void *param,
+						uint16_t length, uint16_t mtu,
+						bt_att_request_func_t callback,
+						void *user_data,
+						bt_att_destroy_func_t destroy)
+{
+	struct att_send_op *op;
+	att_op_type_t op_type;
+
+	op_type = get_op_type(opcode);
+	if (op_type == ATT_OP_TYPE_UNKNOWN)
+		return NULL;
+
+	/* If the opcode corresponds to an operation type that does not elicit a
+	 * response from the remote end, then no callbacks should have been
+	 * provided. Otherwise, at least a callback should be provided.
+	 */
+	if (op_type == ATT_OP_TYPE_REQ || op_type == ATT_OP_TYPE_IND) {
+		if (!callback)
+			return NULL;
+	} else if (callback || user_data || destroy)
+		return NULL;
+
+	if (length > 0 && !param)
+		return NULL;
+
+	op = new0(struct att_send_op, 1);
+	if (!op)
+		return NULL;
+
+	op->op_type = op_type;
+	op->opcode = opcode;
+	op->callback = callback;
+	op->destroy = destroy;
+	op->user_data = user_data;
+
+	if (!encode_pdu(op, param, length, mtu)) {
+		free(op);
+		return NULL;
+	}
+
+	return op;
+}
+
+typedef enum {
+	SEND_QUEUE_REQ,
+	SEND_QUEUE_IND,
+	SEND_QUEUE_WRITE,
+} send_queue_t;
+
+static struct att_send_op *pick_next_send_op(struct bt_att *att,
+						send_queue_t *orig_queue)
+{
+	struct att_send_op *op;
+
+	/* See if any operations are already in the write queue */
+	op = queue_pop_head(att->write_queue);
+	if (op) {
+		*orig_queue = SEND_QUEUE_WRITE;
+		return op;
+	}
+
+	/* If there is no pending request, pick an operation from the
+	 * request queue.
+	 */
+	if (!att->pending_req) {
+		op = queue_pop_head(att->req_queue);
+		if (op) {
+			*orig_queue = SEND_QUEUE_REQ;
+			return op;
+		}
+	}
+
+	/* There is either a request pending or no requests queued. If there is
+	 * no pending indication, pick an operation from the indication queue.
+	 */
+	if (!att->pending_ind) {
+		op = queue_pop_head(att->ind_queue);
+		if (op) {
+			*orig_queue = SEND_QUEUE_IND;
+			return op;
+		}
+	}
+
+	return NULL;
+}
+
 static void destroy_att_send_op(void *data)
 {
 	struct att_send_op *op = data;
@@ -151,6 +295,81 @@ static void destroy_att_send_op(void *data)
 	free(op);
 }
 
+static void wakeup_writer(struct bt_att *att);
+
+static bool can_write_data(struct io *io, void *user_data)
+{
+	struct bt_att *att = user_data;
+	struct att_send_op *op;
+	ssize_t bytes_written;
+	send_queue_t orig_queue;
+
+	op = pick_next_send_op(att, &orig_queue);
+	if (!op)
+		return false;
+
+	bytes_written = write(att->fd, op->pdu, op->len);
+	if (bytes_written < 0) {
+		util_debug(att->debug_callback, att->debug_data,
+					"write failed: %s", strerror(errno));
+		if (op->callback)
+			op->callback(BT_ATT_OP_ERROR_RSP, NULL, 0,
+							op->user_data);
+
+		destroy_att_send_op(op);
+		return true;
+	}
+
+	util_debug(att->debug_callback, att->debug_data,
+					"ATT op 0x%02x", op->opcode);
+
+	util_hexdump('<', op->pdu, bytes_written,
+					att->debug_callback, att->debug_data);
+
+	/* Based on the origin queue, set either the pending request or the
+	 * pending indication. If it came from the write queue, then there is
+	 * no need to keep it around.
+	 */
+	if (orig_queue == SEND_QUEUE_WRITE)
+		destroy_att_send_op(op);
+	else if (orig_queue == SEND_QUEUE_REQ)
+		att->pending_req = op;
+	else if (orig_queue == SEND_QUEUE_IND)
+		att->pending_ind = op;
+
+	/* Try to wake up the writer */
+	att->writer_active = false;
+	wakeup_writer(att);
+
+	return false;
+}
+
+static void wakeup_writer(struct bt_att *att)
+{
+	if (att->writer_active)
+		return;
+
+	/* Set the write handler only if there is anything that can be sent
+	 * at all.
+	 */
+	if (!queue_isempty(att->write_queue))
+		goto set_handler;
+
+	if (!att->pending_req && !queue_isempty(att->req_queue))
+		goto set_handler;
+
+	if (!att->pending_ind && !queue_isempty(att->ind_queue))
+		goto set_handler;
+
+	return;
+
+set_handler:
+	if (!io_set_write_handler(att->io, can_write_data, att, NULL))
+		return;
+
+	att->writer_active = true;
+}
+
 static void read_watch_destroy(void *user_data)
 {
 	struct bt_att *att = user_data;
@@ -370,8 +589,44 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
 				bt_att_request_func_t callback, void *user_data,
 				bt_att_destroy_func_t destroy)
 {
-	/* TODO */
-	return 0;
+	struct att_send_op *op;
+	struct queue *op_queue;
+
+	if (!att)
+		return 0;
+
+	op = create_att_send_op(opcode, param, length, att->mtu, callback,
+							user_data, destroy);
+	if (!op)
+		return 0;
+
+	if (att->next_send_id < 1)
+		att->next_send_id = 1;
+
+	op->id = att->next_send_id++;
+
+	/* Add the op to the correct queue based on its type */
+	switch (op->op_type) {
+	case ATT_OP_TYPE_REQ:
+		op_queue = att->req_queue;
+		break;
+	case ATT_OP_TYPE_IND:
+		op_queue = att->ind_queue;
+		break;
+	default:
+		op_queue = att->write_queue;
+		break;
+	}
+
+	if (!queue_push_tail(op_queue, op)) {
+		free(op->pdu);
+		free(op);
+		return 0;
+	}
+
+	wakeup_writer(att);
+
+	return op->id;
 }
 
 bool bt_att_cancel_all(struct bt_att *att)
-- 
1.8.3.2

[PATCH v3 4/4] src/shared/att: Handle incoming response PDUs

[Arman Uguray]

This patch adds handling for incoming ATT protocol response PDUs. Basic
incoming PDU handling logic added to the io read handler.
---
 src/shared/att.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 1 deletion(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index f3ece02..e967273 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -370,6 +370,109 @@ set_handler:
 	att->writer_active = true;
 }
 
+static bool request_complete(struct bt_att *att, uint8_t req_opcode,
+					uint8_t rsp_opcode, const void *param,
+					uint16_t len)
+{
+	struct att_send_op *op = att->pending_req;
+
+	if (!op) {
+		/* There is no pending request so the response is unexpected.
+		 * TODO: In such a case, the connection should be dropped
+		 * entirely. For now, simply ignore the PDU.
+		 */
+		wakeup_writer(att);
+		return false;
+	}
+
+	if (op->opcode != req_opcode)
+		/* The request opcode corresponding to the received response
+		 * opcode does not match the currently pending request.
+		 * TODO: The connection should be dropped in this case, since
+		 * an invalid response PDU has been received. For now, keep
+		 * waiting for the correct response.
+		 */
+		return false;
+
+	if (op->callback)
+		op->callback(rsp_opcode, param, len, op->user_data);
+
+	destroy_att_send_op(op);
+	att->pending_req = NULL;
+
+	if (!att->destroyed)
+		wakeup_writer(att);
+
+	return true;
+}
+
+static bool handle_error_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
+								ssize_t pdu_len)
+{
+	struct bt_att_error_rsp_param p;
+	bool result;
+
+	if (pdu_len != 5)
+		return false;
+
+	memset(&p, 0, sizeof(p));
+	p.request_opcode = pdu[1];
+	p.handle = get_le16(pdu + 2);
+	p.error_code = pdu[4];
+
+	/* Note: avoid a tail call */
+	result = request_complete(att, p.request_opcode, opcode, &p, sizeof(p));
+	return result;
+}
+
+static bool handle_mtu_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
+								ssize_t pdu_len)
+{
+	struct bt_att_mtu_rsp_param p;
+	bool result;
+
+	if (pdu_len != 3)
+		return false;
+
+	memset(&p, 0, sizeof(p));
+	p.server_rx_mtu = get_le16(pdu + 1);
+
+	/* Note: avoid a tail call */
+	result = request_complete(att, BT_ATT_OP_MTU_REQ, opcode,
+								&p, sizeof(p));
+	return result;
+}
+
+static void handle_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
+								ssize_t pdu_len)
+{
+	bool success;
+
+	switch (opcode) {
+	case BT_ATT_OP_ERROR_RSP:
+		success = handle_error_rsp(att, opcode, pdu, pdu_len);
+		break;
+	case BT_ATT_OP_MTU_RSP:
+		success = handle_mtu_rsp(att, opcode, pdu, pdu_len);
+		break;
+	default:
+		success = false;
+		util_debug(att->debug_callback, att->debug_data,
+				"Unknown response opcode: 0x%02x", opcode);
+		break;
+	}
+
+	if (success)
+		return;
+
+	util_debug(att->debug_callback, att->debug_data,
+			"Failed to handle respone PDU; opcode: 0x%02x", opcode);
+
+	if (att->pending_req)
+		request_complete(att, att->pending_req->opcode,
+						BT_ATT_OP_ERROR_RSP, NULL, 0);
+}
+
 static void read_watch_destroy(void *user_data)
 {
 	struct bt_att *att = user_data;
@@ -389,6 +492,7 @@ static void read_watch_destroy(void *user_data)
 static bool can_read_data(struct io *io, void *user_data)
 {
 	struct bt_att *att = user_data;
+	uint8_t opcode;
 	uint8_t *pdu;
 	ssize_t bytes_read;
 
@@ -402,7 +506,19 @@ static bool can_read_data(struct io *io, void *user_data)
 	if (bytes_read < ATT_MIN_PDU_LEN)
 		goto done;
 
-	/* TODO: Handle different types of PDUs here */
+	pdu = att->buf;
+	opcode = pdu[0];
+
+	/* Act on the received PDU based on the opcode type */
+	switch (get_op_type(opcode)) {
+	case ATT_OP_TYPE_RSP:
+		handle_rsp(att, opcode, pdu, bytes_read);
+		break;
+	default:
+		util_debug(att->debug_callback, att->debug_data,
+				"ATT opcode cannot be handled: 0x%02x", opcode);
+		break;
+	}
 
 done:
 	if (att->destroyed)
-- 
1.8.3.2

Re: [PATCH v3 1/4] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Arman,

> This patch introduces struct bt_att, which handles the transport and
> encoding/decoding for the ATT protocol. The structure of the code
> follows that of src/shared/mgmt and lib/mgmt.h, where individual
> parameter structures are defined for all ATT protocol requests, responses,
> commands, indications, and notifications. The serialization and
> endianness conversion for all parameters are handled by bt_att.
> 
> struct bt_att is based around struct io and operates on a raw file
> descriptor.
> ---
> Makefile.am      |   3 +-
> src/shared/att.c | 167 +++++++++++++++++++++++++++++++++++
> src/shared/att.h | 261 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 430 insertions(+), 1 deletion(-)
> create mode 100644 src/shared/att.c
> create mode 100644 src/shared/att.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index dc88816..9612170 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -156,7 +156,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
> 			src/shared/timeout.h src/shared/timeout-glib.c \
> 			src/shared/queue.h src/shared/queue.c \
> 			src/shared/util.h src/shared/util.c \
> -			src/shared/mgmt.h src/shared/mgmt.c
> +			src/shared/mgmt.h src/shared/mgmt.c \
> +			src/shared/att.h src/shared/att.c
> src_bluetoothd_LDADD = lib/libbluetooth-internal.la gdbus/libgdbus-internal.la \
> 			@GLIB_LIBS@ @DBUS_LIBS@ -ldl -lrt
> src_bluetoothd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic \
> diff --git a/src/shared/att.c b/src/shared/att.c
> new file mode 100644
> index 0000000..6398ca7
> --- /dev/null
> +++ b/src/shared/att.c
> @@ -0,0 +1,167 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <unistd.h>
> +
> +#include "src/shared/io.h"
> +#include "src/shared/queue.h"
> +#include "lib/uuid.h"
> +#include "src/shared/att.h"
> +
> +#define ATT_DEFAULT_LE_MTU		23
> +#define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
> +
> +struct att_send_op;
> +
> +struct bt_att {
> +	int ref_count;
> +	int fd;
> +	bool close_on_unref;
> +	struct io *io;
> +	bool destroyed;
> +
> +	struct queue *req_queue;	/* Queued ATT protocol requests */
> +	struct att_send_op *pending_req;
> +	struct queue *ind_queue;	/* Queued ATT protocol indications */
> +	struct att_send_op *pending_ind;
> +	struct queue *write_queue;	/* Queue of PDUs ready to send */
> +	bool writer_active;
> +
> +	/* TODO Add queues for registered request and notification handlers */
> +
> +	uint8_t *buf;
> +	uint16_t mtu;
> +
> +	uint8_t auth_sig[12];
> +
> +	unsigned int next_send_id;	/* IDs for "send" ops */
> +	unsigned int next_reg_id;	/* IDs for registered callbacks */
> +
> +	bt_att_debug_func_t debug_callback;
> +	bt_att_destroy_func_t debug_destroy;
> +	void *debug_data;
> +};
> +
> +struct att_send_op {

I would call it att_request or something like that. Not really important since we can change that easily later. Just something to think about. I have to read through the rest of the patchset and it make more sense then, but right now att_send_op sounds are bit weird.

> +	unsigned int id;
> +	uint16_t opcode;
> +	void *pdu;
> +	uint16_t len;
> +	bt_att_request_func_t callback;
> +	bt_att_destroy_func_t destroy;
> +	void *user_data;
> +};
> +
> +struct bt_att *bt_att_new(int fd)
> +{
> +	/* TODO */
> +	return NULL;
> +}
> +
> +struct bt_att *bt_att_ref(struct bt_att *att)
> +{
> +	/* TODO */
> +	return NULL;
> +}

I am normally not a big fan of adding empty functions to just fill them later. However it has the advantage of me seeing the big picture which is kinda nice.

<snip>

> diff --git a/src/shared/att.h b/src/shared/att.h
> new file mode 100644
> index 0000000..b358d8f
> --- /dev/null
> +++ b/src/shared/att.h
> @@ -0,0 +1,261 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +/* Error response */
> +#define BT_ATT_OP_ERROR_RSP	      		0x01
> +struct bt_att_error_rsp_param {
> +	uint8_t request_opcode;
> +	uint16_t handle;
> +	uint8_t	error_code;
> +};
> +
> +/* Exchange MTU */
> +#define BT_ATT_OP_MTU_REQ			0x02
> +struct bt_att_mtu_req_param {
> +	uint16_t client_rx_mtu;
> +};
> +
> +#define BT_ATT_OP_MTU_RSP			0x03
> +struct bt_att_mtu_rsp_param {
> +	uint16_t server_rx_mtu;
> +};
> +
> +/* Find Information */
> +#define BT_ATT_OP_FIND_INFO_REQ			0x04
> +struct bt_att_find_info_req_param {
> +	uint16_t start_handle;
> +	uint16_t end_handle;
> +};
> +
> +#define BT_ATT_OP_FIND_INFO_RSP			0x05
> +struct bt_att_find_info_rsp_param {
> +	uint8_t format;
> +	const uint8_t *info_data;
> +	uint16_t length;
> +};
> +
> +/* Find By Type Value */
> +#define BT_ATT_OP_FIND_BY_TYPE_VAL_REQ		0x06
> +struct bt_att_find_by_type_value_req_param {
> +	uint16_t start_handle;
> +	uint16_t end_handle;
> +	uint16_t type;  /* 2 octet UUID */
> +	const uint8_t *value;
> +	uint16_t length;  /* MAX length: (ATT_MTU - 7) */
> +};
> +
> +#define BT_ATT_OP_FIND_BY_TYPE_VAL_RSP		0x07
> +struct bt_att_find_by_type_value_rsp_param {
> +	const uint8_t *handles_info_list;
> +	uint16_t length;
> +};
> +
> +/* Read By Type */
> +#define BT_ATT_OP_READ_BY_TYPE_REQ		0x08
> +struct bt_att_read_by_type_req_param {
> +	uint16_t start_handle;
> +	uint16_t end_handle;
> +	bt_uuid_t type;  /* 2 or 16 octet UUID */
> +};
> +
> +#define BT_ATT_OP_READ_BY_TYPE_RSP		0x09
> +struct bt_att_read_by_type_rsp_param {
> +	uint8_t length;
> +	const uint8_t *attr_data_list;
> +	uint16_t list_length;  /* Length of "attr_data_list" */
> +};
> +
> +/* Read */
> +#define BT_ATT_OP_READ_REQ			0x0a
> +struct bt_att_read_req_param {
> +	uint16_t handle;
> +};
> +
> +#define BT_ATT_OP_READ_RSP			0x0b
> +struct bt_att_read_rsp_param {
> +	const uint8_t *value;
> +	uint16_t length;
> +};
> +
> +/* Read Blob */
> +#define BT_ATT_OP_READ_BLOB_REQ			0x0c
> +struct bt_att_read_blob_req_param {
> +	uint16_t handle;
> +	uint16_t offset;
> +};
> +
> +#define BT_ATT_OP_READ_BLOB_RSP			0x0d
> +struct bt_att_read_blob_rsp_param {
> +	const uint8_t *part_value;
> +	uint16_t length;
> +};
> +
> +/* Read Multiple */
> +#define BT_ATT_OP_READ_MULT_REQ			0x0e
> +struct bt_att_read_multiple_req_param {
> +	const uint16_t *handles;
> +	uint16_t num_handles;
> +};
> +
> +#define BT_ATT_OP_READ_MULT_RSP			0x0f
> +struct bt_att_read_multiple_rsp_param {
> +	const uint8_t *values;
> +	uint16_t length;
> +};
> +
> +/* Read By Group Type */
> +#define BT_ATT_OP_READ_BY_GRP_TYPE_REQ		0x10
> +struct bt_att_read_by_group_type_req_param {
> +	uint16_t start_handle;
> +	uint16_t end_handle;
> +	bt_uuid_t type;
> +};
> +
> +#define BT_ATT_OP_READ_BY_GRP_TYPE_RSP		0x11
> +struct bt_att_read_by_group_type_rsp_param {
> +	uint8_t length;
> +	const uint8_t *attr_data_list;
> +	uint16_t list_length;  /* Length of "attr_data_list" */
> +};
> +
> +/* Write Request */
> +#define BT_ATT_OP_WRITE_REQ			0x12
> +/*
> + * bt_att_write_param is used for write request and signed and unsigned write
> + * command.
> + */
> +struct bt_att_write_param {
> +	uint16_t handle;
> +	const uint8_t *value;
> +	uint16_t length;
> +};
> +
> +#define BT_ATT_OP_WRITE_RSP			0x13  /* No parameters */
> +
> +/* Write Command */
> +#define BT_ATT_OP_WRITE_CMD			0x52
> +
> +/* Signed Write Command */
> +#define BT_ATT_OP_SIGNED_WRITE_CMD		0xD2
> +
> +/* Prepare Write */
> +#define BT_ATT_OP_PREP_WRITE_REQ		0x16
> +struct bt_att_prepare_write_req_param {
> +	uint16_t handle;
> +	uint16_t offset;
> +	const uint8_t *part_value;
> +	uint16_t length;
> +};
> +
> +#define BT_ATT_OP_PREP_WRITE_RSP		0x17
> +struct bt_att_prepare_write_rsp_param {
> +	uint16_t handle;
> +	uint16_t offset;
> +	const uint8_t *part_value;
> +	uint16_t length;
> +};
> +
> +/* Execute Write */
> +#define BT_ATT_OP_EXEC_WRITE_REQ		0x18
> +typedef enum {
> +	BT_ATT_EXEC_WRITE_FLAG_CANCEL	= 0x00,
> +	BT_ATT_EXEC_WRITE_FLAG_WRITE	= 0x01,
> +} bt_att_exec_write_flag_t;
> +
> +struct bt_att_exec_write_req_param {
> +	bt_att_exec_write_flag_t flags;
> +};
> +
> +#define BT_ATT_OP_EXEC_WRITE_RSP		0x19
> +
> +/* Handle Value Notification/Indication */
> +#define BT_ATT_OP_HANDLE_VAL_NOT		0x1B
> +#define BT_ATT_OP_HANDLE_VAL_IND		0x1D
> +struct bt_att_notify_param {
> +	uint16_t handle;
> +	const uint8_t *value;
> +	uint16_t length;
> +};
> +
> +/* Handle Value Confirmation */
> +#define BT_ATT_OP_HANDLE_VAL_CONF		0x1E
> +
> +/* Error codes for Error response PDU */
> +#define BT_ATT_ERROR_INVALID_HANDLE			0x01
> +#define BT_ATT_ERROR_READ_NOT_PERMITTED			0x02
> +#define BT_ATT_ERROR_WRITE_NOT_PERMITTED		0x03
> +#define BT_ATT_ERROR_INVALID_PDU			0x04
> +#define BT_ATT_ERROR_AUTHENTICATION			0x05
> +#define BT_ATT_ERROR_REQUEST_NOT_SUPPORTED		0x06
> +#define BT_ATT_ERROR_INVALID_OFFSET			0x07
> +#define BT_ATT_ERROR_AUTHORIZATION			0x08
> +#define BT_ATT_ERROR_PREPARE_QUEUE_FULL			0x09
> +#define BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND		0x0A
> +#define BT_ATT_ERROR_ATTRIBUTE_NOT_LONG			0x0B
> +#define BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION_KEY_SIZE	0x0C
> +#define BT_ATT_ERROR_INVALID_ATTRIBUTE_VALUE_LEN	0x0D
> +#define BT_ATT_ERROR_UNLIKELY				0x0E
> +#define BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION		0x0F
> +#define BT_ATT_ERROR_UNSUPPORTED_GROUP_TYPE		0x10
> +#define BT_ATT_ERROR_INSUFFICIENT_RESOURCES		0x11

these might be better placed in monitor/bt.h and we eventually move that include file to some other place. One alternate idea is to use src/shared/att_types.h and just include it from src/shared/att.h. I am open for suggestions here. What seems better.

> +
> +struct bt_att;
> +
> +struct bt_att *bt_att_new(int fd);
> +struct bt_att *bt_att_ref(struct bt_att *att);
> +void bt_att_unref(struct bt_att *att);

This is purely cosmetic. Add an empty line here to separate constructor/reference functions from the others.

> +bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
> +
> +typedef void (*bt_att_request_func_t)(uint8_t opcode, const void *param,
> +					uint16_t length, void *user_data);
> +typedef void (*bt_att_destroy_func_t)(void *user_data);
> +typedef void (*bt_att_debug_func_t)(const char *str, void *user_data);
> +typedef void (*bt_att_notify_func_t)(uint8_t opcode,
> +					const struct bt_att_notify_param *param,
> +					void *user_data);
> +
> +bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
> +				void *user_data, bt_att_destroy_func_t destroy);
> +
> +uint16_t bt_att_get_mtu(struct bt_att *att);
> +bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu);
> +
> +void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12]);

This might not work. We need the signature resolving key for each direction. And why did you include it as uint8_t[12]. They overall key is 16 octets.

I think this should be bt_att_set_signing_key() and bt_att_resolving_key() and once they are set we can internally drop signed writes that do not fit. And we can also automatically sign if the signed write opcode is used. In addition skip the signed write and just use a normal write if we are already encrypted.

> +
> +unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> +				const void *param, uint16_t length,
> +				bt_att_request_func_t callback, void *user_data,
> +				bt_att_destroy_func_t destroy);

This should also include a bt_att_cancel to cancel an individual transaction.

> +bool bt_att_cancel_all(struct bt_att *att);
> +
> +unsigned int bt_att_register_request(struct bt_att *att,
> +				bt_att_request_func_t callback,
> +				void *user_data, bt_att_destroy_func_t destroy);
> +unsigned int bt_att_register_notify(struct bt_att *att,
> +				bt_att_notify_func_t callback,
> +				void *user_data, bt_att_destroy_func_t destroy);

I do not like this split between request and notify. Do we need this distinction. I would like to be just able to register for a given opcode. And not all opcode. Just register for the one that you are interested in. This catch all type of things just end up with a big switch statement in the callback. I prefer having individual callbacks right from the beginning.

This also has the advantage that if no specific callback is registered for an opcode, we can internally handle the response to reply with an error. So users don't need to provide all of them if they do not want to. This makes the code nicely reusable.

> +bool bt_att_unregister(struct bt_att *att, unsigned int id);
> +bool bt_att_unregister_all(struct bt_att *att);

Regards

Marcel

Re: [PATCH v3 2/4] src/shared/att: Implement basic boilerplate.

[Marcel Holtmann]

Hi Arman,

> This patch implements the getters, setters, creation, ref, and unref
> functions for struct bt_att. Also added is a simple table for
> determining the ATT op type given an opcode and the io read handler that
> currently does nothing.
> ---
> src/shared/att.c | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 264 insertions(+), 16 deletions(-)
> 
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 6398ca7..270a9df 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -34,6 +34,8 @@
> 
> #define ATT_DEFAULT_LE_MTU		23
> #define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
> +#define ATT_OP_CMD_MASK			0x40
> +#define ATT_OP_SIGNED_MASK		0x80
> 
> struct att_send_op;
> 
> @@ -51,12 +53,14 @@ struct bt_att {
> 	struct queue *write_queue;	/* Queue of PDUs ready to send */
> 	bool writer_active;
> 
> -	/* TODO Add queues for registered request and notification handlers */
> +	/* TODO: Add queues for registered request and notification handlers */
> +	/* TODO: Add a timeout mechanism for pending ops */
> 
> 	uint8_t *buf;
> 	uint16_t mtu;
> 
> 	uint8_t auth_sig[12];
> +	bool sig_set;
> 
> 	unsigned int next_send_id;	/* IDs for "send" ops */
> 	unsigned int next_reg_id;	/* IDs for registered callbacks */
> @@ -66,8 +70,68 @@ struct bt_att {
> 	void *debug_data;
> };
> 
> +typedef enum {
> +	ATT_OP_TYPE_REQ,
> +	ATT_OP_TYPE_RSP,
> +	ATT_OP_TYPE_CMD,
> +	ATT_OP_TYPE_IND,
> +	ATT_OP_TYPE_NOT,
> +	ATT_OP_TYPE_CONF,
> +	ATT_OP_TYPE_UNKNOWN,
> +} att_op_type_t;

no need for a typedef here.

	enum att_op_type {
	};

> +
> +static att_op_type_t get_op_type(uint8_t opcode)
> +{
> +	switch (opcode) {
> +	/* Requests */
> +	case BT_ATT_OP_MTU_REQ:
> +	case BT_ATT_OP_FIND_INFO_REQ:
> +	case BT_ATT_OP_FIND_BY_TYPE_VAL_REQ:
> +	case BT_ATT_OP_READ_BY_TYPE_REQ:
> +	case BT_ATT_OP_READ_REQ:
> +	case BT_ATT_OP_READ_BLOB_REQ:
> +	case BT_ATT_OP_READ_MULT_REQ:
> +	case BT_ATT_OP_READ_BY_GRP_TYPE_REQ:
> +	case BT_ATT_OP_WRITE_REQ:
> +	case BT_ATT_OP_PREP_WRITE_REQ:
> +	case BT_ATT_OP_EXEC_WRITE_REQ:
> +		return ATT_OP_TYPE_REQ;
> +	/* Commands */
> +	case BT_ATT_OP_SIGNED_WRITE_CMD:
> +	case BT_ATT_OP_WRITE_CMD:
> +		return ATT_OP_TYPE_CMD;
> +	/* Responses */
> +	case BT_ATT_OP_ERROR_RSP:
> +	case BT_ATT_OP_MTU_RSP:
> +	case BT_ATT_OP_FIND_INFO_RSP:
> +	case BT_ATT_OP_FIND_BY_TYPE_VAL_RSP:
> +	case BT_ATT_OP_READ_BY_TYPE_RSP:
> +	case BT_ATT_OP_READ_RSP:
> +	case BT_ATT_OP_READ_BLOB_RSP:
> +	case BT_ATT_OP_READ_MULT_RSP:
> +	case BT_ATT_OP_READ_BY_GRP_TYPE_RSP:
> +	case BT_ATT_OP_WRITE_RSP:
> +	case BT_ATT_OP_PREP_WRITE_RSP:
> +	case BT_ATT_OP_EXEC_WRITE_RSP:
> +		return ATT_OP_TYPE_RSP;
> +	/* Notifications */
> +	case BT_ATT_OP_HANDLE_VAL_NOT:
> +		return ATT_OP_TYPE_NOT;
> +	/* Indications */
> +	case BT_ATT_OP_HANDLE_VAL_IND:
> +		return ATT_OP_TYPE_IND;
> +	/* Confirmations */
> +	case BT_ATT_OP_HANDLE_VAL_CONF:
> +		return ATT_OP_TYPE_CONF;
> +	default:
> +		return ATT_OP_TYPE_UNKNOWN;
> +	}
> +	return ATT_OP_TYPE_UNKNOWN;
> +}

I have the feeling that a const static table would be easier to read and understand.

	{ BT_ATT_OP_SIGNED_WRITE_CMD,	ATT_OP_TYPE_CMD		},
	{ BT_ATT_OP_HANDLE_VAL_CONF,	ATT_OP_TYPE_CONF	},
	{ }

And then a simple helper function to walk the table. See how it looks like if you do it that way.

> +
> struct att_send_op {
> 	unsigned int id;
> +	att_op_type_t op_type;
> 	uint16_t opcode;
> 	void *pdu;
> 	uint16_t len;
> @@ -76,51 +140,229 @@ struct att_send_op {
> 	void *user_data;
> };
> 
> +static void destroy_att_send_op(void *data)
> +{
> +	struct att_send_op *op = data;
> +
> +	if (op->destroy)
> +		op->destroy(op->user_data);
> +
> +	free(op->pdu);
> +	free(op);
> +}
> +
> +static void read_watch_destroy(void *user_data)
> +{
> +	struct bt_att *att = user_data;
> +
> +	if (!att->destroyed)
> +		return;
> +
> +	if (att->pending_req)
> +		destroy_att_send_op(att->pending_req);
> +
> +	if (att->pending_ind)
> +		destroy_att_send_op(att->pending_ind);
> +
> +	free(att);
> +}
> +
> +static bool can_read_data(struct io *io, void *user_data)
> +{
> +	struct bt_att *att = user_data;
> +	uint8_t *pdu;
> +	ssize_t bytes_read;
> +
> +	bytes_read = read(att->fd, att->buf, att->mtu);
> +	if (bytes_read < 0)
> +		return false;
> +
> +	util_hexdump('>', att->buf, bytes_read,
> +					att->debug_callback, att->debug_data);
> +
> +	if (bytes_read < ATT_MIN_PDU_LEN)
> +		goto done;
> +
> +	/* TODO: Handle different types of PDUs here */
> +
> +done:
> +	if (att->destroyed)
> +		return false;
> +
> +	return true;
> +}
> +
> struct bt_att *bt_att_new(int fd)
> {
> -	/* TODO */
> +	struct bt_att *att;
> +
> +	if (fd < 0)
> +		return NULL;
> +
> +	att = new0(struct bt_att, 1);
> +	if (!att)
> +		return NULL;
> +
> +	att->fd = fd;
> +
> +	att->mtu = ATT_DEFAULT_LE_MTU;
> +	att->buf = malloc(att->mtu);
> +	if (!att->buf)
> +		goto fail;
> +
> +	att->io = io_new(fd);
> +	if (!att->io)
> +		goto fail;
> +
> +	att->req_queue = queue_new();
> +	if (!att->req_queue)
> +		goto fail;
> +
> +	att->ind_queue = queue_new();
> +	if (!att->ind_queue)
> +		goto fail;
> +
> +	att->write_queue = queue_new();
> +	if (!att->write_queue)
> +		goto fail;
> +
> +	if (!io_set_read_handler(att->io, can_read_data, att,
> +							read_watch_destroy))
> +		goto fail;
> +
> +	return bt_att_ref(att);
> +
> +fail:
> +	queue_destroy(att->req_queue, NULL);
> +	queue_destroy(att->ind_queue, NULL);
> +	queue_destroy(att->write_queue, NULL);
> +	io_destroy(att->io);
> +	free(att->buf);
> +	free(att);
> +
> 	return NULL;
> }
> 
> struct bt_att *bt_att_ref(struct bt_att *att)
> {
> -	/* TODO */
> -	return NULL;
> +	if (!att)
> +		return NULL;
> +
> +	__sync_fetch_and_add(&att->ref_count, 1);
> +
> +	return att;
> }
> 
> void bt_att_unref(struct bt_att *att)
> {
> -	/* TODO */
> +	if (!att)
> +		return;
> +
> +	if (__sync_sub_and_fetch(&att->ref_count, 1))
> +		return;
> +
> +	bt_att_cancel_all(att);
> +
> +	io_set_write_handler(att->io, NULL, NULL, NULL);
> +	io_set_read_handler(att->io, NULL, NULL, NULL);
> +
> +	queue_destroy(att->req_queue, NULL);
> +	queue_destroy(att->ind_queue, NULL);
> +	queue_destroy(att->write_queue, NULL);
> +	att->req_queue = NULL;
> +	att->ind_queue = NULL;
> +	att->write_queue = NULL;
> +
> +	io_destroy(att->io);
> +	att->io = NULL;
> +
> +	if (att->close_on_unref)
> +		close(att->fd);
> +
> +	if (att->debug_destroy)
> +		att->debug_destroy(att->debug_data);
> +
> +	free(att->buf);
> +	att->buf = NULL;
> +
> +	/*
> +	 * Defer freeing the bt_att structure here; if this call to unref
> +	 * happened during a callback called from the io read handler, we can
> +	 * end up freeing the structure prematurely, in which case the clean up
> +	 * should occur in read_watch_destroy.
> +	 */

I think the comment is meant for the att->destroyed = true one. Within mgmt.c we only protect against the in_notify and you might want to do something similar here. If we are in no single callback we should just free here and return. Otherwise nobody will ever process the att->destroyed handling.

This is a bit complicated handling of unrefing, but needed to allow reentrant handling which you end up encountering from time to time. If you want to keep it simple, then initially you can start without dealing with this detail and remove att->destroyed. We can introduce it later (happened with mgmt as well). Totally your choice.

> +	if (att->pending_req)
> +		destroy_att_send_op(att->pending_req);
> +
> +	if (att->pending_ind)
> +		destroy_att_send_op(att->pending_ind);
> +
> +	att->destroyed = true;
> }
> 
> bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close)
> {
> -	/* TODO */
> -	return false;
> +	if (!att)
> +		return false;
> +
> +	att->close_on_unref = do_close;
> +
> +	return true;
> }
> 
> bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
> 				void *user_data, bt_att_destroy_func_t destroy)
> {
> -	/* TODO */
> -	return false;
> +	if (!att)
> +		return false;
> +
> +	if (att->debug_destroy)
> +		att->debug_destroy(att->debug_data);
> +
> +	att->debug_callback = callback;
> +	att->debug_destroy = destroy;
> +	att->debug_data = user_data;
> +
> +	return true;
> }
> 
> uint16_t bt_att_get_mtu(struct bt_att *att)
> {
> -	/* TODO */
> -	return 0;
> +	if (!att)
> +		return 0;
> +
> +	return att->mtu;
> }
> 
> bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu)
> {
> -	/* TODO */
> -	return false;
> +	char *buf;
> +
> +	if (!att)
> +		return false;
> +
> +	if (mtu < ATT_DEFAULT_LE_MTU)
> +		return false;
> +
> +	buf = malloc(mtu);
> +	if (!buf)
> +		return false;
> +
> +	free(att->buf);
> +
> +	att->mtu = mtu;
> +	att->buf = buf;
> +
> +	return true;
> }
> 
> void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12])
> {
> -	/* TODO */
> +	if (!att)
> +		return;
> +
> +	memcpy(att->auth_sig, signature, sizeof(signature));
> +	att->sig_set = true;
> }
> 
> unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> @@ -134,8 +376,14 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> 
> bool bt_att_cancel_all(struct bt_att *att)
> {
> -	/* TODO */
> -	return false;
> +	if (!att)
> +		return false;
> +
> +	queue_remove_all(att->req_queue, NULL, NULL, destroy_att_send_op);
> +	queue_remove_all(att->ind_queue, NULL, NULL, destroy_att_send_op);
> +	queue_remove_all(att->write_queue, NULL, NULL, destroy_att_send_op);
> +
> +	return true;
> }
> 
> unsigned int bt_att_register_request(struct bt_att *att,

Regards

Marcel

Re: [PATCH v3 3/4] src/shared/att: Implement write handler and bt_att_send.

[Marcel Holtmann]

Hi Arman,

> This patch implements the write handler logic, including the way send
> operations are process from the various internal queues. Added PDU
> encoding for the Exchange MTU request.
> ---
> src/shared/att.c | 261 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 258 insertions(+), 3 deletions(-)
> 
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 270a9df..f3ece02 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -26,9 +26,11 @@
> #endif
> 
> #include <unistd.h>
> +#include <errno.h>
> 
> #include "src/shared/io.h"
> #include "src/shared/queue.h"
> +#include "src/shared/util.h"
> #include "lib/uuid.h"
> #include "src/shared/att.h"
> 
> @@ -133,13 +135,155 @@ struct att_send_op {
> 	unsigned int id;
> 	att_op_type_t op_type;
> 	uint16_t opcode;
> -	void *pdu;
> +	uint8_t *pdu;

Why? I prefer we operate on void pointers and if you need access to specific elements without being able to have a packed struct, use the unaligned access helpers. You need to do that anyway for anything that is not a 1 octet size field.

> 	uint16_t len;
> 	bt_att_request_func_t callback;
> 	bt_att_destroy_func_t destroy;
> 	void *user_data;
> };
> 
> +static bool encode_mtu_req(struct att_send_op *op, const void *param,
> +						uint16_t length, uint16_t mtu)
> +{
> +	const struct bt_att_mtu_req_param *p = param;
> +	const uint16_t len = 3;
> +
> +	if (length != sizeof(*p))
> +		return false;
> +
> +	if (len > mtu)
> +		return false;
> +
> +	op->pdu = malloc(len);
> +	if (!op->pdu)
> +		return false;
> +
> +	op->pdu[0] = op->opcode;
> +	put_le16(p->client_rx_mtu, op->pdu + 1);
> +	op->len = len;

I see. Just use *((uint8_t *) op->pdu) = op->opcode here. If the pattern of adding opcode plus some extra data repeats, then we might should consider a define or a simpler att_hdr struct.

	struct att_hdr {
		uint8_t opcode;
		uint8_t data[0];
	} __packed;

You need to play a little bit with your options here and see what turns into most readable code.

> +
> +	return true;
> +}
> +
> +static bool encode_pdu(struct att_send_op *op, const void *param,
> +						uint16_t length, uint16_t mtu)
> +{
> +	/* If no parameters are given, simply set the PDU to consist of the
> +	 * opcode.
> +	 */

Is this possible at all? I am just too lazy to open the spec and check, but you might want to give an example n which opcode has this.

> +	if (length == 0) {

Here I am on the fence with !length or length < 1 or length == 0. In general we moved towards !length syntax for new code. So might just do it here as well. Additionally it is also a good idea to check that !param unless it all internal and we know for sure it will never be NULL.

> +		op->len = 1;
> +		op->pdu = malloc(1);
> +		if (!op->pdu)
> +			return false;
> +
> +		op->pdu[0] = op->opcode;
> +		return true;
> +	}
> +
> +	/* TODO: If the opcode has the "signed" bit set, make sure that the
> +	 * resulting PDU contains the authentication signature. Return an error,
> +	 * if the provided parameters structure is such that it leaves no room
> +	 * for an authentication signature in the PDU.
> +	 */

I want us to handle this in the background. Just make sure we allocate enough memory.

> +
> +	switch (op->opcode) {
> +	case BT_ATT_OP_MTU_REQ:
> +		return encode_mtu_req(op, param, length, mtu);
> +	dafault:
> +		break;
> +	}
> +
> +	return false;
> +}
> +
> +static struct att_send_op *create_att_send_op(uint8_t opcode, const void *param,
> +						uint16_t length, uint16_t mtu,
> +						bt_att_request_func_t callback,
> +						void *user_data,
> +						bt_att_destroy_func_t destroy)
> +{
> +	struct att_send_op *op;
> +	att_op_type_t op_type;
> +
> +	op_type = get_op_type(opcode);
> +	if (op_type == ATT_OP_TYPE_UNKNOWN)
> +		return NULL;
> +
> +	/* If the opcode corresponds to an operation type that does not elicit a
> +	 * response from the remote end, then no callbacks should have been
> +	 * provided. Otherwise, at least a callback should be provided.
> +	 */
> +	if (op_type == ATT_OP_TYPE_REQ || op_type == ATT_OP_TYPE_IND) {
> +		if (!callback)
> +			return NULL;
> +	} else if (callback || user_data || destroy)
> +		return NULL;

So I think we should do be basic NULL checks first. However it should never be required to provide user_data and destroy. That is a caller choice and if they want to be stupid so be it. These values are just handed through on bt_att side. We do not care what they are. If they are NULL, then that is fine as well.

> +
> +	if (length > 0 && !param)
> +		return NULL;

Same would apply for !length && param as noted above. We need to make sure that these input values are sane.

> +
> +	op = new0(struct att_send_op, 1);
> +	if (!op)
> +		return NULL;
> +
> +	op->op_type = op_type;
> +	op->opcode = opcode;
> +	op->callback = callback;
> +	op->destroy = destroy;
> +	op->user_data = user_data;
> +
> +	if (!encode_pdu(op, param, length, mtu)) {
> +		free(op);
> +		return NULL;
> +	}
> +
> +	return op;
> +}
> +
> +typedef enum {
> +	SEND_QUEUE_REQ,
> +	SEND_QUEUE_IND,
> +	SEND_QUEUE_WRITE,
> +} send_queue_t;

enum send_queue_type {
}

> +
> +static struct att_send_op *pick_next_send_op(struct bt_att *att,
> +						send_queue_t *orig_queue)
> +{
> +	struct att_send_op *op;
> +
> +	/* See if any operations are already in the write queue */
> +	op = queue_pop_head(att->write_queue);
> +	if (op) {
> +		*orig_queue = SEND_QUEUE_WRITE;
> +		return op;
> +	}
> +
> +	/* If there is no pending request, pick an operation from the
> +	 * request queue.
> +	 */
> +	if (!att->pending_req) {
> +		op = queue_pop_head(att->req_queue);
> +		if (op) {
> +			*orig_queue = SEND_QUEUE_REQ;
> +			return op;
> +		}
> +	}
> +
> +	/* There is either a request pending or no requests queued. If there is
> +	 * no pending indication, pick an operation from the indication queue.
> +	 */
> +	if (!att->pending_ind) {
> +		op = queue_pop_head(att->ind_queue);
> +		if (op) {
> +			*orig_queue = SEND_QUEUE_IND;
> +			return op;
> +		}
> +	}
> +
> +	return NULL;
> +}

I feel a bit uneasy about this queue type thing. Maybe I am just not getting it usefulness. You need to explain this to me a bit in detail.

> +
> static void destroy_att_send_op(void *data)
> {
> 	struct att_send_op *op = data;
> @@ -151,6 +295,81 @@ static void destroy_att_send_op(void *data)
> 	free(op);
> }
> 
> +static void wakeup_writer(struct bt_att *att);
> +

I now that we can not always avoid forward declaration, but if possible with reordering some functions, then lets try. If you tell me that you tried and we can not avoid it, then that is fine as well, but at least try to avoid forward declarations.

> +static bool can_write_data(struct io *io, void *user_data)
> +{
> +	struct bt_att *att = user_data;
> +	struct att_send_op *op;
> +	ssize_t bytes_written;
> +	send_queue_t orig_queue;
> +
> +	op = pick_next_send_op(att, &orig_queue);
> +	if (!op)
> +		return false;
> +
> +	bytes_written = write(att->fd, op->pdu, op->len);
> +	if (bytes_written < 0) {
> +		util_debug(att->debug_callback, att->debug_data,
> +					"write failed: %s", strerror(errno));
> +		if (op->callback)
> +			op->callback(BT_ATT_OP_ERROR_RSP, NULL, 0,
> +							op->user_data);
> +
> +		destroy_att_send_op(op);
> +		return true;
> +	}
> +
> +	util_debug(att->debug_callback, att->debug_data,
> +					"ATT op 0x%02x", op->opcode);
> +
> +	util_hexdump('<', op->pdu, bytes_written,
> +					att->debug_callback, att->debug_data);
> +
> +	/* Based on the origin queue, set either the pending request or the
> +	 * pending indication. If it came from the write queue, then there is
> +	 * no need to keep it around.
> +	 */
> +	if (orig_queue == SEND_QUEUE_WRITE)
> +		destroy_att_send_op(op);
> +	else if (orig_queue == SEND_QUEUE_REQ)
> +		att->pending_req = op;
> +	else if (orig_queue == SEND_QUEUE_IND)
> +		att->pending_ind = op;
> +
> +	/* Try to wake up the writer */
> +	att->writer_active = false;
> +	wakeup_writer(att);
> +
> +	return false;
> +}

I do not get this wakeup_writer here. We not just return true and the writer will be called again. Maybe this is where the forward declaration comes from. Seriously, if we still have data in the queue to write, just return true.

Maybe mgmt is a bad example since it does not have these multiple queues and it is really just one write and then wait for a response.

> +
> +static void wakeup_writer(struct bt_att *att)
> +{
> +	if (att->writer_active)
> +		return;
> +
> +	/* Set the write handler only if there is anything that can be sent
> +	 * at all.
> +	 */
> +	if (!queue_isempty(att->write_queue))
> +		goto set_handler;
> +
> +	if (!att->pending_req && !queue_isempty(att->req_queue))
> +		goto set_handler;
> +
> +	if (!att->pending_ind && !queue_isempty(att->ind_queue))
> +		goto set_handler;
> +
> +	return;

I am not liking this goto logic. Can we just turn this around. If here is nothing to write, then just return. If the if statement gets too complex and it is a reoccurring pattern, then helper functions might be needed.

> +
> +set_handler:
> +	if (!io_set_write_handler(att->io, can_write_data, att, NULL))
> +		return;
> +
> +	att->writer_active = true;
> +}

There might be actually a bug in mgmt->writer_active = true since we never set that. Wonder if it is worth patching or something that never occurs since the logic is a lot simpler with the queues protecting the writer operation anyway.

Anyhow, I wonder why are you not setting the IO destroy function to clear out the writer_active = false. This makes it a lot simpler especially when you return true from the IO callback. That way once the IO write handler is removed it is guaranteed the the write_active is set back to false.

> +
> static void read_watch_destroy(void *user_data)
> {
> 	struct bt_att *att = user_data;
> @@ -370,8 +589,44 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
> 				bt_att_request_func_t callback, void *user_data,
> 				bt_att_destroy_func_t destroy)
> {
> -	/* TODO */
> -	return 0;
> +	struct att_send_op *op;
> +	struct queue *op_queue;
> +
> +	if (!att)
> +		return 0;
> +
> +	op = create_att_send_op(opcode, param, length, att->mtu, callback,
> +							user_data, destroy);
> +	if (!op)
> +		return 0;
> +
> +	if (att->next_send_id < 1)
> +		att->next_send_id = 1;
> +
> +	op->id = att->next_send_id++;
> +
> +	/* Add the op to the correct queue based on its type */
> +	switch (op->op_type) {
> +	case ATT_OP_TYPE_REQ:
> +		op_queue = att->req_queue;
> +		break;
> +	case ATT_OP_TYPE_IND:
> +		op_queue = att->ind_queue;
> +		break;
> +	default:
> +		op_queue = att->write_queue;
> +		break;
> +	}
> +
> +	if (!queue_push_tail(op_queue, op)) {
> +		free(op->pdu);
> +		free(op);
> +		return 0;
> +	}

So I would have done it this way:

	switch (op->op_type) {
	case ATT_OP_TYPE_REQ:
		result = queue_push_tail(att->req_queue, op);
		break;
	...
	}

	if (!result) {
		free(..

> +
> +	wakeup_writer(att);
> +
> +	return op->id;
> }

Regards

Marcel

Re: [PATCH v3 4/4] src/shared/att: Handle incoming response PDUs

[Marcel Holtmann]

Hi Arman,

> This patch adds handling for incoming ATT protocol response PDUs. Basic
> incoming PDU handling logic added to the io read handler.
> ---
> src/shared/att.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/src/shared/att.c b/src/shared/att.c
> index f3ece02..e967273 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -370,6 +370,109 @@ set_handler:
> 	att->writer_active = true;
> }
> 
> +static bool request_complete(struct bt_att *att, uint8_t req_opcode,
> +					uint8_t rsp_opcode, const void *param,
> +					uint16_t len)
> +{
> +	struct att_send_op *op = att->pending_req;
> +
> +	if (!op) {
> +		/* There is no pending request so the response is unexpected.
> +		 * TODO: In such a case, the connection should be dropped
> +		 * entirely. For now, simply ignore the PDU.
> +		 */

is this what the specification say?

> +		wakeup_writer(att);
> +		return false;
> +	}
> +
> +	if (op->opcode != req_opcode)
> +		/* The request opcode corresponding to the received response
> +		 * opcode does not match the currently pending request.
> +		 * TODO: The connection should be dropped in this case, since
> +		 * an invalid response PDU has been received. For now, keep
> +		 * waiting for the correct response.
> +		 */
> +		return false;

In these case { } are a bit of sugar to make it easier on the eyes. The compiler knows what it is, but my brain gets confused too easy ;)

> +
> +	if (op->callback)
> +		op->callback(rsp_opcode, param, len, op->user_data);
> +
> +	destroy_att_send_op(op);
> +	att->pending_req = NULL;
> +
> +	if (!att->destroyed)
> +		wakeup_writer(att);

I would have done this:

	if (att->destroyed)
		return true;

	wakeup_writer(att);

I prefer it this way since it is easier to read when you go through the function. Especially with the att->destroyed cases for the reentrant support. We want to make it clear that we expect that object to be gone.

> +
> +	return true;
> +}
> +
> +static bool handle_error_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
> +								ssize_t pdu_len)
> +{
> +	struct bt_att_error_rsp_param p;
> +	bool result;
> +
> +	if (pdu_len != 5)
> +		return false;
> +
> +	memset(&p, 0, sizeof(p));
> +	p.request_opcode = pdu[1];
> +	p.handle = get_le16(pdu + 2);
> +	p.error_code = pdu[4];
> +
> +	/* Note: avoid a tail call */

I do not get this comment. Explain please.

> +	result = request_complete(att, p.request_opcode, opcode, &p, sizeof(p));
> +	return result;

	return request_complete(..

> +}
> +
> +static bool handle_mtu_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
> +								ssize_t pdu_len)
> +{
> +	struct bt_att_mtu_rsp_param p;
> +	bool result;
> +
> +	if (pdu_len != 3)
> +		return false;
> +
> +	memset(&p, 0, sizeof(p));
> +	p.server_rx_mtu = get_le16(pdu + 1);
> +
> +	/* Note: avoid a tail call */
> +	result = request_complete(att, BT_ATT_OP_MTU_REQ, opcode,
> +								&p, sizeof(p));
> +	return result;

See above.

> +}
> +
> +static void handle_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
> +								ssize_t pdu_len)
> +{
> +	bool success;
> +
> +	switch (opcode) {
> +	case BT_ATT_OP_ERROR_RSP:
> +		success = handle_error_rsp(att, opcode, pdu, pdu_len);
> +		break;
> +	case BT_ATT_OP_MTU_RSP:
> +		success = handle_mtu_rsp(att, opcode, pdu, pdu_len);
> +		break;
> +	default:
> +		success = false;
> +		util_debug(att->debug_callback, att->debug_data,
> +				"Unknown response opcode: 0x%02x", opcode);
> +		break;
> +	}
> +
> +	if (success)
> +		return;
> +
> +	util_debug(att->debug_callback, att->debug_data,
> +			"Failed to handle respone PDU; opcode: 0x%02x", opcode);
> +
> +	if (att->pending_req)
> +		request_complete(att, att->pending_req->opcode,
> +						BT_ATT_OP_ERROR_RSP, NULL, 0);
> +}
> +
> static void read_watch_destroy(void *user_data)
> {
> 	struct bt_att *att = user_data;
> @@ -389,6 +492,7 @@ static void read_watch_destroy(void *user_data)
> static bool can_read_data(struct io *io, void *user_data)
> {
> 	struct bt_att *att = user_data;
> +	uint8_t opcode;
> 	uint8_t *pdu;
> 	ssize_t bytes_read;
> 
> @@ -402,7 +506,19 @@ static bool can_read_data(struct io *io, void *user_data)
> 	if (bytes_read < ATT_MIN_PDU_LEN)
> 		goto done;
> 
> -	/* TODO: Handle different types of PDUs here */
> +	pdu = att->buf;
> +	opcode = pdu[0];
> +
> +	/* Act on the received PDU based on the opcode type */
> +	switch (get_op_type(opcode)) {
> +	case ATT_OP_TYPE_RSP:
> +		handle_rsp(att, opcode, pdu, bytes_read);
> +		break;
> +	default:
> +		util_debug(att->debug_callback, att->debug_data,
> +				"ATT opcode cannot be handled: 0x%02x", opcode);
> +		break;
> +	}
> 
> done:
> 	if (att->destroyed)

Regards

Marcel

Re: [PATCH v3 1/4] src/shared/att: Introduce struct bt_att.

[Arman Uguray]

Hi Marcel,

> I would call it att_request or something like that. Not really important =
since we can change that easily later. Just something to think about. I hav=
e to read through the rest of the patchset and it make more sense then, but=
 right now att_send_op sounds are bit weird.
>

att_send_op sounds fine to me :P I don't like att_request since it
might lead people to think that it represents ATT protocol requests
only, while it represents requests, commands, responses, etc. (i.e. it
represents ATT protocol "ops"). Though if you have a better sounding
name in mind, then I'm all ears.


>> +struct bt_att *bt_att_ref(struct bt_att *att)
>> +{
>> +     /* TODO */
>> +     return NULL;
>> +}
>
> I am normally not a big fan of adding empty functions to just fill them l=
ater. However it has the advantage of me seeing the big picture which is ki=
nda nice.
>
> <snip>

Yeah I did this to make the review easier for you. In the end, if you
don't want to have this as a standalone patch in the tree, feel free
to squash the first two patches while merging. I prefer to leave it as
it is for review purposes.


>> +#define BT_ATT_ERROR_UNLIKELY                                0x0E
>> +#define BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION         0x0F
>> +#define BT_ATT_ERROR_UNSUPPORTED_GROUP_TYPE          0x10
>> +#define BT_ATT_ERROR_INSUFFICIENT_RESOURCES          0x11
>
> these might be better placed in monitor/bt.h and we eventually move that =
include file to some other place. One alternate idea is to use src/shared/a=
tt_types.h and just include it from src/shared/att.h. I am open for suggest=
ions here. What seems better.
>

I like the idea of having a src/shared/att_types.h. All the structures
that are defined in monitor/bt.h are packed and seem like they are
meant to be sent over the wire, whereas the structs defined in att.h
aren't: some of them have fields for variable length arrays and the
PDU encoding is handled internally. So, I don't know if they really
belong there, though I can see why it may make sense to include these
in monitor/bt.h. In the end it's up to you.


>> +void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12=
]);
>
> This might not work. We need the signature resolving key for each directi=
on. And why did you include it as uint8_t[12]. They overall key is 16 octet=
s.
>

Hmm, according to the spec the "Optional authentication signature for
the Attribute Opcode and Attribute Parameters" is 12 octets in length.
See "Core v4.1, Vol 3, Part F, Section 3.3.1, Table 3: Format of
Attribute PDU", and tell me what I'm missing. Or are you suggesting
that we calculate the authentication signature in bt_att? My idea was
that the calculation of the signature and resolving (for incoming
signed requests) would be handled by an upper layer.

> I think this should be bt_att_set_signing_key() and bt_att_resolving_key(=
) and once they are set we can internally drop signed writes that do not fi=
t. And we can also automatically sign if the signed write opcode is used. I=
n addition skip the signed write and just use a normal write if we are alre=
ady encrypted.
>

If the authentication signature is set, I think bt_att should always
try to sign if the opcode has the signed bit set. I think it's cleaner
if bt_att has no knowledge of whether or not the underlying connection
is encrypted; if the higher layer knows that it's encrypted, they
should just pass the correct opcode to bt_att_send.

I agree with all other comments and I'll fix them in the next patch set.

Thanks!
Arman

Re: [PATCH v3 3/4] src/shared/att: Implement write handler and bt_att_send.

[Arman Uguray]

Hi Marcel,

> I see. Just use *((uint8_t *) op->pdu) = op->opcode here. If the pattern of adding opcode plus some extra data repeats, then we might should consider a define or a simpler att_hdr struct.
>
>         struct att_hdr {
>                 uint8_t opcode;
>                 uint8_t data[0];
>         } __packed;
>
> You need to play a little bit with your options here and see what turns into most readable code.
>

I'll just do the simple cast and assign for now. I might add a hdr
struct if the cast becomes too tedious to type.

> Is this possible at all? I am just too lazy to open the spec and check, but you might want to give an example n which opcode has this.
>

WRITE_RSP and HANDLE_VAL_CONF are two examples. Will mention one of
the in the comments.

>> +     /* TODO: If the opcode has the "signed" bit set, make sure that the
>> +      * resulting PDU contains the authentication signature. Return an error,
>> +      * if the provided parameters structure is such that it leaves no room
>> +      * for an authentication signature in the PDU.
>> +      */
>
> I want us to handle this in the background. Just make sure we allocate enough memory.
>

Yeah, for now I'm leaving a TODO. I'll revisit it while implementing
SIGNED_WRITE_CMD.

> I feel a bit uneasy about this queue type thing. Maybe I am just not getting it usefulness. You need to explain this to me a bit in detail.
>

Totally useless. Removed it.

Cheers,
Arman

Re: [PATCH v3 4/4] src/shared/att: Handle incoming response PDUs

[Arman Uguray]

Hi Marcel,

>> +     if (!op) {
>> +             /* There is no pending request so the response is unexpected.
>> +              * TODO: In such a case, the connection should be dropped
>> +              * entirely. For now, simply ignore the PDU.
>> +              */
>
> is this what the specification say?

Actually, I can't seem to find the section in the spec that says this,
so I might be remembering it wrong. What it does say is that if a
request times out (after 30 seconds) then the ATT bearer should be
terminated. Once I add way to notify the upper layers of a request
time out (perhaps as a ATT Error OP with a special BlueZ specific
error code) we will technically be handling this case. I already added
a TODO for implementing timeouts.

>         if (att->destroyed)
>                 return true;
>
>         wakeup_writer(att);
>
> I prefer it this way since it is easier to read when you go through the function. Especially with the att->destroyed cases for the reentrant support. We want to make it clear that we expect that object to be gone.

I got rid of att->destroyed for now.

>> +static bool handle_error_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
>> +                                                             ssize_t pdu_len)
>> +{
>> +     struct bt_att_error_rsp_param p;
>> +     bool result;
>> +
>> +     if (pdu_len != 5)
>> +             return false;
>> +
>> +     memset(&p, 0, sizeof(p));
>> +     p.request_opcode = pdu[1];
>> +     p.handle = get_le16(pdu + 2);
>> +     p.error_code = pdu[4];
>> +
>> +     /* Note: avoid a tail call */
>
> I do not get this comment. Explain please.
>
>> +     result = request_complete(att, p.request_opcode, opcode, &p, sizeof(p));
>> +     return result;
>
>         return request_complete(..
>

Ah yes, if you do a tail call here (i.e. return request_complete(... )
the "struct bt_att_error_rsp_param p" will go out of scope before
request_complete executes, due to tail call optimization, so the
pointer to to "p" that I'm passing down to request_complete will be
invalid. Hence the workaround of calling before returning.

Cheers,
Arman

Re: [PATCH v3 1/4] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Arman,

>> I would call it att_request or something like that. Not really important since we can change that easily later. Just something to think about. I have to read through the rest of the patchset and it make more sense then, but right now att_send_op sounds are bit weird.
>> 
> 
> att_send_op sounds fine to me :P I don't like att_request since it
> might lead people to think that it represents ATT protocol requests
> only, while it represents requests, commands, responses, etc. (i.e. it
> represents ATT protocol "ops"). Though if you have a better sounding
> name in mind, then I'm all ears.

fair enough. Keep it as is. We can optimize the name later if it someone comes up with a better name.

>>> +struct bt_att *bt_att_ref(struct bt_att *att)
>>> +{
>>> +     /* TODO */
>>> +     return NULL;
>>> +}
>> 
>> I am normally not a big fan of adding empty functions to just fill them later. However it has the advantage of me seeing the big picture which is kinda nice.
>> 
>> <snip>
> 
> Yeah I did this to make the review easier for you. In the end, if you
> don't want to have this as a standalone patch in the tree, feel free
> to squash the first two patches while merging. I prefer to leave it as
> it is for review purposes.

Either way is fine with me. I would have it done differently by introducing basics like new, ref/unref, set_debug etc. and then refine it with the send and register one. Not mandatory to this merged however.

>>> +#define BT_ATT_ERROR_UNLIKELY                                0x0E
>>> +#define BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION         0x0F
>>> +#define BT_ATT_ERROR_UNSUPPORTED_GROUP_TYPE          0x10
>>> +#define BT_ATT_ERROR_INSUFFICIENT_RESOURCES          0x11
>> 
>> these might be better placed in monitor/bt.h and we eventually move that include file to some other place. One alternate idea is to use src/shared/att_types.h and just include it from src/shared/att.h. I am open for suggestions here. What seems better.
>> 
> 
> I like the idea of having a src/shared/att_types.h. All the structures
> that are defined in monitor/bt.h are packed and seem like they are
> meant to be sent over the wire, whereas the structs defined in att.h
> aren't: some of them have fields for variable length arrays and the
> PDU encoding is handled internally. So, I don't know if they really
> belong there, though I can see why it may make sense to include these
> in monitor/bt.h. In the end it's up to you.

Start out with src/shared/att_types.h and then we go from there. Moving defines and structs internally round is easy enough that we do not have to worry to much about it at the moment.

For the packed structs, you need them if you use them to access data that comes in from the fd. You need to worry about alignment and the only way to ensure this for wire protocols is packed structs or unaligned access helpers.

>>> +void bt_att_set_auth_signature(struct bt_att *att, uint8_t signature[12]);
>> 
>> This might not work. We need the signature resolving key for each direction. And why did you include it as uint8_t[12]. They overall key is 16 octets.
>> 
> 
> Hmm, according to the spec the "Optional authentication signature for
> the Attribute Opcode and Attribute Parameters" is 12 octets in length.
> See "Core v4.1, Vol 3, Part F, Section 3.3.1, Table 3: Format of
> Attribute PDU", and tell me what I'm missing. Or are you suggesting
> that we calculate the authentication signature in bt_att? My idea was
> that the calculation of the signature and resolving (for incoming
> signed requests) would be handled by an upper layer.

We have to calculate the signature brand new anyway since it is generated over the actual PDU. So yes, lets store the master + slave signature keys in bt_att and handle the signature generation and verification internally.

>> I think this should be bt_att_set_signing_key() and bt_att_resolving_key() and once they are set we can internally drop signed writes that do not fit. And we can also automatically sign if the signed write opcode is used. In addition skip the signed write and just use a normal write if we are already encrypted.
>> 
> 
> If the authentication signature is set, I think bt_att should always
> try to sign if the opcode has the signed bit set. I think it's cleaner
> if bt_att has no knowledge of whether or not the underlying connection
> is encrypted; if the higher layer knows that it's encrypted, they
> should just pass the correct opcode to bt_att_send.

Lets then start it that. However I want to code to automatically also verify writes that have been signed and response with a proper error message if they do not match. However in that case we need to know the underlying security of the link and reject the signed write since it is not allowed.

Regards

Marcel

Re: [PATCH v3 4/4] src/shared/att: Handle incoming response PDUs

[Marcel Holtmann]

Hi Marcel,

>>> +     if (!op) {
>>> +             /* There is no pending request so the response is unexpected.
>>> +              * TODO: In such a case, the connection should be dropped
>>> +              * entirely. For now, simply ignore the PDU.
>>> +              */
>> 
>> is this what the specification say?
> 
> Actually, I can't seem to find the section in the spec that says this,
> so I might be remembering it wrong. What it does say is that if a
> request times out (after 30 seconds) then the ATT bearer should be
> terminated. Once I add way to notify the upper layers of a request
> time out (perhaps as a ATT Error OP with a special BlueZ specific
> error code) we will technically be handling this case. I already added
> a TODO for implementing timeouts.

we need to be able to provide a callback that triggers if we do not receive a response within the 30 seconds and in this case be able to terminate the link. And I think this should be a specific callback indicating to terminate the link and free the bt_att object. Internally we also have to mark the bt_att as invalid now and return false on all future bt_att_send.

>>        if (att->destroyed)
>>                return true;
>> 
>>        wakeup_writer(att);
>> 
>> I prefer it this way since it is easier to read when you go through the function. Especially with the att->destroyed cases for the reentrant support. We want to make it clear that we expect that object to be gone.
> 
> I got rid of att->destroyed for now.
> 
>>> +static bool handle_error_rsp(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
>>> +                                                             ssize_t pdu_len)
>>> +{
>>> +     struct bt_att_error_rsp_param p;
>>> +     bool result;
>>> +
>>> +     if (pdu_len != 5)
>>> +             return false;
>>> +
>>> +     memset(&p, 0, sizeof(p));
>>> +     p.request_opcode = pdu[1];
>>> +     p.handle = get_le16(pdu + 2);
>>> +     p.error_code = pdu[4];
>>> +
>>> +     /* Note: avoid a tail call */
>> 
>> I do not get this comment. Explain please.
>> 
>>> +     result = request_complete(att, p.request_opcode, opcode, &p, sizeof(p));
>>> +     return result;
>> 
>>        return request_complete(..
>> 
> 
> Ah yes, if you do a tail call here (i.e. return request_complete(... )
> the "struct bt_att_error_rsp_param p" will go out of scope before
> request_complete executes, due to tail call optimization, so the
> pointer to to "p" that I'm passing down to request_complete will be
> invalid. Hence the workaround of calling before returning.

The compiler is really this stupid? I can not see how the stack variable (or a pointer to it) can go out of scope here. If this is really a problem, then I would imagine we have a few more of these lingering in the overall code of BlueZ.

Regards

Marcel

Re: [PATCH v3 1/4] src/shared/att: Introduce struct bt_att.

[Arman Uguray]

Hi Marcel,

>> Hmm, according to the spec the "Optional authentication signature for
>> the Attribute Opcode and Attribute Parameters" is 12 octets in length.
>> See "Core v4.1, Vol 3, Part F, Section 3.3.1, Table 3: Format of
>> Attribute PDU", and tell me what I'm missing. Or are you suggesting
>> that we calculate the authentication signature in bt_att? My idea was
>> that the calculation of the signature and resolving (for incoming
>> signed requests) would be handled by an upper layer.
>
> We have to calculate the signature brand new anyway since it is generated over the actual PDU. So yes, lets store the master + slave signature keys in bt_att and handle the signature generation and verification internally.
>

In that case, we need to access the remote and local sign counters as
well, which change dynamically. Should bt_att store and internally
manage that count then? Basically, the first time the CSRK is
generated, the upper layer stores it by calling
bt_att_set_signing_key() and that function internally initializes the
local sign counter to 0? Similarly, bt_att would also keep track of
the remote sign count?

-Arman

Re: [PATCH v3 1/4] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Arman,

>>> Hmm, according to the spec the "Optional authentication signature for
>>> the Attribute Opcode and Attribute Parameters" is 12 octets in length.
>>> See "Core v4.1, Vol 3, Part F, Section 3.3.1, Table 3: Format of
>>> Attribute PDU", and tell me what I'm missing. Or are you suggesting
>>> that we calculate the authentication signature in bt_att? My idea was
>>> that the calculation of the signature and resolving (for incoming
>>> signed requests) would be handled by an upper layer.
>> 
>> We have to calculate the signature brand new anyway since it is generated over the actual PDU. So yes, lets store the master + slave signature keys in bt_att and handle the signature generation and verification internally.
>> 
> 
> In that case, we need to access the remote and local sign counters as
> well, which change dynamically. Should bt_att store and internally
> manage that count then? Basically, the first time the CSRK is
> generated, the upper layer stores it by calling
> bt_att_set_signing_key() and that function internally initializes the
> local sign counter to 0? Similarly, bt_att would also keep track of
> the remote sign count?

so for the ATT channels you will get a new fd for each connection, either from connect() or from accept() and when you create the bt_att via bt_att_new(), then you also need to set they CSRK if you have them available.

But you have a point with the sign counters. I think that for the lifetime of the bt_att, the sign counter should be managed internally. However it might be a good idea to provide the current sign counter when setting the signature key. In addition if we want to make the sign counter persistent, we need a way to read the current one so we can store it.

Regards

Marcel