[PATCH v2 00/10] bt_att initial implementation

[PATCH v2 00/10] bt_att initial implementation

[Arman Uguray]

v1:
   - Updated the copyright notice in new files to include Google Inc. Retained
   the Intel copyright in src/shared/att.c and unit/test-att.c which take a lot
   of code from src/shared/mgmt.c and unit/test-mgmt.c respectively.

   - Removed ATT_ERROR_IO from src/shared/att as 0x80 is a legitimate
   application error that varies among GATT profiles. On I/O errors, the code
   now invokes the callback with ATT_OP_ERROR_RESP with NULL parameters.

v2:
   - Renamed all opcode macros to look like BT_ATT_OP_* instead of ATT_OP_*.
   - Fixed bt_att_set_mtu to resize the internal buffer.
   - Renamed bt_att_send to bt_att_send_sequential, for sequential request/
   response and indication/confirmation flows. The non-sequential version will
   be called bt_att_send and not take in any callbacks.

Arman Uguray (10):
  src/shared/att: Introduce struct bt_att.
  unit/test-att: Add unit tests for src/shared/att.
  tools/btatt: Add command-line tool for ATT protocol testing.
  tools/btatt: Add "exchange-mtu" command.
  src/shared/att: Add "Find Information" request and response.
  unit/test-att: Add unit test for "Find Information" request/response.
  tools/btatt: Add "find-information" command.
  src/shared/att: Add "Find By Type Value" request and response.
  unit/test-att: Add unit test for "Find By Type Value"
    request/response.
  tools/btatt: Add "find-by-type-value" command.

 Makefile.am      |  12 +-
 Makefile.tools   |  10 +-
 src/shared/att.c | 694 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/att.h | 131 +++++++++++
 tools/btatt.c    | 629 +++++++++++++++++++++++++++++++++++++++++++++++++
 unit/test-att.c  | 474 +++++++++++++++++++++++++++++++++++++
 6 files changed, 1948 insertions(+), 2 deletions(-)
 create mode 100644 src/shared/att.c
 create mode 100644 src/shared/att.h
 create mode 100644 tools/btatt.c
 create mode 100644 unit/test-att.c

-- 
1.8.3.2

[PATCH v2 01/10] 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. The initial patch implements the Exchange MTU request &
response and the Error Response. Currently, only requests that are
initiated locally are supported.
---
 Makefile.am      |   3 +-
 src/shared/att.c | 531 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/att.h | 101 +++++++++++
 3 files changed, 634 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 f96c700..0c3424f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -155,7 +155,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..8b10c30
--- /dev/null
+++ b/src/shared/att.c
@@ -0,0 +1,531 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *  Copyright (C) 2014  Intel Corporation.
+ *
+ *
+ *  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 <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+
+#include "src/shared/io.h"
+#include "src/shared/queue.h"
+#include "src/shared/util.h"
+#include "src/shared/att.h"
+
+#define ATT_MAX_VALUE_LEN		512
+#define ATT_DEFAULT_LE_MTU		23
+#define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
+
+struct bt_att {
+	int ref_count;
+	int fd;
+	bool close_on_unref;
+	struct io *io;
+	bool writer_active;
+	struct queue *request_queue;
+	struct queue *pending_list;
+	unsigned int next_request_id;
+	bool destroyed;
+	void *buf;
+	uint16_t len;
+	bt_att_debug_func_t debug_callback;
+	bt_att_destroy_func_t debug_destroy;
+	void *debug_data;
+};
+
+struct bt_att_request {
+	unsigned int id;
+	uint8_t opcode;
+	void *pdu;
+	uint16_t len;
+	bt_att_request_func_t callback;
+	bt_att_destroy_func_t destroy;
+	void *user_data;
+};
+
+static void destroy_request(void *data)
+{
+	struct bt_att_request *request = data;
+
+	if (request->destroy)
+		request->destroy(request->user_data);
+
+	free(request->pdu);
+	free(request);
+}
+
+static bool match_request_opcode(const void *a, const void *b)
+{
+	const struct bt_att_request *request = a;
+	const uint8_t opcode = PTR_TO_UINT(b);
+
+	return request->opcode == opcode;
+}
+
+static void write_watch_destroy(void *user_data)
+{
+	struct bt_att *att = user_data;
+
+	att->writer_active = false;
+}
+
+static bool can_write_data(struct io *io, void *user_data)
+{
+	struct bt_att *att = user_data;
+	struct bt_att_request *request;
+	ssize_t bytes_written;
+
+	if (!queue_isempty(att->pending_list))
+		return false;
+
+	request = queue_pop_head(att->request_queue);
+	if (!request)
+		return false;
+
+	bytes_written = write(att->fd, request->pdu, request->len);
+	if (bytes_written < 0) {
+		util_debug(att->debug_callback, att->debug_data,
+					"write failed: %s", strerror(errno));
+		if (request->callback)
+			request->callback(BT_ATT_OP_ERROR_RESP, NULL, 0,
+							request->user_data);
+
+		destroy_request(request);
+		return true;
+	}
+
+	util_debug(att->debug_callback, att->debug_data,
+					"ATT command 0x%02x", request->opcode);
+
+	util_hexdump('<', request->pdu, bytes_written,
+					att->debug_callback, att->debug_data);
+
+	queue_push_tail(att->pending_list, request);
+
+	return false;
+}
+
+static void wakeup_writer(struct bt_att *att)
+{
+	if (!queue_isempty(att->pending_list))
+		return;
+
+	if (att->writer_active)
+		return;
+
+	io_set_write_handler(att->io, can_write_data, att, write_watch_destroy);
+}
+
+static void request_complete(struct bt_att *att, uint8_t req_opcode,
+					uint8_t rsp_opcode, const void *param,
+					uint16_t length)
+{
+	struct bt_att_request *request;
+
+	request = queue_remove_if(att->pending_list,
+						match_request_opcode,
+						UINT_TO_PTR(req_opcode));
+
+	if (request) {
+		if (request->callback)
+			request->callback(rsp_opcode, param, length,
+							request->user_data);
+
+		destroy_request(request);
+	}
+
+	if (att->destroyed)
+		return;
+
+	wakeup_writer(att);
+}
+
+static bool handle_error_rsp(struct bt_att *att, const uint8_t *pdu,
+							uint16_t pdu_length)
+{
+	struct bt_att_error_rsp_param param;
+
+	if (pdu_length != 5)
+		return false;
+
+	memset(&param, 0, sizeof(param));
+
+	param.request_opcode = pdu[1];
+	param.handle = get_le16(pdu + 2);
+	param.error_code = pdu[4];
+
+	request_complete(att, param.request_opcode, pdu[0],
+							&param, sizeof(param));
+
+	return true;
+}
+
+static bool handle_exchange_mtu_rsp(struct bt_att *att, const uint8_t *pdu,
+							uint16_t pdu_length)
+{
+	struct bt_att_exchange_mtu_rsp_param param;
+
+	if (pdu_length != 3)
+		return false;
+
+	param.server_rx_mtu = get_le16(pdu + 1);
+
+	request_complete(att, BT_ATT_OP_EXCHANGE_MTU_REQ, pdu[0],
+							&param, sizeof(param));
+
+	return true;
+}
+
+static bool handle_response(struct bt_att *att, const uint8_t *pdu,
+							uint16_t pdu_length)
+{
+	uint8_t opcode = pdu[0];
+
+	if (opcode == BT_ATT_OP_ERROR_RESP)
+		return handle_error_rsp(att, pdu, pdu_length);
+
+	if (opcode == BT_ATT_OP_EXCHANGE_MTU_RESP)
+		return handle_exchange_mtu_rsp(att, pdu, pdu_length);
+
+	return false;
+}
+
+static void read_watch_destroy(void *user_data)
+{
+	struct bt_att *att = user_data;
+
+	if (att->destroyed) {
+		queue_destroy(att->pending_list, NULL);
+		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->len);
+	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)
+		return true;
+
+	pdu = att->buf;
+
+	/* The opcode is either a response to a pending request, a new request
+	 * from a client, or a notification/indication. Handle them separately
+	 * here.
+	 */
+	/* TODO: Handle other types of data here. */
+	handle_response(att, pdu, bytes_read);
+
+	if (att->destroyed)
+		return false;
+
+	return true;
+}
+
+struct bt_att *bt_att_new(int fd)
+{
+	struct bt_att *att;
+
+	if (fd < 0)
+		return NULL;
+
+	att = new0(struct bt_att, 1);
+	if (!att)
+		return NULL;
+
+	att->fd = fd;
+	att->close_on_unref = false;
+
+	att->len = ATT_DEFAULT_LE_MTU;
+	att->buf = malloc(att->len);
+	if (!att->buf)
+		goto fail;
+
+	att->io = io_new(fd);
+	if (!att->io)
+		goto fail;
+
+	att->request_queue = queue_new();
+	if (!att->request_queue)
+		goto fail;
+
+	att->pending_list = queue_new();
+	if (!att->pending_list)
+		goto fail;
+
+	if (!io_set_read_handler(att->io, can_read_data,
+						att, read_watch_destroy))
+		goto fail;
+
+	att->writer_active = false;
+
+	return bt_att_ref(att);
+
+fail:
+	queue_destroy(att->pending_list, NULL);
+	queue_destroy(att->request_queue, NULL);
+	io_destroy(att->io);
+	free(att->buf);
+	free(att);
+
+	return NULL;
+}
+
+struct bt_att *bt_att_ref(struct bt_att *att)
+{
+	if (!att)
+		return NULL;
+
+	__sync_fetch_and_add(&att->ref_count, 1);
+
+	return att;
+}
+
+void bt_att_unref(struct bt_att *att)
+{
+	if (!att)
+		return;
+
+	if (__sync_sub_and_fetch(&att->ref_count, 1))
+		return;
+
+	bt_att_cancel_all(att);
+
+	queue_destroy(att->request_queue, NULL);
+
+	io_set_write_handler(att->io, NULL, NULL, NULL);
+	io_set_read_handler(att->io, NULL, NULL, 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;
+
+	att->destroyed = 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)
+{
+	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;
+}
+
+bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close)
+{
+	if (!att)
+		return false;
+
+	att->close_on_unref = do_close;
+
+	return true;
+}
+
+uint16_t bt_att_get_mtu(struct bt_att *att)
+{
+	if (!att)
+		return 0;
+
+	return att->len;
+}
+
+bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu)
+{
+	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->len = mtu;
+	att->buf = buf;
+
+	return true;
+}
+
+static bool encode_mtu_req(const void *param, uint16_t length, void *buf,
+					uint16_t mtu, uint16_t *pdu_length)
+{
+	const struct bt_att_exchange_mtu_req_param *cp = param;
+	const uint16_t len = 3;
+
+	if (length != sizeof(struct bt_att_exchange_mtu_req_param))
+		return false;
+
+	if (len > mtu)
+		return false;
+
+	put_le16(cp->client_rx_mtu, buf);
+	*pdu_length = len;
+
+	return true;
+}
+
+static bool encode_pdu(uint8_t opcode, const void *param, uint16_t length,
+				uint16_t mtu, void *pdu, uint16_t *pdu_length)
+{
+	uint8_t *bytes = pdu;
+
+	if (!bytes)
+		return false;
+
+	bytes[0] = opcode;
+
+	/* The length of the PDU depends on the specific ATT method. Special
+	 * case the operations with variable-length PDU's here.
+	 */
+	if (length == 0) {
+		*pdu_length = 1;
+		return true;
+	}
+
+	if (!param)
+		return false;
+
+	if (opcode == BT_ATT_OP_EXCHANGE_MTU_REQ) {
+		return encode_mtu_req(param, length, bytes + 1,
+							mtu, pdu_length);
+	}
+
+	return false;
+}
+
+static struct bt_att_request *create_request(uint8_t opcode, const void *param,
+				uint16_t length, void *buf, uint16_t mtu,
+				bt_att_request_func_t callback, void *user_data,
+				bt_att_destroy_func_t destroy)
+{
+	struct bt_att_request *request;
+
+	if (!opcode)
+		return NULL;
+
+	if (length > 0 && !param)
+		return NULL;
+
+	request = new0(struct bt_att_request, 1);
+	if (!request)
+		return NULL;
+
+	if (!encode_pdu(opcode, param, length, mtu, buf, &request->len)) {
+		free(request);
+		return NULL;
+	}
+
+	/* request->len is at least 1 due to the opcode */
+	request->pdu = malloc(request->len);
+	if (!request->pdu) {
+		free(request);
+		return NULL;
+	}
+
+	if (request->len > 0)
+		memcpy(request->pdu, buf, request->len);
+
+	request->opcode = opcode;
+	request->callback = callback;
+	request->destroy = destroy;
+	request->user_data = user_data;
+
+	return request;
+}
+
+unsigned int bt_att_send_sequential(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)
+{
+	struct bt_att_request *request;
+
+	if (!att)
+		return 0;
+
+	request = create_request(opcode, param, length, att->buf, att->len,
+						callback, user_data, destroy);
+
+	if (!request)
+		return 0;
+
+	if (att->next_request_id < 1)
+		att->next_request_id = 1;
+
+	request->id = att->next_request_id++;
+
+	if (!queue_push_tail(att->request_queue, request)) {
+		free(request->pdu);
+		free(request);
+		return 0;
+	}
+
+	wakeup_writer(att);
+
+	return request->id;
+}
+
+bool bt_att_cancel_all(struct bt_att *att)
+{
+	if (!att)
+		return false;
+
+	queue_remove_all(att->pending_list, NULL, NULL, destroy_request);
+	queue_remove_all(att->request_queue, NULL, NULL, destroy_request);
+
+	return true;
+}
diff --git a/src/shared/att.h b/src/shared/att.h
new file mode 100644
index 0000000..8fe8805
--- /dev/null
+++ b/src/shared/att.h
@@ -0,0 +1,101 @@
+/*
+ *
+ *  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_RESP	      		0x01
+struct bt_att_error_rsp_param {
+	uint8_t request_opcode;
+	uint16_t handle;
+	uint8_t	error_code;
+};
+
+/* Exchange MTU */
+#define BT_ATT_OP_EXCHANGE_MTU_REQ		0x02
+struct bt_att_exchange_mtu_req_param {
+	uint16_t client_rx_mtu;
+};
+
+#define BT_ATT_OP_EXCHANGE_MTU_RESP		0x03
+struct bt_att_exchange_mtu_rsp_param {
+	uint16_t server_rx_mtu;
+};
+
+/* 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
+
+typedef void (*bt_att_destroy_func_t)(void *user_data);
+
+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);
+
+typedef void (*bt_att_debug_func_t)(const char *str, 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);
+
+bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
+
+uint16_t bt_att_get_mtu(struct bt_att *att);
+bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu);
+
+typedef void (*bt_att_request_func_t)(uint8_t opcode, const void *param,
+					uint16_t length, void *user_data);
+
+/*
+ * bt_att_send_sequential is used to send ATT protocol requests and
+ * indications which invoke a response or confirmation from the remote device.
+ * All commands issued through this function will be queued and processed
+ * sequentially as a response/confirmation for each one is received from the
+ * remote device. Note that a client implementation would use this to only send
+ * ATT protocol requests whereas a server implementation would use this to only
+ * send ATT protocol indications.
+ */
+unsigned int bt_att_send_sequential(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);
-- 
1.8.3.2

Re: [PATCH v2 01/10] 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. The initial patch implements the Exchange MTU request &
> response and the Error Response. Currently, only requests that are
> initiated locally are supported.
> ---
> Makefile.am      |   3 +-
> src/shared/att.c | 531 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/att.h | 101 +++++++++++
> 3 files changed, 634 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 f96c700..0c3424f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -155,7 +155,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..8b10c30
> --- /dev/null
> +++ b/src/shared/att.c
> @@ -0,0 +1,531 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *  Copyright (C) 2014  Intel Corporation.
> + *
> + *
> + *  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 <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include "src/shared/io.h"
> +#include "src/shared/queue.h"
> +#include "src/shared/util.h"
> +#include "src/shared/att.h"
> +
> +#define ATT_MAX_VALUE_LEN		512
> +#define ATT_DEFAULT_LE_MTU		23
> +#define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
> +
> +struct bt_att {
> +	int ref_count;
> +	int fd;
> +	bool close_on_unref;
> +	struct io *io;
> +	bool writer_active;
> +	struct queue *request_queue;
> +	struct queue *pending_list;
> +	unsigned int next_request_id;
> +	bool destroyed;
> +	void *buf;
> +	uint16_t len;
> +	bt_att_debug_func_t debug_callback;
> +	bt_att_destroy_func_t debug_destroy;
> +	void *debug_data;
> +};
> +
> +struct bt_att_request {
> +	unsigned int id;
> +	uint8_t opcode;
> +	void *pdu;
> +	uint16_t len;
> +	bt_att_request_func_t callback;
> +	bt_att_destroy_func_t destroy;
> +	void *user_data;
> +};
> +
> +static void destroy_request(void *data)
> +{
> +	struct bt_att_request *request = data;
> +
> +	if (request->destroy)
> +		request->destroy(request->user_data);
> +
> +	free(request->pdu);
> +	free(request);
> +}
> +
> +static bool match_request_opcode(const void *a, const void *b)
> +{
> +	const struct bt_att_request *request = a;
> +	const uint8_t opcode = PTR_TO_UINT(b);
> +
> +	return request->opcode == opcode;
> +}
> +
> +static void write_watch_destroy(void *user_data)
> +{
> +	struct bt_att *att = user_data;
> +
> +	att->writer_active = false;
> +}
> +
> +static bool can_write_data(struct io *io, void *user_data)
> +{
> +	struct bt_att *att = user_data;
> +	struct bt_att_request *request;
> +	ssize_t bytes_written;
> +
> +	if (!queue_isempty(att->pending_list))
> +		return false;
> +
> +	request = queue_pop_head(att->request_queue);
> +	if (!request)
> +		return false;
> +
> +	bytes_written = write(att->fd, request->pdu, request->len);
> +	if (bytes_written < 0) {
> +		util_debug(att->debug_callback, att->debug_data,
> +					"write failed: %s", strerror(errno));

a few projects started to use %m these days. Might be a good idea to just go with it as well.

> +		if (request->callback)
> +			request->callback(BT_ATT_OP_ERROR_RESP, NULL, 0,
> +							request->user_data);
> +
> +		destroy_request(request);
> +		return true;
> +	}
> +
> +	util_debug(att->debug_callback, att->debug_data,
> +					"ATT command 0x%02x", request->opcode);
> +
> +	util_hexdump('<', request->pdu, bytes_written,
> +					att->debug_callback, att->debug_data);
> +
> +	queue_push_tail(att->pending_list, request);
> +
> +	return false;
> +}
> +
> +static void wakeup_writer(struct bt_att *att)
> +{
> +	if (!queue_isempty(att->pending_list))
> +		return;
> +
> +	if (att->writer_active)
> +		return;
> +
> +	io_set_write_handler(att->io, can_write_data, att, write_watch_destroy);
> +}
> +
> +static void request_complete(struct bt_att *att, uint8_t req_opcode,
> +					uint8_t rsp_opcode, const void *param,
> +					uint16_t length)
> +{
> +	struct bt_att_request *request;
> +
> +	request = queue_remove_if(att->pending_list,
> +						match_request_opcode,
> +						UINT_TO_PTR(req_opcode));

Please do not have these extra empty lines between the function and the if checking if the result is valid. If we accidentally have this in some other code, please send patches to fix it since that is an oversight.

> +
> +	if (request) {
> +		if (request->callback)
> +			request->callback(rsp_opcode, param, length,
> +							request->user_data);
> +
> +		destroy_request(request);
> +	}
> +
> +	if (att->destroyed)
> +		return;
> +
> +	wakeup_writer(att);
> +}
> +
> +static bool handle_error_rsp(struct bt_att *att, const uint8_t *pdu,
> +							uint16_t pdu_length)
> +{
> +	struct bt_att_error_rsp_param param;
> +
> +	if (pdu_length != 5)
> +		return false;
> +
> +	memset(&param, 0, sizeof(param));
> +
> +	param.request_opcode = pdu[1];
> +	param.handle = get_le16(pdu + 2);
> +	param.error_code = pdu[4];
> +
> +	request_complete(att, param.request_opcode, pdu[0],
> +							&param, sizeof(param));
> +
> +	return true;
> +}
> +
> +static bool handle_exchange_mtu_rsp(struct bt_att *att, const uint8_t *pdu,
> +							uint16_t pdu_length)
> +{
> +	struct bt_att_exchange_mtu_rsp_param param;
> +
> +	if (pdu_length != 3)
> +		return false;
> +
> +	param.server_rx_mtu = get_le16(pdu + 1);
> +
> +	request_complete(att, BT_ATT_OP_EXCHANGE_MTU_REQ, pdu[0],
> +							&param, sizeof(param));
> +
> +	return true;
> +}
> +
> +static bool handle_response(struct bt_att *att, const uint8_t *pdu,
> +							uint16_t pdu_length)
> +{
> +	uint8_t opcode = pdu[0];
> +
> +	if (opcode == BT_ATT_OP_ERROR_RESP)
> +		return handle_error_rsp(att, pdu, pdu_length);
> +
> +	if (opcode == BT_ATT_OP_EXCHANGE_MTU_RESP)
> +		return handle_exchange_mtu_rsp(att, pdu, pdu_length);
> +
> +	return false;
> +}
> +
> +static void read_watch_destroy(void *user_data)
> +{
> +	struct bt_att *att = user_data;
> +
> +	if (att->destroyed) {
> +		queue_destroy(att->pending_list, NULL);
> +		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->len);
> +	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)
> +		return true;
> +
> +	pdu = att->buf;
> +
> +	/* The opcode is either a response to a pending request, a new request
> +	 * from a client, or a notification/indication. Handle them separately
> +	 * here.
> +	 */
> +	/* TODO: Handle other types of data here. */
> +	handle_response(att, pdu, bytes_read);
> +
> +	if (att->destroyed)
> +		return false;
> +
> +	return true;
> +}
> +
> +struct bt_att *bt_att_new(int fd)
> +{
> +	struct bt_att *att;
> +
> +	if (fd < 0)
> +		return NULL;
> +
> +	att = new0(struct bt_att, 1);
> +	if (!att)
> +		return NULL;
> +
> +	att->fd = fd;
> +	att->close_on_unref = false;
> +
> +	att->len = ATT_DEFAULT_LE_MTU;
> +	att->buf = malloc(att->len);
> +	if (!att->buf)
> +		goto fail;
> +
> +	att->io = io_new(fd);
> +	if (!att->io)
> +		goto fail;
> +
> +	att->request_queue = queue_new();
> +	if (!att->request_queue)
> +		goto fail;
> +
> +	att->pending_list = queue_new();
> +	if (!att->pending_list)
> +		goto fail;
> +
> +	if (!io_set_read_handler(att->io, can_read_data,
> +						att, read_watch_destroy))
> +		goto fail;
> +
> +	att->writer_active = false;
> +
> +	return bt_att_ref(att);
> +
> +fail:
> +	queue_destroy(att->pending_list, NULL);
> +	queue_destroy(att->request_queue, NULL);
> +	io_destroy(att->io);
> +	free(att->buf);
> +	free(att);
> +
> +	return NULL;
> +}
> +
> +struct bt_att *bt_att_ref(struct bt_att *att)
> +{
> +	if (!att)
> +		return NULL;
> +
> +	__sync_fetch_and_add(&att->ref_count, 1);
> +
> +	return att;
> +}
> +
> +void bt_att_unref(struct bt_att *att)
> +{
> +	if (!att)
> +		return;
> +
> +	if (__sync_sub_and_fetch(&att->ref_count, 1))
> +		return;
> +
> +	bt_att_cancel_all(att);
> +
> +	queue_destroy(att->request_queue, NULL);
> +
> +	io_set_write_handler(att->io, NULL, NULL, NULL);
> +	io_set_read_handler(att->io, NULL, NULL, 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;
> +
> +	att->destroyed = 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)
> +{
> +	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;
> +}
> +
> +bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close)
> +{
> +	if (!att)
> +		return false;
> +
> +	att->close_on_unref = do_close;
> +
> +	return true;
> +}
> +
> +uint16_t bt_att_get_mtu(struct bt_att *att)
> +{
> +	if (!att)
> +		return 0;
> +
> +	return att->len;
> +}
> +
> +bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu)
> +{
> +	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->len = mtu;
> +	att->buf = buf;
> +
> +	return true;
> +}
> +
> +static bool encode_mtu_req(const void *param, uint16_t length, void *buf,
> +					uint16_t mtu, uint16_t *pdu_length)
> +{
> +	const struct bt_att_exchange_mtu_req_param *cp = param;
> +	const uint16_t len = 3;
> +
> +	if (length != sizeof(struct bt_att_exchange_mtu_req_param))
> +		return false;
> +
> +	if (len > mtu)
> +		return false;
> +
> +	put_le16(cp->client_rx_mtu, buf);
> +	*pdu_length = len;
> +
> +	return true;
> +}
> +
> +static bool encode_pdu(uint8_t opcode, const void *param, uint16_t length,
> +				uint16_t mtu, void *pdu, uint16_t *pdu_length)
> +{
> +	uint8_t *bytes = pdu;
> +
> +	if (!bytes)
> +		return false;
> +
> +	bytes[0] = opcode;
> +
> +	/* The length of the PDU depends on the specific ATT method. Special
> +	 * case the operations with variable-length PDU's here.
> +	 */
> +	if (length == 0) {
> +		*pdu_length = 1;
> +		return true;
> +	}
> +
> +	if (!param)
> +		return false;
> +
> +	if (opcode == BT_ATT_OP_EXCHANGE_MTU_REQ) {
> +		return encode_mtu_req(param, length, bytes + 1,
> +							mtu, pdu_length);
> +	}
> +
> +	return false;
> +}
> +
> +static struct bt_att_request *create_request(uint8_t opcode, const void *param,
> +				uint16_t length, void *buf, uint16_t mtu,
> +				bt_att_request_func_t callback, void *user_data,
> +				bt_att_destroy_func_t destroy)
> +{
> +	struct bt_att_request *request;
> +
> +	if (!opcode)
> +		return NULL;
> +
> +	if (length > 0 && !param)
> +		return NULL;
> +
> +	request = new0(struct bt_att_request, 1);
> +	if (!request)
> +		return NULL;
> +
> +	if (!encode_pdu(opcode, param, length, mtu, buf, &request->len)) {
> +		free(request);
> +		return NULL;
> +	}
> +
> +	/* request->len is at least 1 due to the opcode */
> +	request->pdu = malloc(request->len);
> +	if (!request->pdu) {
> +		free(request);
> +		return NULL;
> +	}
> +
> +	if (request->len > 0)
> +		memcpy(request->pdu, buf, request->len);
> +
> +	request->opcode = opcode;
> +	request->callback = callback;
> +	request->destroy = destroy;
> +	request->user_data = user_data;
> +
> +	return request;
> +}
> +
> +unsigned int bt_att_send_sequential(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)
> +{
> +	struct bt_att_request *request;
> +
> +	if (!att)
> +		return 0;
> +
> +	request = create_request(opcode, param, length, att->buf, att->len,
> +						callback, user_data, destroy);
> +
> +	if (!request)
> +		return 0;
> +
> +	if (att->next_request_id < 1)
> +		att->next_request_id = 1;
> +
> +	request->id = att->next_request_id++;
> +
> +	if (!queue_push_tail(att->request_queue, request)) {
> +		free(request->pdu);
> +		free(request);
> +		return 0;
> +	}
> +
> +	wakeup_writer(att);
> +
> +	return request->id;
> +}
> +
> +bool bt_att_cancel_all(struct bt_att *att)
> +{
> +	if (!att)
> +		return false;
> +
> +	queue_remove_all(att->pending_list, NULL, NULL, destroy_request);
> +	queue_remove_all(att->request_queue, NULL, NULL, destroy_request);
> +
> +	return true;
> +}
> diff --git a/src/shared/att.h b/src/shared/att.h
> new file mode 100644
> index 0000000..8fe8805
> --- /dev/null
> +++ b/src/shared/att.h
> @@ -0,0 +1,101 @@
> +/*
> + *
> + *  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_RESP	      		0x01
> +struct bt_att_error_rsp_param {
> +	uint8_t request_opcode;
> +	uint16_t handle;
> +	uint8_t	error_code;
> +};
> +
> +/* Exchange MTU */
> +#define BT_ATT_OP_EXCHANGE_MTU_REQ		0x02
> +struct bt_att_exchange_mtu_req_param {
> +	uint16_t client_rx_mtu;
> +};
> +
> +#define BT_ATT_OP_EXCHANGE_MTU_RESP		0x03
> +struct bt_att_exchange_mtu_rsp_param {
> +	uint16_t server_rx_mtu;
> +};
> +
> +/* 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
> +
> +typedef void (*bt_att_destroy_func_t)(void *user_data);
> +
> +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);
> +
> +typedef void (*bt_att_debug_func_t)(const char *str, 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);
> +
> +bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
> +
> +uint16_t bt_att_get_mtu(struct bt_att *att);
> +bool bt_att_set_mtu(struct bt_att *att, uint16_t mtu);
> +
> +typedef void (*bt_att_request_func_t)(uint8_t opcode, const void *param,
> +					uint16_t length, void *user_data);
> +
> +/*
> + * bt_att_send_sequential is used to send ATT protocol requests and
> + * indications which invoke a response or confirmation from the remote device.
> + * All commands issued through this function will be queued and processed
> + * sequentially as a response/confirmation for each one is received from the
> + * remote device. Note that a client implementation would use this to only send
> + * ATT protocol requests whereas a server implementation would use this to only
> + * send ATT protocol indications.
> + */
> +unsigned int bt_att_send_sequential(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);

That is the implied behavior for _send functions. So I would not use _send_sequential here at all.

I am still not sure that we should be doing it like this. We tried this with gattrib and it did not turn out that great. I really like the approach of I send this command and expect this PDU back or an error. The reason here is that in theory you can get interleaved commands/response on the same channel.

So this is what you would normally expect:

	-> command
	<- response

Straight forward and simple, right. However nothing is stopping the other side from issues the same command in the other direction.

	-> command
	<- command
	<- response
	-> response

And this is not to mention notifications and confirmations that can happen as well.

I am thinking out loud here. What might help is a table that classifies which opcode is a command, response, notification or confirmation and based on that we just do the right thing.

Unfortunately we need to get this one right and I know that rebasing patches can be a pain. So if you just want to send the initial 01/01 patch for this then that is fine with me as well. I think some units tests with this overlapping bi-directional communication would be amazingly great to have. I do wonder if other OSes handle this well.

Regards

Marcel

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Arman Uguray]

Hi Marcel,

> > +unsigned int bt_att_send_sequential(struct bt_att *att, uint8_t opcode=
,
> > +                             const void *param, uint16_t length,
> > +                             bt_att_request_func_t callback, void *use=
r_data,
> > +                             bt_att_destroy_func_t destroy);
>
> That is the implied behavior for _send functions. So I would not use _sen=
d_sequential here at all.

This mainly came about after the discussion with Luiz on patch set v1.
My idea was to use bt_att_send for non-sequential ops and
bt_att_send_sequential for the opposite. I could rename these to
bt_att_send_without_rsp and bt_att_send, respectively.

> I am still not sure that we should be doing it like this. We tried this w=
ith gattrib and it did not turn out that great. I really like the approach =
of I send this command and expect this PDU back or an error. The reason her=
e is that in theory you can get interleaved commands/response on the same c=
hannel.

I understand the reasoning behind this; then again it introduces
potential error cases such as expecting the wrong PDU back, e.g.
expecting a "Write Response" while sending a "Read Request". In the
end, I don't see much value in building the API that way, since in
ATT, all sequential requests have a 1-to-1 mapping to a sequential
response. e.g., if we receive a "Read Response", the currently pending
request absolutely has to be a "Read Request", otherwise we are
supposed to ignore the PDU (or drop the connection entirely, according
to the spec).

>
> So this is what you would normally expect:
>
>         -> command
>         <- response
>
> Straight forward and simple, right. However nothing is stopping the other=
 side from issues the same command in the other direction.
>
>         -> command
>         <- command
>         <- response
>         -> response
>
> And this is not to mention notifications and confirmations that can happe=
n as well.
>
> I am thinking out loud here. What might help is a table that classifies w=
hich opcode is a command, response, notification or confirmation and based =
on that we just do the right thing.

Well, I actually did write a similar unit test on my local branch
using my current implementation and these work as expected. The way I
structured the code actually does (or will do in upcoming patches)
exactly what you say, though instead of using tables, each "handler"
function explicitly checks the opcode to see if it falls into its
expected category (e.g. handle_response, handle_notification, etc.).

ATT is a really simple protocol in this regard.

>
> Unfortunately we need to get this one right and I know that rebasing patc=
hes can be a pain. So if you just want to send the initial 01/01 patch for =
this then that is fine with me as well. I think some units tests with this =
overlapping bi-directional communication would be amazingly great to have. =
I do wonder if other OSes handle this well.

Absolutely. Let me know if this has convinced you. So my idea is to
have a bt_att_send and bt_att_send_without_rsp. I originally had a
single bt_att_send for all opcodes, but Luiz brought up the fact that
having a one for all function leads to some inconsistencies, such as
cases where a callback is passed along with an opcode that doesn't
expect a response. Personally I don't think this is such a problem as
long as the API is well documented.

Luiz's suggestion was to actually have a separate function per ATT
opcode. In that approach we would remove the opcode + parameter
structure design entirely and have the individual functions accept the
right parameters. So, no bt_att_send, but bt_att_send_read_request,
etc. Do you think this would be a cleaner approach in the end?

Cheers,
-Arman

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Arman,

>>> +unsigned int bt_att_send_sequential(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);
>> 
>> That is the implied behavior for _send functions. So I would not use _send_sequential here at all.
> 
> This mainly came about after the discussion with Luiz on patch set v1.
> My idea was to use bt_att_send for non-sequential ops and
> bt_att_send_sequential for the opposite. I could rename these to
> bt_att_send_without_rsp and bt_att_send, respectively.

I am getting the feeling that just having bt_att_send only seems much cleaner. And if you give an opcode that does not expect a response, then you have to give an empty callback function. If not you get an error. I have to scratch my head a bit against this.

>> I am still not sure that we should be doing it like this. We tried this with gattrib and it did not turn out that great. I really like the approach of I send this command and expect this PDU back or an error. The reason here is that in theory you can get interleaved commands/response on the same channel.
> 
> I understand the reasoning behind this; then again it introduces
> potential error cases such as expecting the wrong PDU back, e.g.
> expecting a "Write Response" while sending a "Read Request". In the
> end, I don't see much value in building the API that way, since in
> ATT, all sequential requests have a 1-to-1 mapping to a sequential
> response. e.g., if we receive a "Read Response", the currently pending
> request absolutely has to be a "Read Request", otherwise we are
> supposed to ignore the PDU (or drop the connection entirely, according
> to the spec).
> 
>> 
>> So this is what you would normally expect:
>> 
>>        -> command
>>        <- response
>> 
>> Straight forward and simple, right. However nothing is stopping the other side from issues the same command in the other direction.
>> 
>>        -> command
>>        <- command
>>        <- response
>>        -> response
>> 
>> And this is not to mention notifications and confirmations that can happen as well.
>> 
>> I am thinking out loud here. What might help is a table that classifies which opcode is a command, response, notification or confirmation and based on that we just do the right thing.
> 
> Well, I actually did write a similar unit test on my local branch
> using my current implementation and these work as expected. The way I
> structured the code actually does (or will do in upcoming patches)
> exactly what you say, though instead of using tables, each "handler"
> function explicitly checks the opcode to see if it falls into its
> expected category (e.g. handle_response, handle_notification, etc.).
> 
> ATT is a really simple protocol in this regard.

It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.

>> 
>> Unfortunately we need to get this one right and I know that rebasing patches can be a pain. So if you just want to send the initial 01/01 patch for this then that is fine with me as well. I think some units tests with this overlapping bi-directional communication would be amazingly great to have. I do wonder if other OSes handle this well.
> 
> Absolutely. Let me know if this has convinced you. So my idea is to
> have a bt_att_send and bt_att_send_without_rsp. I originally had a
> single bt_att_send for all opcodes, but Luiz brought up the fact that
> having a one for all function leads to some inconsistencies, such as
> cases where a callback is passed along with an opcode that doesn't
> expect a response. Personally I don't think this is such a problem as
> long as the API is well documented.

Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.

> Luiz's suggestion was to actually have a separate function per ATT
> opcode. In that approach we would remove the opcode + parameter
> structure design entirely and have the individual functions accept the
> right parameters. So, no bt_att_send, but bt_att_send_read_request,
> etc. Do you think this would be a cleaner approach in the end?

I do not want this. Having separate function for each opcode pair or function gets really complicated. We have been down this rabbit hole before. The more generic approach have worked out nicely with mgmt handling and I really like trying to duplicate that. And my current feeling is that bt_att_send is the way to go.

Regards

Marcel

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, May 28, 2014 at 11:37 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Arman,
>
>>>> +unsigned int bt_att_send_sequential(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);
>>>
>>> That is the implied behavior for _send functions. So I would not use _send_sequential here at all.
>>
>> This mainly came about after the discussion with Luiz on patch set v1.
>> My idea was to use bt_att_send for non-sequential ops and
>> bt_att_send_sequential for the opposite. I could rename these to
>> bt_att_send_without_rsp and bt_att_send, respectively.
>
> I am getting the feeling that just having bt_att_send only seems much cleaner. And if you give an opcode that does not expect a response, then you have to give an empty callback function. If not you get an error. I have to scratch my head a bit against this.
>
>>> I am still not sure that we should be doing it like this. We tried this with gattrib and it did not turn out that great. I really like the approach of I send this command and expect this PDU back or an error. The reason here is that in theory you can get interleaved commands/response on the same channel.
>>
>> I understand the reasoning behind this; then again it introduces
>> potential error cases such as expecting the wrong PDU back, e.g.
>> expecting a "Write Response" while sending a "Read Request". In the
>> end, I don't see much value in building the API that way, since in
>> ATT, all sequential requests have a 1-to-1 mapping to a sequential
>> response. e.g., if we receive a "Read Response", the currently pending
>> request absolutely has to be a "Read Request", otherwise we are
>> supposed to ignore the PDU (or drop the connection entirely, according
>> to the spec).
>>
>>>
>>> So this is what you would normally expect:
>>>
>>>        -> command
>>>        <- response
>>>
>>> Straight forward and simple, right. However nothing is stopping the other side from issues the same command in the other direction.
>>>
>>>        -> command
>>>        <- command
>>>        <- response
>>>        -> response
>>>
>>> And this is not to mention notifications and confirmations that can happen as well.
>>>
>>> I am thinking out loud here. What might help is a table that classifies which opcode is a command, response, notification or confirmation and based on that we just do the right thing.
>>
>> Well, I actually did write a similar unit test on my local branch
>> using my current implementation and these work as expected. The way I
>> structured the code actually does (or will do in upcoming patches)
>> exactly what you say, though instead of using tables, each "handler"
>> function explicitly checks the opcode to see if it falls into its
>> expected category (e.g. handle_response, handle_notification, etc.).
>>
>> ATT is a really simple protocol in this regard.
>
> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
>
>>>
>>> Unfortunately we need to get this one right and I know that rebasing patches can be a pain. So if you just want to send the initial 01/01 patch for this then that is fine with me as well. I think some units tests with this overlapping bi-directional communication would be amazingly great to have. I do wonder if other OSes handle this well.
>>
>> Absolutely. Let me know if this has convinced you. So my idea is to
>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>> having a one for all function leads to some inconsistencies, such as
>> cases where a callback is passed along with an opcode that doesn't
>> expect a response. Personally I don't think this is such a problem as
>> long as the API is well documented.
>
> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
>
>> Luiz's suggestion was to actually have a separate function per ATT
>> opcode. In that approach we would remove the opcode + parameter
>> structure design entirely and have the individual functions accept the
>> right parameters. So, no bt_att_send, but bt_att_send_read_request,
>> etc. Do you think this would be a cleaner approach in the end?
>
> I do not want this. Having separate function for each opcode pair or function gets really complicated. We have been down this rabbit hole before. The more generic approach have worked out nicely with mgmt handling and I really like trying to duplicate that. And my current feeling is that bt_att_send is the way to go.

At least the responses seems useless to queue to write latter and then
auto acknowledge once written because that will just cause more
latency, memory and complexity and we can probably just return
synchronously and retry/fail if a response cannot be sent out, we have
to remember that there is a socket in between so it is very unlikely
that a write would fail and in case that fail it is actually better to
return and error than just keep queuing.

In case of MGMT perhaps it was different because there you don't
actually have responses but actual requests that needs to be
acknowledged before the next request can be sent out.


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Luiz,

>>>>> +unsigned int bt_att_send_sequential(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);
>>>> 
>>>> That is the implied behavior for _send functions. So I would not use _send_sequential here at all.
>>> 
>>> This mainly came about after the discussion with Luiz on patch set v1.
>>> My idea was to use bt_att_send for non-sequential ops and
>>> bt_att_send_sequential for the opposite. I could rename these to
>>> bt_att_send_without_rsp and bt_att_send, respectively.
>> 
>> I am getting the feeling that just having bt_att_send only seems much cleaner. And if you give an opcode that does not expect a response, then you have to give an empty callback function. If not you get an error. I have to scratch my head a bit against this.
>> 
>>>> I am still not sure that we should be doing it like this. We tried this with gattrib and it did not turn out that great. I really like the approach of I send this command and expect this PDU back or an error. The reason here is that in theory you can get interleaved commands/response on the same channel.
>>> 
>>> I understand the reasoning behind this; then again it introduces
>>> potential error cases such as expecting the wrong PDU back, e.g.
>>> expecting a "Write Response" while sending a "Read Request". In the
>>> end, I don't see much value in building the API that way, since in
>>> ATT, all sequential requests have a 1-to-1 mapping to a sequential
>>> response. e.g., if we receive a "Read Response", the currently pending
>>> request absolutely has to be a "Read Request", otherwise we are
>>> supposed to ignore the PDU (or drop the connection entirely, according
>>> to the spec).
>>> 
>>>> 
>>>> So this is what you would normally expect:
>>>> 
>>>>       -> command
>>>>       <- response
>>>> 
>>>> Straight forward and simple, right. However nothing is stopping the other side from issues the same command in the other direction.
>>>> 
>>>>       -> command
>>>>       <- command
>>>>       <- response
>>>>       -> response
>>>> 
>>>> And this is not to mention notifications and confirmations that can happen as well.
>>>> 
>>>> I am thinking out loud here. What might help is a table that classifies which opcode is a command, response, notification or confirmation and based on that we just do the right thing.
>>> 
>>> Well, I actually did write a similar unit test on my local branch
>>> using my current implementation and these work as expected. The way I
>>> structured the code actually does (or will do in upcoming patches)
>>> exactly what you say, though instead of using tables, each "handler"
>>> function explicitly checks the opcode to see if it falls into its
>>> expected category (e.g. handle_response, handle_notification, etc.).
>>> 
>>> ATT is a really simple protocol in this regard.
>> 
>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
>> 
>>>> 
>>>> Unfortunately we need to get this one right and I know that rebasing patches can be a pain. So if you just want to send the initial 01/01 patch for this then that is fine with me as well. I think some units tests with this overlapping bi-directional communication would be amazingly great to have. I do wonder if other OSes handle this well.
>>> 
>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>> having a one for all function leads to some inconsistencies, such as
>>> cases where a callback is passed along with an opcode that doesn't
>>> expect a response. Personally I don't think this is such a problem as
>>> long as the API is well documented.
>> 
>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
>> 
>>> Luiz's suggestion was to actually have a separate function per ATT
>>> opcode. In that approach we would remove the opcode + parameter
>>> structure design entirely and have the individual functions accept the
>>> right parameters. So, no bt_att_send, but bt_att_send_read_request,
>>> etc. Do you think this would be a cleaner approach in the end?
>> 
>> I do not want this. Having separate function for each opcode pair or function gets really complicated. We have been down this rabbit hole before. The more generic approach have worked out nicely with mgmt handling and I really like trying to duplicate that. And my current feeling is that bt_att_send is the way to go.
> 
> At least the responses seems useless to queue to write latter and then
> auto acknowledge once written because that will just cause more
> latency, memory and complexity and we can probably just return
> synchronously and retry/fail if a response cannot be sent out, we have
> to remember that there is a socket in between so it is very unlikely
> that a write would fail and in case that fail it is actually better to
> return and error than just keep queuing.

that is exactly the one thing I do not not want. We are doing asynchronous IO. Meaning the write needs to be schedules once the socket signals POLLOUT. You can not do it differently. It all has to go through the same queue. We can have one queue for each direction to not block one with the other.

> In case of MGMT perhaps it was different because there you don't
> actually have responses but actual requests that needs to be
> acknowledged before the next request can be sent out.

Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue everything so that orders actually become predictable.

Regards

Marcel

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Arman Uguray]

Hi Luiz & Marcel,

>>> ATT is a really simple protocol in this regard.
>>
>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.

Sure. At least for requests, responses, and indications/notifications
we can have a table. For commands, you can actually check if the
command bit of the opcode has been set; in that regard the opcodes do
have some higher meaning: they do encode whether or not the operation
should be signed and whether or not they are commands. Then again, the
spec does define specifically which opcodes should have those bits
set. Eitherway, I will make it so that the semantic meaning of opcodes
is more explicitly checked in the source file.

>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>> having a one for all function leads to some inconsistencies, such as
>>> cases where a callback is passed along with an opcode that doesn't
>>> expect a response. Personally I don't think this is such a problem as
>>> long as the API is well documented.
>>
>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.

So, basically what I had earlier, with more explicit handling of
opcode semantics instead of somewhat vague handlers?

>> In case of MGMT perhaps it was different because there you don't
>> actually have responses but actual requests that needs to be
>> acknowledged before the next request can be sent out.
>
> Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue > everything so that orders actually become predictable.

Well this is also true for ATT. I.e. a client is not supposed to send
a second request before it has received a response to a previous
request on the same connection, hence they have to go through two
queues: a write queue waiting for POLLOUT, and a pending queue while
waiting for the response from the remote device, where the pending
queue bars any more requests from getting added to the write queue.
Outgoing indications will also go through the same queues. Commands
and notifications, on the other hand, skip the pending queue and can
go straight to the write queue.

The way I ended up implementing this involved having separate queues
for requests, responses, commands, notifications, and indications, but
I think that's overkill. We really just need a "sequential" queue, to
queue requests and indications and a "non-sequential" queue for
commands/notifications. In the former case, I think it's fine to use
the same queue for indications and requests, since indications are
only sent from servers and requests are only sent from clients, and
afaik you can't have both ends of the same ATT connection be
simultaneously in peripheral and central roles, though I may be wrong.
At least this is why iOS (& Android afaik) seem to expose a different
LE random address for each app that implements a GATT server.

-Arman

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Luiz Augusto von Dentz]

Hi Arman, Marcel,

On Wed, May 28, 2014 at 9:45 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz & Marcel,
>
>>>> ATT is a really simple protocol in this regard.
>>>
>>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
>
> Sure. At least for requests, responses, and indications/notifications
> we can have a table. For commands, you can actually check if the
> command bit of the opcode has been set; in that regard the opcodes do
> have some higher meaning: they do encode whether or not the operation
> should be signed and whether or not they are commands. Then again, the
> spec does define specifically which opcodes should have those bits
> set. Eitherway, I will make it so that the semantic meaning of opcodes
> is more explicitly checked in the source file.
>
>>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>>> having a one for all function leads to some inconsistencies, such as
>>>> cases where a callback is passed along with an opcode that doesn't
>>>> expect a response. Personally I don't think this is such a problem as
>>>> long as the API is well documented.
>>>
>>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
>
> So, basically what I had earlier, with more explicit handling of
> opcode semantics instead of somewhat vague handlers?
>
>>> In case of MGMT perhaps it was different because there you don't
>>> actually have responses but actual requests that needs to be
>>> acknowledged before the next request can be sent out.
>>
>> Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue > everything so that orders actually become predictable.
>
> Well this is also true for ATT. I.e. a client is not supposed to send
> a second request before it has received a response to a previous
> request on the same connection, hence they have to go through two
> queues: a write queue waiting for POLLOUT, and a pending queue while
> waiting for the response from the remote device, where the pending
> queue bars any more requests from getting added to the write queue.
> Outgoing indications will also go through the same queues. Commands
> and notifications, on the other hand, skip the pending queue and can
> go straight to the write queue.

Well regardless if you have it calling write directly or waiting
POLLOUT it can still fail which needs a callback and if the caller did
not have it maybe we should write it immediately.

> The way I ended up implementing this involved having separate queues
> for requests, responses, commands, notifications, and indications, but
> I think that's overkill. We really just need a "sequential" queue, to
> queue requests and indications and a "non-sequential" queue for
> commands/notifications. In the former case, I think it's fine to use
> the same queue for indications and requests, since indications are
> only sent from servers and requests are only sent from clients, and
> afaik you can't have both ends of the same ATT connection be
> simultaneously in peripheral and central roles, though I may be wrong.
> At least this is why iOS (& Android afaik) seem to expose a different
> LE random address for each app that implements a GATT server.

Here is where things get interesting, the role of a server and client
I believe is not per connection but per profile, so in a given
connection device A may act as a client for profile X but as a server
for profile Y.

Take a look at 3.3.2 Sequential Protocol in the core spec, I think in
some cases it is actually allowed to have multiple outstanding
requests if their types are different, perhaps that is why you had the
idea of multiple queues, but this makes the handling much more
complicated for the client but as a server we have to be able to
respond to those in parallel. One think that catches my eye was these
huge timeout of 30 seconds for a transaction, having a single request
queue can have a bad impact in responsiveness where a single request
can block anything to happen for 30 seconds.


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Luiz,

>>>>> ATT is a really simple protocol in this regard.
>>>> 
>>>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
>> 
>> Sure. At least for requests, responses, and indications/notifications
>> we can have a table. For commands, you can actually check if the
>> command bit of the opcode has been set; in that regard the opcodes do
>> have some higher meaning: they do encode whether or not the operation
>> should be signed and whether or not they are commands. Then again, the
>> spec does define specifically which opcodes should have those bits
>> set. Eitherway, I will make it so that the semantic meaning of opcodes
>> is more explicitly checked in the source file.
>> 
>>>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>>>> having a one for all function leads to some inconsistencies, such as
>>>>> cases where a callback is passed along with an opcode that doesn't
>>>>> expect a response. Personally I don't think this is such a problem as
>>>>> long as the API is well documented.
>>>> 
>>>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
>> 
>> So, basically what I had earlier, with more explicit handling of
>> opcode semantics instead of somewhat vague handlers?
>> 
>>>> In case of MGMT perhaps it was different because there you don't
>>>> actually have responses but actual requests that needs to be
>>>> acknowledged before the next request can be sent out.
>>> 
>>> Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue > everything so that orders actually become predictable.
>> 
>> Well this is also true for ATT. I.e. a client is not supposed to send
>> a second request before it has received a response to a previous
>> request on the same connection, hence they have to go through two
>> queues: a write queue waiting for POLLOUT, and a pending queue while
>> waiting for the response from the remote device, where the pending
>> queue bars any more requests from getting added to the write queue.
>> Outgoing indications will also go through the same queues. Commands
>> and notifications, on the other hand, skip the pending queue and can
>> go straight to the write queue.
> 
> Well regardless if you have it calling write directly or waiting
> POLLOUT it can still fail which needs a callback and if the caller did
> not have it maybe we should write it immediately.

if it fails due to the fact the kernel interrupted it, then you just queue it first item again and enter the main loop for the next attempt. It it fails fatal, then you disconnect the link. Either way, we are going through a queue and POLLOUT.

>> The way I ended up implementing this involved having separate queues
>> for requests, responses, commands, notifications, and indications, but
>> I think that's overkill. We really just need a "sequential" queue, to
>> queue requests and indications and a "non-sequential" queue for
>> commands/notifications. In the former case, I think it's fine to use
>> the same queue for indications and requests, since indications are
>> only sent from servers and requests are only sent from clients, and
>> afaik you can't have both ends of the same ATT connection be
>> simultaneously in peripheral and central roles, though I may be wrong.
>> At least this is why iOS (& Android afaik) seem to expose a different
>> LE random address for each app that implements a GATT server.
> 
> Here is where things get interesting, the role of a server and client
> I believe is not per connection but per profile, so in a given
> connection device A may act as a client for profile X but as a server
> for profile Y.

They are, but this does not change the fact that in case you have two profiles that use client roles that they can send requests at the same time. It all needs to be sequentially handled. No matter what.

> Take a look at 3.3.2 Sequential Protocol in the core spec, I think in
> some cases it is actually allowed to have multiple outstanding
> requests if their types are different, perhaps that is why you had the
> idea of multiple queues, but this makes the handling much more
> complicated for the client but as a server we have to be able to
> respond to those in parallel. One think that catches my eye was these
> huge timeout of 30 seconds for a transaction, having a single request
> queue can have a bad impact in responsiveness where a single request
> can block anything to happen for 30 seconds.

I do not not see how this is valid at all. Lets quote the spec here (same section)

"Once a client sends a request to a server, that client shall send no other request to the same server until a response PDU has been received.”

This makes it pretty clear that if you send a request, then you are not allow to send any other request. No matter if it is for a different profile/service or different PDU type. The same applies to indications.

"Indications sent from a server also use a sequential indication-confirmation protocol. No other indications shall be sent to the same client from this server until a confirmation PDU has been received.”

This falls also in line that transaction between a server and multiple clients are required to be atomic. And it is clear that the flow control / transaction model is applied for each direction individual.

"The client, however, is free to send commands and requests prior to sending a confirmation.”

What the specification is missing is a bit in the opcode that says, I am a notification. At that point it would be really simple to know which opcodes are allowed when a transaction is going on and which are not. More important which ones are part of the transaction and which are not when receiving.

That is why I was going to propose an internal table that clearly identifies which opcodes are requests, which ones are responses and most important which one is the notification.

Regards

Marcel

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Luiz Augusto von Dentz]

Hi Marcel,

On Sat, May 31, 2014 at 8:02 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Luiz,
>
>>>>>> ATT is a really simple protocol in this regard.
>>>>>
>>>>> It is simple, but it is also not structured. Opcode numbers have no h=
igher meaning. They are just numbers. That why having a table of known opco=
des and its semantics might be the simplest way.
>>>
>>> Sure. At least for requests, responses, and indications/notifications
>>> we can have a table. For commands, you can actually check if the
>>> command bit of the opcode has been set; in that regard the opcodes do
>>> have some higher meaning: they do encode whether or not the operation
>>> should be signed and whether or not they are commands. Then again, the
>>> spec does define specifically which opcodes should have those bits
>>> set. Eitherway, I will make it so that the semantic meaning of opcodes
>>> is more explicitly checked in the source file.
>>>
>>>>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>>>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>>>>> single bt_att_send for all opcodes, but Luiz brought up the fact tha=
t
>>>>>> having a one for all function leads to some inconsistencies, such as
>>>>>> cases where a callback is passed along with an opcode that doesn't
>>>>>> expect a response. Personally I don't think this is such a problem a=
s
>>>>>> long as the API is well documented.
>>>>>
>>>>> Right now as said above, just having bt_att_send seems enough. Start =
with that and see how it goes. Add an extra simpler helper that utilizes bt=
_att_send with an empty callback is dead simple. We can worry about that la=
ter.
>>>
>>> So, basically what I had earlier, with more explicit handling of
>>> opcode semantics instead of somewhat vague handlers?
>>>
>>>>> In case of MGMT perhaps it was different because there you don't
>>>>> actually have responses but actual requests that needs to be
>>>>> acknowledged before the next request can be sent out.
>>>>
>>>> Actually mgmt with calling write directly caused a bunch of issues. We=
 are not doing that anymore. We nicely queue > everything so that orders ac=
tually become predictable.
>>>
>>> Well this is also true for ATT. I.e. a client is not supposed to send
>>> a second request before it has received a response to a previous
>>> request on the same connection, hence they have to go through two
>>> queues: a write queue waiting for POLLOUT, and a pending queue while
>>> waiting for the response from the remote device, where the pending
>>> queue bars any more requests from getting added to the write queue.
>>> Outgoing indications will also go through the same queues. Commands
>>> and notifications, on the other hand, skip the pending queue and can
>>> go straight to the write queue.
>>
>> Well regardless if you have it calling write directly or waiting
>> POLLOUT it can still fail which needs a callback and if the caller did
>> not have it maybe we should write it immediately.
>
> if it fails due to the fact the kernel interrupted it, then you just queu=
e it first item again and enter the main loop for the next attempt. It it f=
ails fatal, then you disconnect the link. Either way, we are going through =
a queue and POLLOUT.
>
>>> The way I ended up implementing this involved having separate queues
>>> for requests, responses, commands, notifications, and indications, but
>>> I think that's overkill. We really just need a "sequential" queue, to
>>> queue requests and indications and a "non-sequential" queue for
>>> commands/notifications. In the former case, I think it's fine to use
>>> the same queue for indications and requests, since indications are
>>> only sent from servers and requests are only sent from clients, and
>>> afaik you can't have both ends of the same ATT connection be
>>> simultaneously in peripheral and central roles, though I may be wrong.
>>> At least this is why iOS (& Android afaik) seem to expose a different
>>> LE random address for each app that implements a GATT server.
>>
>> Here is where things get interesting, the role of a server and client
>> I believe is not per connection but per profile, so in a given
>> connection device A may act as a client for profile X but as a server
>> for profile Y.
>
> They are, but this does not change the fact that in case you have two pro=
files that use client roles that they can send requests at the same time. I=
t all needs to be sequentially handled. No matter what.
>
>> Take a look at 3.3.2 Sequential Protocol in the core spec, I think in
>> some cases it is actually allowed to have multiple outstanding
>> requests if their types are different, perhaps that is why you had the
>> idea of multiple queues, but this makes the handling much more
>> complicated for the client but as a server we have to be able to
>> respond to those in parallel. One think that catches my eye was these
>> huge timeout of 30 seconds for a transaction, having a single request
>> queue can have a bad impact in responsiveness where a single request
>> can block anything to happen for 30 seconds.
>
> I do not not see how this is valid at all. Lets quote the spec here (same=
 section)
>
> "Once a client sends a request to a server, that client shall send no oth=
er request to the same server until a response PDU has been received.=E2=80=
=9D
>
> This makes it pretty clear that if you send a request, then you are not a=
llow to send any other request. No matter if it is for a different profile/=
service or different PDU type. The same applies to indications.

Im afraid the spec is talking about a PDU with request type since it
has this kind of note:

"It is possible for a server to receive a request and then a command
before responding to the original request. The flow control of requests is =
not
affected by the transmission of commands."

My comments regarding requests was in general for outstanding PDUs
that requires a response (any type), this is reinforced by:

"Flow control for each client and a server is independent."

> "Indications sent from a server also use a sequential indication-confirma=
tion protocol. No other indications shall be sent to the same client from t=
his server until a confirmation PDU has been received.=E2=80=9D

Since the client and server are independent I believe you can have a
both a request and a indication outstanding given that the role is
defined by profile and we can have independent profiles using bt_att.

> This falls also in line that transaction between a server and multiple cl=
ients are required to be atomic. And it is clear that the flow control / tr=
ansaction model is applied for each direction individual.
>
> "The client, however, is free to send commands and requests prior to send=
ing a confirmation.=E2=80=9D
>
> What the specification is missing is a bit in the opcode that says, I am =
a notification. At that point it would be really simple to know which opcod=
es are allowed when a transaction is going on and which are not. More impor=
tant which ones are part of the transaction and which are not when receivin=
g.
>
> That is why I was going to propose an internal table that clearly identif=
ies which opcodes are requests, which ones are responses and most important=
 which one is the notification.

Well my argument was that we gonna need different queues for different
PDUs types due to the fact that each transaction has a huge timeout of
30 seconds, perhaps this is actually what you meant by identifying
what type of request it is but you didn't talk about how this would
affect the queueing. For commands in special there is no need for
queuing as they don't have any flow control.

--=20
Luiz Augusto von Dentz

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Luiz,

>>>>>>> ATT is a really simple protocol in this regard.
>>>>>> 
>>>>>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
>>>> 
>>>> Sure. At least for requests, responses, and indications/notifications
>>>> we can have a table. For commands, you can actually check if the
>>>> command bit of the opcode has been set; in that regard the opcodes do
>>>> have some higher meaning: they do encode whether or not the operation
>>>> should be signed and whether or not they are commands. Then again, the
>>>> spec does define specifically which opcodes should have those bits
>>>> set. Eitherway, I will make it so that the semantic meaning of opcodes
>>>> is more explicitly checked in the source file.
>>>> 
>>>>>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>>>>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>>>>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>>>>>> having a one for all function leads to some inconsistencies, such as
>>>>>>> cases where a callback is passed along with an opcode that doesn't
>>>>>>> expect a response. Personally I don't think this is such a problem as
>>>>>>> long as the API is well documented.
>>>>>> 
>>>>>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
>>>> 
>>>> So, basically what I had earlier, with more explicit handling of
>>>> opcode semantics instead of somewhat vague handlers?
>>>> 
>>>>>> In case of MGMT perhaps it was different because there you don't
>>>>>> actually have responses but actual requests that needs to be
>>>>>> acknowledged before the next request can be sent out.
>>>>> 
>>>>> Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue > everything so that orders actually become predictable.
>>>> 
>>>> Well this is also true for ATT. I.e. a client is not supposed to send
>>>> a second request before it has received a response to a previous
>>>> request on the same connection, hence they have to go through two
>>>> queues: a write queue waiting for POLLOUT, and a pending queue while
>>>> waiting for the response from the remote device, where the pending
>>>> queue bars any more requests from getting added to the write queue.
>>>> Outgoing indications will also go through the same queues. Commands
>>>> and notifications, on the other hand, skip the pending queue and can
>>>> go straight to the write queue.
>>> 
>>> Well regardless if you have it calling write directly or waiting
>>> POLLOUT it can still fail which needs a callback and if the caller did
>>> not have it maybe we should write it immediately.
>> 
>> if it fails due to the fact the kernel interrupted it, then you just queue it first item again and enter the main loop for the next attempt. It it fails fatal, then you disconnect the link. Either way, we are going through a queue and POLLOUT.
>> 
>>>> The way I ended up implementing this involved having separate queues
>>>> for requests, responses, commands, notifications, and indications, but
>>>> I think that's overkill. We really just need a "sequential" queue, to
>>>> queue requests and indications and a "non-sequential" queue for
>>>> commands/notifications. In the former case, I think it's fine to use
>>>> the same queue for indications and requests, since indications are
>>>> only sent from servers and requests are only sent from clients, and
>>>> afaik you can't have both ends of the same ATT connection be
>>>> simultaneously in peripheral and central roles, though I may be wrong.
>>>> At least this is why iOS (& Android afaik) seem to expose a different
>>>> LE random address for each app that implements a GATT server.
>>> 
>>> Here is where things get interesting, the role of a server and client
>>> I believe is not per connection but per profile, so in a given
>>> connection device A may act as a client for profile X but as a server
>>> for profile Y.
>> 
>> They are, but this does not change the fact that in case you have two profiles that use client roles that they can send requests at the same time. It all needs to be sequentially handled. No matter what.
>> 
>>> Take a look at 3.3.2 Sequential Protocol in the core spec, I think in
>>> some cases it is actually allowed to have multiple outstanding
>>> requests if their types are different, perhaps that is why you had the
>>> idea of multiple queues, but this makes the handling much more
>>> complicated for the client but as a server we have to be able to
>>> respond to those in parallel. One think that catches my eye was these
>>> huge timeout of 30 seconds for a transaction, having a single request
>>> queue can have a bad impact in responsiveness where a single request
>>> can block anything to happen for 30 seconds.
>> 
>> I do not not see how this is valid at all. Lets quote the spec here (same section)
>> 
>> "Once a client sends a request to a server, that client shall send no other request to the same server until a response PDU has been received.”
>> 
>> This makes it pretty clear that if you send a request, then you are not allow to send any other request. No matter if it is for a different profile/service or different PDU type. The same applies to indications.
> 
> Im afraid the spec is talking about a PDU with request type since it
> has this kind of note:
> 
> "It is possible for a server to receive a request and then a command
> before responding to the original request. The flow control of requests is not
> affected by the transmission of commands."
> 
> My comments regarding requests was in general for outstanding PDUs
> that requires a response (any type), this is reinforced by:
> 
> "Flow control for each client and a server is independent."

that is pretty much what I said. Since requests and commands are different, you can only having one request at a time, but potentially multiple commands as well.

>> "Indications sent from a server also use a sequential indication-confirmation protocol. No other indications shall be sent to the same client from this server until a confirmation PDU has been received.”
> 
> Since the client and server are independent I believe you can have a
> both a request and a indication outstanding given that the role is
> defined by profile and we can have independent profiles using bt_att.

That is my current assumption as well. You can send a request, then send an indication. However I am curious on how many current implementations handle this correctly. We need test cases for this interleaving.

>> This falls also in line that transaction between a server and multiple clients are required to be atomic. And it is clear that the flow control / transaction model is applied for each direction individual.
>> 
>> "The client, however, is free to send commands and requests prior to sending a confirmation.”
>> 
>> What the specification is missing is a bit in the opcode that says, I am a notification. At that point it would be really simple to know which opcodes are allowed when a transaction is going on and which are not. More important which ones are part of the transaction and which are not when receiving.
>> 
>> That is why I was going to propose an internal table that clearly identifies which opcodes are requests, which ones are responses and most important which one is the notification.
> 
> Well my argument was that we gonna need different queues for different
> PDUs types due to the fact that each transaction has a huge timeout of
> 30 seconds, perhaps this is actually what you meant by identifying
> what type of request it is but you didn't talk about how this would
> affect the queueing. For commands in special there is no need for
> queuing as they don't have any flow control.

See my other email for how we should handle this. While commands do not need flow control, the commands still have to go through a queue for writing.

Regards

Marcel

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Marcel Holtmann]

Hi Arman,

>>>> ATT is a really simple protocol in this regard.
>>> 
>>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
> 
> Sure. At least for requests, responses, and indications/notifications
> we can have a table. For commands, you can actually check if the
> command bit of the opcode has been set; in that regard the opcodes do
> have some higher meaning: they do encode whether or not the operation
> should be signed and whether or not they are commands. Then again, the
> spec does define specifically which opcodes should have those bits
> set. Eitherway, I will make it so that the semantic meaning of opcodes
> is more explicitly checked in the source file.

good point with the commands. I wish they would have done the same for notifications.

This reminds me that we should include the support for signed payloads right into bt_att. We ran into this with the Android support. The actual opcode is part of the signed data. So I think we just need to have functions that allows us to attach the CSRK for signing and resolving right to the bt_att and we do the actual signing and resolving in the background for the user.

> 
>>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>>> having a one for all function leads to some inconsistencies, such as
>>>> cases where a callback is passed along with an opcode that doesn't
>>>> expect a response. Personally I don't think this is such a problem as
>>>> long as the API is well documented.
>>> 
>>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
> 
> So, basically what I had earlier, with more explicit handling of
> opcode semantics instead of somewhat vague handlers?

Yes. If you go with one bt_att_send that just takes the opcode, then we need builtin semantics on what that opcode means and in case of errors, just returns false.

>>> In case of MGMT perhaps it was different because there you don't
>>> actually have responses but actual requests that needs to be
>>> acknowledged before the next request can be sent out.
>> 
>> Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue > everything so that orders actually become predictable.
> 
> Well this is also true for ATT. I.e. a client is not supposed to send
> a second request before it has received a response to a previous
> request on the same connection, hence they have to go through two
> queues: a write queue waiting for POLLOUT, and a pending queue while
> waiting for the response from the remote device, where the pending
> queue bars any more requests from getting added to the write queue.
> Outgoing indications will also go through the same queues. Commands
> and notifications, on the other hand, skip the pending queue and can
> go straight to the write queue.

In general I think we need a request queue and an indication queue. Since these are the two transaction that we have. One is for the client and one for the server and they can intermix. Additionally we keep a busy flag for each queue. That way the write handler can easily process the queues. In addition we need a general queue for commands and notifications since they can go at any time. This is the sending side. When we are the client for requests/commands and when we are the server for indications/notifications.

For the receiving side and having to respond to requests and indications, we can just use the general queue to get things written out. However the API might be a bt_att_reply in that case.

> The way I ended up implementing this involved having separate queues
> for requests, responses, commands, notifications, and indications, but
> I think that's overkill. We really just need a "sequential" queue, to
> queue requests and indications and a "non-sequential" queue for
> commands/notifications. In the former case, I think it's fine to use
> the same queue for indications and requests, since indications are
> only sent from servers and requests are only sent from clients, and
> afaik you can't have both ends of the same ATT connection be
> simultaneously in peripheral and central roles, though I may be wrong.
> At least this is why iOS (& Android afaik) seem to expose a different
> LE random address for each app that implements a GATT server.

The peripheral and central roles have nothing to do with GATT. A peripheral can implement a GATT client + server and a central can also have a GATT client + server. In the general case, you have a server and client no matter what LE role you are in.

And for iOS and the different LE random addresses, that is just plain LE Privacy support with resolvable random addresses. You will see that iOS devices switch their address every 9 minutes. However if you have the IRK, then you can resolve them into the device public address. Newer Linux kernels will do that job for you. Once paired with an iOS peripheral or central, the kernel will just give you the public address.

Regards

Marcel

Re: [PATCH v2 01/10] src/shared/att: Introduce struct bt_att.

[Arman Uguray]

Hi Marcel,

Thanks, this conversation was very helpful. I'll revise the initial
patch and send it to the mailing list some time next week.

Cheers,
-Arman

On Fri, May 30, 2014 at 10:13 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Arman,
>
>>>>> ATT is a really simple protocol in this regard.
>>>>
>>>> It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way.
>>
>> Sure. At least for requests, responses, and indications/notifications
>> we can have a table. For commands, you can actually check if the
>> command bit of the opcode has been set; in that regard the opcodes do
>> have some higher meaning: they do encode whether or not the operation
>> should be signed and whether or not they are commands. Then again, the
>> spec does define specifically which opcodes should have those bits
>> set. Eitherway, I will make it so that the semantic meaning of opcodes
>> is more explicitly checked in the source file.
>
> good point with the commands. I wish they would have done the same for notifications.
>
> This reminds me that we should include the support for signed payloads right into bt_att. We ran into this with the Android support. The actual opcode is part of the signed data. So I think we just need to have functions that allows us to attach the CSRK for signing and resolving right to the bt_att and we do the actual signing and resolving in the background for the user.
>
>>
>>>>> Absolutely. Let me know if this has convinced you. So my idea is to
>>>>> have a bt_att_send and bt_att_send_without_rsp. I originally had a
>>>>> single bt_att_send for all opcodes, but Luiz brought up the fact that
>>>>> having a one for all function leads to some inconsistencies, such as
>>>>> cases where a callback is passed along with an opcode that doesn't
>>>>> expect a response. Personally I don't think this is such a problem as
>>>>> long as the API is well documented.
>>>>
>>>> Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later.
>>
>> So, basically what I had earlier, with more explicit handling of
>> opcode semantics instead of somewhat vague handlers?
>
> Yes. If you go with one bt_att_send that just takes the opcode, then we need builtin semantics on what that opcode means and in case of errors, just returns false.
>
>>>> In case of MGMT perhaps it was different because there you don't
>>>> actually have responses but actual requests that needs to be
>>>> acknowledged before the next request can be sent out.
>>>
>>> Actually mgmt with calling write directly caused a bunch of issues. We are not doing that anymore. We nicely queue > everything so that orders actually become predictable.
>>
>> Well this is also true for ATT. I.e. a client is not supposed to send
>> a second request before it has received a response to a previous
>> request on the same connection, hence they have to go through two
>> queues: a write queue waiting for POLLOUT, and a pending queue while
>> waiting for the response from the remote device, where the pending
>> queue bars any more requests from getting added to the write queue.
>> Outgoing indications will also go through the same queues. Commands
>> and notifications, on the other hand, skip the pending queue and can
>> go straight to the write queue.
>
> In general I think we need a request queue and an indication queue. Since these are the two transaction that we have. One is for the client and one for the server and they can intermix. Additionally we keep a busy flag for each queue. That way the write handler can easily process the queues. In addition we need a general queue for commands and notifications since they can go at any time. This is the sending side. When we are the client for requests/commands and when we are the server for indications/notifications.
>
> For the receiving side and having to respond to requests and indications, we can just use the general queue to get things written out. However the API might be a bt_att_reply in that case.
>
>> The way I ended up implementing this involved having separate queues
>> for requests, responses, commands, notifications, and indications, but
>> I think that's overkill. We really just need a "sequential" queue, to
>> queue requests and indications and a "non-sequential" queue for
>> commands/notifications. In the former case, I think it's fine to use
>> the same queue for indications and requests, since indications are
>> only sent from servers and requests are only sent from clients, and
>> afaik you can't have both ends of the same ATT connection be
>> simultaneously in peripheral and central roles, though I may be wrong.
>> At least this is why iOS (& Android afaik) seem to expose a different
>> LE random address for each app that implements a GATT server.
>
> The peripheral and central roles have nothing to do with GATT. A peripheral can implement a GATT client + server and a central can also have a GATT client + server. In the general case, you have a server and client no matter what LE role you are in.
>
> And for iOS and the different LE random addresses, that is just plain LE Privacy support with resolvable random addresses. You will see that iOS devices switch their address every 9 minutes. However if you have the IRK, then you can resolve them into the device public address. Newer Linux kernels will do that job for you. Once paired with an iOS peripheral or central, the kernel will just give you the public address.
>
> Regards
>
> Marcel
>

[PATCH v2 02/10] unit/test-att: Add unit tests for src/shared/att.

[Arman Uguray]

This patch introduces unit tests for src/shared/att. The code currently
only tests locally initiated requests and responses.
---
 Makefile.am     |   9 ++
 unit/test-att.c | 308 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 317 insertions(+)
 create mode 100644 unit/test-att.c

diff --git a/Makefile.am b/Makefile.am
index 0c3424f..04be6c4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -262,6 +262,15 @@ unit_test_mgmt_SOURCES = unit/test-mgmt.c \
 				src/shared/mgmt.h src/shared/mgmt.c
 unit_test_mgmt_LDADD = @GLIB_LIBS@
 
+unit_tests += unit/test-att
+
+unit_test_att_SOURCES = unit/test-att.c \
+				src/shared/io.h src/shared/io-glib.c \
+				src/shared/queue.h src/shared/queue.c \
+				src/shared/util.h src/shared/util.c \
+				src/shared/att.h src/shared/att.c
+unit_test_att_LDADD = @GLIB_LIBS@
+
 unit_tests += unit/test-sdp
 
 unit_test_sdp_SOURCES = unit/test-sdp.c \
diff --git a/unit/test-att.c b/unit/test-att.c
new file mode 100644
index 0000000..9d631fb
--- /dev/null
+++ b/unit/test-att.c
@@ -0,0 +1,308 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *  Copyright (C) 2014  Intel Corporation.
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program 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 General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; 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 <stdlib.h>
+#include <stdbool.h>
+#include <unistd.h>
+#include <sys/socket.h>
+
+#include <glib.h>
+
+#include "src/shared/att.h"
+
+struct context {
+	GMainLoop *main_loop;
+	struct bt_att *att_client;
+	guint server_source;
+	GList *handler_list;
+};
+
+enum action {
+	ACTION_PASSED,
+	ACTION_IGNORE,
+};
+
+struct handler {
+	const void *req_data;
+	uint16_t req_size;
+	const void *resp_data;
+	uint16_t resp_size;
+	enum action action;
+};
+
+static void att_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	g_printf("%s%s\n", prefix, str);
+}
+
+static void context_quit(struct context *context)
+{
+	g_main_loop_quit(context->main_loop);
+}
+
+static void check_actions(struct context *context, int server_socket,
+					const void *data, uint16_t size)
+{
+	GList *list;
+	ssize_t written;
+
+	for (list = g_list_first(context->handler_list); list;
+						list = g_list_next(list)) {
+		struct handler *handler = list->data;
+
+		if (size != handler->req_size)
+			continue;
+
+		if (memcmp(data, handler->req_data, handler->req_size))
+			continue;
+
+		switch (handler->action) {
+		case ACTION_PASSED:
+			if (!handler->resp_data || !handler->resp_size) {
+				context_quit(context);
+				return;
+			}
+
+			/* Test case involves a response. */
+			written = write(server_socket, handler->resp_data,
+							handler->resp_size);
+			g_assert(written == handler->resp_size);
+			return;
+		case ACTION_IGNORE:
+			return;
+		}
+	}
+
+	g_test_message("Request not handled\n");
+	g_assert_not_reached();
+}
+
+static gboolean server_handler(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct context *context = user_data;
+	unsigned char buf[512];
+	ssize_t result;
+	int fd;
+
+	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP))
+		return FALSE;
+
+	fd = g_io_channel_unix_get_fd(channel);
+
+	result = read(fd, buf, sizeof(buf));
+	if (result < 0)
+		return FALSE;
+
+	check_actions(context, fd, buf, result);
+
+	return TRUE;
+}
+
+static struct context *create_context(void)
+{
+	struct context *context = g_new0(struct context, 1);
+	GIOChannel *channel;
+	int err, sv[2];
+
+	context->main_loop = g_main_loop_new(NULL, FALSE);
+	g_assert(context->main_loop);
+
+	err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+	g_assert(err == 0);
+
+	channel = g_io_channel_unix_new(sv[0]);
+
+	g_io_channel_set_close_on_unref(channel, TRUE);
+	g_io_channel_set_encoding(channel, NULL, NULL);
+	g_io_channel_set_buffered(channel, FALSE);
+
+	context->server_source = g_io_add_watch(channel,
+				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				server_handler, context);
+	g_assert(context->server_source > 0);
+
+	g_io_channel_unref(channel);
+
+	context->att_client = bt_att_new(sv[1]);
+	g_assert(context->att_client);
+
+	if (g_test_verbose() == TRUE)
+		bt_att_set_debug(context->att_client, att_debug, "att: ", NULL);
+
+	bt_att_set_close_on_unref(context->att_client, true);
+
+	return context;
+}
+
+static void execute_context(struct context *context)
+{
+	g_main_loop_run(context->main_loop);
+
+	g_list_free_full(context->handler_list, g_free);
+
+	g_source_remove(context->server_source);
+
+	bt_att_unref(context->att_client);
+
+	g_main_loop_unref(context->main_loop);
+
+	g_free(context);
+}
+
+static void add_action(struct context *context,
+				const void *req_data, uint16_t req_size,
+				const void *resp_data, uint16_t resp_size,
+				enum action action)
+{
+	struct handler *handler = g_new0(struct handler, 1);
+
+	handler->req_data = req_data;
+	handler->req_size = req_size;
+	handler->resp_data = resp_data;
+	handler->resp_size = resp_size;
+	handler->action = action;
+
+	context->handler_list = g_list_append(context->handler_list, handler);
+}
+
+struct request_test_data {
+	uint8_t req_opcode;
+	const void *req_param;
+	uint16_t req_param_length;
+	uint8_t resp_opcode;
+	const void *resp_param;
+	uint16_t resp_param_length;
+	const void *req_data;
+	uint16_t req_size;
+	const  void *resp_data;
+	uint16_t resp_size;
+};
+
+struct request_test {
+	const struct request_test_data *test_data;
+	struct context *context;
+};
+
+/* Exchange MTU request <-> response test */
+static const unsigned char exchange_mtu_req_bytes_1[] = { 0x02, 0x32, 0x00 };
+static const struct bt_att_exchange_mtu_req_param exchange_mtu_req_param_1 = {
+	.client_rx_mtu = 50,
+};
+static const unsigned char exchange_mtu_resp_bytes_1[] = { 0x03, 0x30, 0x00 };
+static const struct bt_att_exchange_mtu_rsp_param exchange_mtu_rsp_param_1 = {
+	.server_rx_mtu = 48,
+};
+static const struct request_test_data request_test_1 = {
+	.req_opcode = BT_ATT_OP_EXCHANGE_MTU_REQ,
+	.req_param = &exchange_mtu_req_param_1,
+	.req_param_length = sizeof(exchange_mtu_req_param_1),
+	.resp_opcode = BT_ATT_OP_EXCHANGE_MTU_RESP,
+	.resp_param = &exchange_mtu_rsp_param_1,
+	.resp_param_length = sizeof(exchange_mtu_rsp_param_1),
+	.req_data = exchange_mtu_req_bytes_1,
+	.req_size = sizeof(exchange_mtu_req_bytes_1),
+	.resp_data = exchange_mtu_resp_bytes_1,
+	.resp_size = sizeof(exchange_mtu_resp_bytes_1),
+};
+
+/* Exchange MTU request <-> error response test */
+static const unsigned char exchange_mtu_req_bytes_2[] = { 0x02, 0x32, 0x00 };
+static const struct bt_att_exchange_mtu_req_param exchange_mtu_req_param_2 = {
+	.client_rx_mtu = 50,
+};
+static const unsigned char exchange_mtu_resp_bytes_2[] = {
+	0x01, 0x02, 0x00, 0x00, 0x06
+};
+static const struct bt_att_error_rsp_param exchange_mtu_rsp_param_2 = {
+	.request_opcode = BT_ATT_OP_EXCHANGE_MTU_REQ,
+	.handle = 0,
+	.error_code = BT_ATT_ERROR_REQUEST_NOT_SUPPORTED,
+};
+static const struct request_test_data request_test_2 = {
+	.req_opcode = BT_ATT_OP_EXCHANGE_MTU_REQ,
+	.req_param = &exchange_mtu_req_param_2,
+	.req_param_length = sizeof(exchange_mtu_req_param_2),
+	.resp_opcode = BT_ATT_OP_ERROR_RESP,
+	.resp_param = &exchange_mtu_rsp_param_2,
+	.resp_param_length = sizeof(exchange_mtu_rsp_param_2),
+	.req_data = exchange_mtu_req_bytes_2,
+	.req_size = sizeof(exchange_mtu_req_bytes_2),
+	.resp_data = exchange_mtu_resp_bytes_2,
+	.resp_size = sizeof(exchange_mtu_resp_bytes_2),
+};
+
+static void test_request_callback(uint8_t opcode, const void *param,
+					uint16_t length, void *user_data)
+{
+	struct request_test *test = user_data;
+	const struct request_test_data *test_data = test->test_data;
+
+	g_assert(opcode == test_data->resp_opcode);
+	g_assert(length == test_data->resp_param_length);
+
+	g_assert(0 == memcmp(param, test_data->resp_param, length));
+
+	context_quit(test->context);
+}
+
+static void test_request(gconstpointer data)
+{
+	struct request_test *test;
+	const struct request_test_data *test_data = data;
+	struct context *context = create_context();
+	unsigned int req_id = 0;
+
+	add_action(context, test_data->req_data, test_data->req_size,
+			test_data->resp_data, test_data->resp_size,
+			ACTION_PASSED);
+
+	test = malloc(sizeof(struct request_test));
+	test->test_data = test_data; 
+	test->context = context;
+
+	req_id = bt_att_send_sequential(context->att_client,
+				test_data->req_opcode, test_data->req_param,
+				test_data->req_param_length,
+				test_request_callback, test, free);
+	g_assert(req_id);
+
+	execute_context(context);
+}
+
+int main(int argc, char *argv[])
+{
+	g_test_init(&argc, &argv, NULL);
+
+	g_test_add_data_func("/att/request/1", &request_test_1, test_request);
+	g_test_add_data_func("/att/request/2", &request_test_2, test_request);
+
+	return g_test_run();
+}
-- 
1.8.3.2

Re: [PATCH v2 02/10] unit/test-att: Add unit tests for src/shared/att.

[Marcel Holtmann]

Hi Arman,

> This patch introduces unit tests for src/shared/att. The code currently
> only tests locally initiated requests and responses.
> ---
> Makefile.am     |   9 ++
> unit/test-att.c | 308 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 317 insertions(+)
> create mode 100644 unit/test-att.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 0c3424f..04be6c4 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -262,6 +262,15 @@ unit_test_mgmt_SOURCES = unit/test-mgmt.c \
> 				src/shared/mgmt.h src/shared/mgmt.c
> unit_test_mgmt_LDADD = @GLIB_LIBS@
> 
> +unit_tests += unit/test-att
> +
> +unit_test_att_SOURCES = unit/test-att.c \
> +				src/shared/io.h src/shared/io-glib.c \
> +				src/shared/queue.h src/shared/queue.c \
> +				src/shared/util.h src/shared/util.c \
> +				src/shared/att.h src/shared/att.c
> +unit_test_att_LDADD = @GLIB_LIBS@
> +
> unit_tests += unit/test-sdp
> 
> unit_test_sdp_SOURCES = unit/test-sdp.c \
> diff --git a/unit/test-att.c b/unit/test-att.c
> new file mode 100644
> index 0000000..9d631fb
> --- /dev/null
> +++ b/unit/test-att.c
> @@ -0,0 +1,308 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *  Copyright (C) 2014  Intel Corporation.
> + *
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program 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 General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; 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 <stdlib.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <sys/socket.h>
> +
> +#include <glib.h>
> +
> +#include "src/shared/att.h"
> +
> +struct context {
> +	GMainLoop *main_loop;
> +	struct bt_att *att_client;
> +	guint server_source;
> +	GList *handler_list;
> +};
> +
> +enum action {
> +	ACTION_PASSED,
> +	ACTION_IGNORE,
> +};
> +
> +struct handler {
> +	const void *req_data;
> +	uint16_t req_size;
> +	const void *resp_data;
> +	uint16_t resp_size;
> +	enum action action;
> +};
> +
> +static void att_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	g_printf("%s%s\n", prefix, str);
> +}
> +
> +static void context_quit(struct context *context)
> +{
> +	g_main_loop_quit(context->main_loop);
> +}
> +
> +static void check_actions(struct context *context, int server_socket,
> +					const void *data, uint16_t size)
> +{
> +	GList *list;
> +	ssize_t written;
> +
> +	for (list = g_list_first(context->handler_list); list;
> +						list = g_list_next(list)) {
> +		struct handler *handler = list->data;
> +
> +		if (size != handler->req_size)
> +			continue;
> +
> +		if (memcmp(data, handler->req_data, handler->req_size))
> +			continue;
> +
> +		switch (handler->action) {
> +		case ACTION_PASSED:
> +			if (!handler->resp_data || !handler->resp_size) {
> +				context_quit(context);
> +				return;
> +			}
> +
> +			/* Test case involves a response. */
> +			written = write(server_socket, handler->resp_data,
> +							handler->resp_size);
> +			g_assert(written == handler->resp_size);
> +			return;
> +		case ACTION_IGNORE:
> +			return;
> +		}
> +	}
> +
> +	g_test_message("Request not handled\n");
> +	g_assert_not_reached();
> +}
> +
> +static gboolean server_handler(GIOChannel *channel, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	struct context *context = user_data;
> +	unsigned char buf[512];
> +	ssize_t result;
> +	int fd;
> +
> +	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP))
> +		return FALSE;
> +
> +	fd = g_io_channel_unix_get_fd(channel);
> +
> +	result = read(fd, buf, sizeof(buf));
> +	if (result < 0)
> +		return FALSE;
> +
> +	check_actions(context, fd, buf, result);
> +
> +	return TRUE;
> +}
> +
> +static struct context *create_context(void)
> +{
> +	struct context *context = g_new0(struct context, 1);
> +	GIOChannel *channel;
> +	int err, sv[2];
> +
> +	context->main_loop = g_main_loop_new(NULL, FALSE);
> +	g_assert(context->main_loop);
> +
> +	err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
> +	g_assert(err == 0);
> +
> +	channel = g_io_channel_unix_new(sv[0]);
> +
> +	g_io_channel_set_close_on_unref(channel, TRUE);
> +	g_io_channel_set_encoding(channel, NULL, NULL);
> +	g_io_channel_set_buffered(channel, FALSE);
> +
> +	context->server_source = g_io_add_watch(channel,
> +				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> +				server_handler, context);
> +	g_assert(context->server_source > 0);
> +
> +	g_io_channel_unref(channel);

please target struct io as wrapper for IO handling. Long term we want to get rid of GIOChannel and GLib.

Regards

Marcel

[PATCH v2 03/10] tools/btatt: Add command-line tool for ATT protocol testing.

[Arman Uguray]

Initial commit for tools/btatt. Added basic command line options parsing
and added the tool to Makefile.tools as experimental.
---
 Makefile.tools |  10 +++-
 tools/btatt.c  | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100644 tools/btatt.c

diff --git a/Makefile.tools b/Makefile.tools
index 40f076b..13f6691 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -278,7 +278,7 @@ noinst_PROGRAMS += tools/bdaddr tools/avinfo tools/avtest \
 			tools/btmgmt tools/btinfo tools/btattach \
 			tools/btsnoop tools/btproxy tools/btiotest \
 			tools/mpris-player tools/cltest tools/seq2bseq \
-			tools/ibeacon
+			tools/ibeacon tools/btatt
 
 tools_bdaddr_SOURCES = tools/bdaddr.c src/oui.h src/oui.c
 tools_bdaddr_LDADD = lib/libbluetooth-internal.la @UDEV_LIBS@
@@ -342,6 +342,14 @@ tools_ibeacon_SOURCES = tools/ibeacon.c monitor/bt.h \
 				src/shared/queue.h src/shared/queue.c \
 				src/shared/ringbuf.h src/shared/ringbuf.c
 
+tools_btatt_SOURCES = tools/btatt.c src/uuid-helper.c \
+				monitor/mainloop.h monitor/mainloop.c \
+				src/shared/io.h src/shared/io-mainloop.c \
+				src/shared/queue.h src/shared/queue.c \
+				src/shared/util.h src/shared/util.c \
+				src/shared/att.h src/shared/att.c
+tools_btatt_LDADD = lib/libbluetooth-internal.la
+
 EXTRA_DIST += tools/bdaddr.1
 endif
 
diff --git a/tools/btatt.c b/tools/btatt.c
new file mode 100644
index 0000000..476bb94
--- /dev/null
+++ b/tools/btatt.c
@@ -0,0 +1,152 @@
+/*
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program 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 General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; 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 <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <getopt.h>
+#include <string.h>
+
+#include <bluetooth/bluetooth.h>
+
+#include "monitor/mainloop.h"
+#include "src/shared/att.h"
+
+#define ATT_CID 4
+
+static bool verbose = false;
+
+static struct {
+	char *cmd;
+	void (*func)(struct bt_att *att, int argc, char **argv);
+	char *doc;
+} command[] = {
+	{ }
+};
+
+static void usage(void)
+{
+	int i;
+
+	printf("btatt\n");
+	printf("Usage:\n"
+		"\tbtatt [options] <command> [command parameters]\n");
+
+	printf("Options:\n"
+		"\t-i, --index <id>\tSpecify adapter index, e.g. hci0\n"
+		"\t-d, --dest <addr>\tSpecify the destination address\n"
+		"\t-t, --type [random|public] \tSpecify the LE address type\n"
+		"\t-v, --verbose\tEnable extra logging\n"
+		"\t-h, --help\tDisplay help\n");
+
+	printf("Commands:\n");
+	for (i = 0; command[i].cmd; i++)
+		printf("\t%-15s\t%s\n", command[i].cmd, command[i].doc);
+
+	printf("\n"
+		"For more information on the usage of each command use:\n"
+		"\tbtatt <command> --help\n" );
+}
+
+static struct option main_options[] = {
+	{ "index",	1, 0, 'i' },
+	{ "dest",	1, 0, 'd' },
+	{ "type",	1, 0, 't' },
+	{ "verbose",	0, 0, 'v' },
+	{ "help",	0, 0, 'h' },
+	{ 0, 0, 0, 0}
+};
+
+int main(int argc, char *argv[])
+{
+	int opt, i;
+	int dev_id = -1;
+	bool dest_given = false;
+	uint8_t dest_type = BDADDR_LE_PUBLIC;
+	bdaddr_t src_addr, dst_addr;
+	int fd;
+
+	while ((opt = getopt_long(argc, argv, "+hvt:i:d:",
+						main_options, NULL)) != -1) {
+		switch (opt) {
+		case 'i':
+			dev_id = hci_devid(optarg);
+			if (dev_id < 0) {
+				perror("Invalid adapter");
+				return EXIT_FAILURE;
+			}
+			break;
+		case 'd':
+			if (str2ba(optarg, &dst_addr) < 0) {
+				fprintf(stderr, "Invalid destination address\n");
+				return EXIT_FAILURE;
+			}
+			dest_given = true;
+			break;
+		case 't':
+			if (strcmp(optarg, "random") == 0)
+				dest_type = BDADDR_LE_RANDOM;
+			else if (strcmp(optarg, "public") == 0)
+				dest_type = BDADDR_LE_PUBLIC;
+			else {
+				fprintf(stderr, "Allowed types: random, public\n");
+				return EXIT_FAILURE;
+			}
+			break;
+		case 'v':
+			verbose = true;
+			break;
+		case 'h':
+			usage();
+			return 0;
+		default:
+			break;
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+	optind = 0;
+
+	if (argc < 1) {
+		usage();
+		return 0;
+	}
+
+	if (dev_id == -1)
+		bacpy(&src_addr, BDADDR_ANY);
+	else if (hci_devba(dev_id, &src_addr) < 0) {
+		perror("Adapter not available");
+		return EXIT_FAILURE;
+	}
+
+	if (!dest_given) {
+		fprintf(stderr, "Destination address required\n");
+		return EXIT_FAILURE;
+	}
+
+	return 0;
+}
-- 
1.8.3.2

Re: [PATCH v2 03/10] tools/btatt: Add command-line tool for ATT protocol testing.

[Marcel Holtmann]

Hi Arman,

> Initial commit for tools/btatt. Added basic command line options parsing
> and added the tool to Makefile.tools as experimental.
> ---
> Makefile.tools |  10 +++-
> tools/btatt.c  | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 161 insertions(+), 1 deletion(-)
> create mode 100644 tools/btatt.c
> 
> diff --git a/Makefile.tools b/Makefile.tools
> index 40f076b..13f6691 100644
> --- a/Makefile.tools
> +++ b/Makefile.tools
> @@ -278,7 +278,7 @@ noinst_PROGRAMS += tools/bdaddr tools/avinfo tools/avtest \
> 			tools/btmgmt tools/btinfo tools/btattach \
> 			tools/btsnoop tools/btproxy tools/btiotest \
> 			tools/mpris-player tools/cltest tools/seq2bseq \
> -			tools/ibeacon
> +			tools/ibeacon tools/btatt
> 
> tools_bdaddr_SOURCES = tools/bdaddr.c src/oui.h src/oui.c
> tools_bdaddr_LDADD = lib/libbluetooth-internal.la @UDEV_LIBS@
> @@ -342,6 +342,14 @@ tools_ibeacon_SOURCES = tools/ibeacon.c monitor/bt.h \
> 				src/shared/queue.h src/shared/queue.c \
> 				src/shared/ringbuf.h src/shared/ringbuf.c
> 
> +tools_btatt_SOURCES = tools/btatt.c src/uuid-helper.c \
> +				monitor/mainloop.h monitor/mainloop.c \
> +				src/shared/io.h src/shared/io-mainloop.c \
> +				src/shared/queue.h src/shared/queue.c \
> +				src/shared/util.h src/shared/util.c \
> +				src/shared/att.h src/shared/att.c
> +tools_btatt_LDADD = lib/libbluetooth-internal.la
> +
> EXTRA_DIST += tools/bdaddr.1
> endif
> 
> diff --git a/tools/btatt.c b/tools/btatt.c
> new file mode 100644
> index 0000000..476bb94
> --- /dev/null
> +++ b/tools/btatt.c
> @@ -0,0 +1,152 @@
> +/*
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program 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 General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; 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 <stdbool.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <getopt.h>
> +#include <string.h>
> +
> +#include <bluetooth/bluetooth.h>
> +
> +#include "monitor/mainloop.h"
> +#include "src/shared/att.h"
> +
> +#define ATT_CID 4
> +
> +static bool verbose = false;
> +
> +static struct {
> +	char *cmd;
> +	void (*func)(struct bt_att *att, int argc, char **argv);
> +	char *doc;
> +} command[] = {
> +	{ }
> +};
> +
> +static void usage(void)
> +{
> +	int i;
> +
> +	printf("btatt\n");
> +	printf("Usage:\n"
> +		"\tbtatt [options] <command> [command parameters]\n");
> +
> +	printf("Options:\n"
> +		"\t-i, --index <id>\tSpecify adapter index, e.g. hci0\n"
> +		"\t-d, --dest <addr>\tSpecify the destination address\n"
> +		"\t-t, --type [random|public] \tSpecify the LE address type\n"
> +		"\t-v, --verbose\tEnable extra logging\n"
> +		"\t-h, --help\tDisplay help\n");
> +
> +	printf("Commands:\n");
> +	for (i = 0; command[i].cmd; i++)
> +		printf("\t%-15s\t%s\n", command[i].cmd, command[i].doc);
> +
> +	printf("\n"
> +		"For more information on the usage of each command use:\n"
> +		"\tbtatt <command> --help\n" );
> +}
> +
> +static struct option main_options[] = {
> +	{ "index",	1, 0, 'i' },
> +	{ "dest",	1, 0, 'd' },
> +	{ "type",	1, 0, 't' },
> +	{ "verbose",	0, 0, 'v' },
> +	{ "help",	0, 0, 'h' },
> +	{ 0, 0, 0, 0}
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +	int opt, i;
> +	int dev_id = -1;
> +	bool dest_given = false;
> +	uint8_t dest_type = BDADDR_LE_PUBLIC;
> +	bdaddr_t src_addr, dst_addr;
> +	int fd;
> +
> +	while ((opt = getopt_long(argc, argv, "+hvt:i:d:",
> +						main_options, NULL)) != -1) {
> +		switch (opt) {
> +		case 'i':
> +			dev_id = hci_devid(optarg);
> +			if (dev_id < 0) {
> +				perror("Invalid adapter");
> +				return EXIT_FAILURE;
> +			}

if at all possible, avoid libbluetooth functions. And we should support --index 0 and also --index hci0 here as valid parameters. See btmon for example.

> +			break;
> +		case 'd':
> +			if (str2ba(optarg, &dst_addr) < 0) {
> +				fprintf(stderr, "Invalid destination address\n");
> +				return EXIT_FAILURE;
> +			}
> +			dest_given = true;
> +			break;
> +		case 't':
> +			if (strcmp(optarg, "random") == 0)
> +				dest_type = BDADDR_LE_RANDOM;
> +			else if (strcmp(optarg, "public") == 0)
> +				dest_type = BDADDR_LE_PUBLIC;
> +			else {
> +				fprintf(stderr, "Allowed types: random, public\n");
> +				return EXIT_FAILURE;
> +			}

This is cosmetic, but public should become before random.

> +			break;
> +		case 'v':
> +			verbose = true;
> +			break;
> +		case 'h':
> +			usage();
> +			return 0;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	argc -= optind;
> +	argv += optind;
> +	optind = 0;
> +
> +	if (argc < 1) {
> +		usage();
> +		return 0;
> +	}
> +
> +	if (dev_id == -1)
> +		bacpy(&src_addr, BDADDR_ANY);

I would just default src_addr to BDADDR_ANY in the beginning of main() and then have index option overwrite it.

> +	else if (hci_devba(dev_id, &src_addr) < 0) {
> +		perror("Adapter not available");
> +		return EXIT_FAILURE;
> +	}

I would say, lets just have the connect() fail if the adapter is not available. No need to be super smart upfront. The L2CAP socket will give us a proper error.

> +
> +	if (!dest_given) {
> +		fprintf(stderr, "Destination address required\n");
> +		return EXIT_FAILURE;
> +	}

Comparing it to BDADDR_ANY here might be simpler. Same as with src_addr, set it to BDADDR_ANY in the beginning.

Regards

Marcel

[PATCH v2 04/10] tools/btatt: Add "exchange-mtu" command.

[Arman Uguray]

Added command for "Exchange MTU" request/response. Also added code for
initiating an L2CAP connection over the ATT channel. Tested using a CSR
uEnergy 1000 board with the following results:

$ btatt -d 00:02:5B:00:15:10 -v exchange-mtu 50
Going to set MTU to 16 bit value: 50
att: ATT command 0x02
att < 02 32 00
att > 03 17 00
Exchange MTU response: Server Rx MTU 23
---
 tools/btatt.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 191 insertions(+), 1 deletion(-)

diff --git a/tools/btatt.c b/tools/btatt.c
index 476bb94..da1eb3d 100644
--- a/tools/btatt.c
+++ b/tools/btatt.c
@@ -31,6 +31,7 @@
 #include <string.h>
 
 #include <bluetooth/bluetooth.h>
+#include <bluetooth/l2cap.h>
 
 #include "monitor/mainloop.h"
 #include "src/shared/att.h"
@@ -39,11 +40,153 @@
 
 static bool verbose = false;
 
+static void handle_error(const void *param, uint16_t len)
+{
+	const struct bt_att_error_rsp_param *rp = param;
+
+	if (len != sizeof(*rp)) {
+		fprintf(stderr, "Invalid error response length (%u)\n", len);
+		return;
+	}
+
+	printf("Request failed:\n\topcode:\t%u\n\thandle:\t%u\n\terror:\t%u\n",
+			rp->request_opcode, rp->handle, rp->error_code);
+}
+
+static void exchange_mtu_cb(uint8_t opcode, const void *param,
+						uint16_t len, void *user_data)
+{
+	const struct bt_att_exchange_mtu_rsp_param *rp;
+
+	if (opcode == BT_ATT_OP_ERROR_RESP) {
+		handle_error(param, len);
+		goto done;
+	}
+
+	if (opcode != BT_ATT_OP_EXCHANGE_MTU_RESP) {
+		fprintf(stderr, "Invalid response opcode (%u)\n", opcode);
+		goto done;
+	}
+
+	rp = param;
+
+	if (len != sizeof(*rp)) {
+		fprintf(stderr, "Invalid \"Exchange MTU\" response length "
+								"(%u)\n", len);
+		goto done;
+	}
+
+	printf("Exchange MTU response: Server Rx MTU: %u\n", rp->server_rx_mtu);
+
+done:
+	mainloop_quit();
+}
+
+static void mtu_usage(void)
+{
+	printf("Usage: btatt exchange-mtu <ATT_MTU>\n");
+}
+
+static struct option mtu_options[] = {
+	{ "help",	0, 0, 'h'},
+	{}
+};
+
+static void cmd_mtu(struct bt_att *att, int argc, char **argv)
+{
+	struct bt_att_exchange_mtu_req_param param;
+	uint16_t mtu;
+	int opt;
+
+	while ((opt = getopt_long(argc, argv, "+h", mtu_options,
+							NULL)) != -1) {
+		switch (opt) {
+		case 'h':
+		default:
+			mtu_usage();
+			exit(EXIT_SUCCESS);
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+	optind = 0;
+
+	if (argc < 1) {
+		mtu_usage();
+		exit(EXIT_FAILURE);
+	}
+
+	mtu = atoi(argv[0]);
+
+	if (verbose)
+		printf("Going to set MTU to 16 bit value: %u\n", mtu);
+
+	memset(&param, 0, sizeof(param));
+	param.client_rx_mtu = mtu;
+	if (bt_att_send_sequential(att, BT_ATT_OP_EXCHANGE_MTU_REQ,
+					&param, sizeof(param),
+					exchange_mtu_cb, NULL, NULL) == 0) {
+		fprintf(stderr, "Unable to send \"Exchange MTU\" request\n");
+		exit(EXIT_FAILURE);
+	}
+}
+
+static int l2cap_le_att_connect(bdaddr_t *src, bdaddr_t *dst, uint8_t dst_type)
+{
+	int sock;
+	struct sockaddr_l2 srcaddr, dstaddr;
+	char srcaddr_str[18], dstaddr_str[18];
+
+	if (verbose) {
+		ba2str(src, srcaddr_str);
+		ba2str(dst, dstaddr_str);
+
+		printf("Opening L2CAP LE connection on ATT channel: "
+					"src: %s dest: %s\n", srcaddr_str, dstaddr_str);
+	}
+
+	sock = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP);
+	if (sock < 0) {
+		perror("Failed to create L2CAP socket");
+		return -1;
+	}
+
+	/* Set up source address */
+	memset(&srcaddr, 0, sizeof(srcaddr));
+	srcaddr.l2_family = AF_BLUETOOTH;
+	bacpy(&srcaddr.l2_bdaddr, src);
+	srcaddr.l2_cid = htobs(ATT_CID);
+	srcaddr.l2_bdaddr_type = BDADDR_LE_PUBLIC;
+
+	if (bind(sock, (struct sockaddr *) &srcaddr, sizeof(srcaddr)) < 0) {
+		perror("Failed to bind L2CAP socket");
+		close(sock);
+		return -1;
+	}
+
+	/* Set up destination address */
+	memset(&dstaddr, 0, sizeof(dstaddr));
+	dstaddr.l2_family = AF_BLUETOOTH;
+	bacpy(&dstaddr.l2_bdaddr, dst);
+	dstaddr.l2_cid = htobs(ATT_CID);
+	dstaddr.l2_bdaddr_type = dst_type;
+
+	if (connect(sock, (struct sockaddr *) &dstaddr, sizeof(dstaddr)) < 0) {
+		perror("Failed to connect");
+		close(sock);
+		return -1;
+	}
+
+	return sock;
+}
+
 static struct {
 	char *cmd;
 	void (*func)(struct bt_att *att, int argc, char **argv);
 	char *doc;
 } command[] = {
+	{ "exchange-mtu",	cmd_mtu,	"\"Exchange MTU\" request." },
 	{ }
 };
 
@@ -80,6 +223,13 @@ static struct option main_options[] = {
 	{ 0, 0, 0, 0}
 };
 
+static void att_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	printf("%s%s\n", prefix, str);
+}
+
 int main(int argc, char *argv[])
 {
 	int opt, i;
@@ -88,6 +238,8 @@ int main(int argc, char *argv[])
 	uint8_t dest_type = BDADDR_LE_PUBLIC;
 	bdaddr_t src_addr, dst_addr;
 	int fd;
+	struct bt_att *att;
+	int exit_status;
 
 	while ((opt = getopt_long(argc, argv, "+hvt:i:d:",
 						main_options, NULL)) != -1) {
@@ -148,5 +300,43 @@ int main(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	return 0;
+	mainloop_init();
+
+	fd = l2cap_le_att_connect(&src_addr, &dst_addr, dest_type);
+
+	if (fd < 0)
+		return EXIT_FAILURE;
+
+	att = bt_att_new(fd);
+
+	if (!att) {
+		fprintf(stderr, "Failed to create bt_att.");
+		close(fd);
+		return EXIT_FAILURE;
+	}
+
+	if (verbose)
+		bt_att_set_debug(att, att_debug, "att: ", NULL);
+
+	bt_att_set_close_on_unref(att, true);
+
+	for (i = 0; command[i].cmd; i++) {
+		if (strcmp(command[i].cmd, argv[0]) == 0)
+			break;
+	}
+
+	if (command[i].cmd == NULL) {
+		fprintf(stderr, "Unknown command: %s\n", argv[0]);
+		bt_att_unref(att);
+		return EXIT_FAILURE;
+	}
+
+	command[i].func(att, argc, argv);
+
+	exit_status = mainloop_run();
+
+	bt_att_cancel_all(att);
+	bt_att_unref(att);
+
+	return exit_status;
 }
-- 
1.8.3.2

Re: [PATCH v2 04/10] tools/btatt: Add "exchange-mtu" command.

[Marcel Holtmann]

Hi Arman,

> Added command for "Exchange MTU" request/response. Also added code for
> initiating an L2CAP connection over the ATT channel. Tested using a CSR
> uEnergy 1000 board with the following results:
> 
> $ btatt -d 00:02:5B:00:15:10 -v exchange-mtu 50
> Going to set MTU to 16 bit value: 50
> att: ATT command 0x02
> att < 02 32 00
> att > 03 17 00
> Exchange MTU response: Server Rx MTU 23
> ---
> tools/btatt.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 191 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/btatt.c b/tools/btatt.c
> index 476bb94..da1eb3d 100644
> --- a/tools/btatt.c
> +++ b/tools/btatt.c
> @@ -31,6 +31,7 @@
> #include <string.h>
> 
> #include <bluetooth/bluetooth.h>
> +#include <bluetooth/l2cap.h>
> 
> #include "monitor/mainloop.h"
> #include "src/shared/att.h"
> @@ -39,11 +40,153 @@
> 
> static bool verbose = false;
> 
> +static void handle_error(const void *param, uint16_t len)
> +{
> +	const struct bt_att_error_rsp_param *rp = param;
> +
> +	if (len != sizeof(*rp)) {
> +		fprintf(stderr, "Invalid error response length (%u)\n", len);
> +		return;
> +	}
> +
> +	printf("Request failed:\n\topcode:\t%u\n\thandle:\t%u\n\terror:\t%u\n",
> +			rp->request_opcode, rp->handle, rp->error_code);
> +}
> +
> +static void exchange_mtu_cb(uint8_t opcode, const void *param,
> +						uint16_t len, void *user_data)
> +{
> +	const struct bt_att_exchange_mtu_rsp_param *rp;
> +
> +	if (opcode == BT_ATT_OP_ERROR_RESP) {
> +		handle_error(param, len);
> +		goto done;
> +	}
> +
> +	if (opcode != BT_ATT_OP_EXCHANGE_MTU_RESP) {
> +		fprintf(stderr, "Invalid response opcode (%u)\n", opcode);
> +		goto done;
> +	}
> +
> +	rp = param;
> +
> +	if (len != sizeof(*rp)) {
> +		fprintf(stderr, "Invalid \"Exchange MTU\" response length "
> +								"(%u)\n", len);
> +		goto done;
> +	}
> +
> +	printf("Exchange MTU response: Server Rx MTU: %u\n", rp->server_rx_mtu);
> +
> +done:
> +	mainloop_quit();
> +}
> +
> +static void mtu_usage(void)
> +{
> +	printf("Usage: btatt exchange-mtu <ATT_MTU>\n");
> +}
> +
> +static struct option mtu_options[] = {
> +	{ "help",	0, 0, 'h'},
> +	{}
> +};
> +
> +static void cmd_mtu(struct bt_att *att, int argc, char **argv)
> +{
> +	struct bt_att_exchange_mtu_req_param param;
> +	uint16_t mtu;
> +	int opt;
> +
> +	while ((opt = getopt_long(argc, argv, "+h", mtu_options,
> +							NULL)) != -1) {
> +		switch (opt) {
> +		case 'h':
> +		default:
> +			mtu_usage();
> +			exit(EXIT_SUCCESS);
> +		}
> +	}

these days I prefer the for (;;) style we are using in btmon and some newer tools.

> +
> +	argc -= optind;
> +	argv += optind;
> +	optind = 0;
> +
> +	if (argc < 1) {
> +		mtu_usage();
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	mtu = atoi(argv[0]);
> +
> +	if (verbose)
> +		printf("Going to set MTU to 16 bit value: %u\n", mtu);
> +
> +	memset(&param, 0, sizeof(param));
> +	param.client_rx_mtu = mtu;
> +	if (bt_att_send_sequential(att, BT_ATT_OP_EXCHANGE_MTU_REQ,
> +					&param, sizeof(param),
> +					exchange_mtu_cb, NULL, NULL) == 0) {
> +		fprintf(stderr, "Unable to send \"Exchange MTU\" request\n");
> +		exit(EXIT_FAILURE);
> +	}
> +}
> +
> +static int l2cap_le_att_connect(bdaddr_t *src, bdaddr_t *dst, uint8_t dst_type)
> +{
> +	int sock;
> +	struct sockaddr_l2 srcaddr, dstaddr;
> +	char srcaddr_str[18], dstaddr_str[18];
> +
> +	if (verbose) {
> +		ba2str(src, srcaddr_str);
> +		ba2str(dst, dstaddr_str);
> +
> +		printf("Opening L2CAP LE connection on ATT channel: "
> +					"src: %s dest: %s\n", srcaddr_str, dstaddr_str);
> +	}
> +
> +	sock = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP);
> +	if (sock < 0) {
> +		perror("Failed to create L2CAP socket");
> +		return -1;
> +	}
> +
> +	/* Set up source address */
> +	memset(&srcaddr, 0, sizeof(srcaddr));
> +	srcaddr.l2_family = AF_BLUETOOTH;
> +	bacpy(&srcaddr.l2_bdaddr, src);
> +	srcaddr.l2_cid = htobs(ATT_CID);
> +	srcaddr.l2_bdaddr_type = BDADDR_LE_PUBLIC;
> +
> +	if (bind(sock, (struct sockaddr *) &srcaddr, sizeof(srcaddr)) < 0) {
> +		perror("Failed to bind L2CAP socket");
> +		close(sock);
> +		return -1;
> +	}
> +
> +	/* Set up destination address */
> +	memset(&dstaddr, 0, sizeof(dstaddr));
> +	dstaddr.l2_family = AF_BLUETOOTH;
> +	bacpy(&dstaddr.l2_bdaddr, dst);
> +	dstaddr.l2_cid = htobs(ATT_CID);
> +	dstaddr.l2_bdaddr_type = dst_type;
> +
> +	if (connect(sock, (struct sockaddr *) &dstaddr, sizeof(dstaddr)) < 0) {
> +		perror("Failed to connect");
> +		close(sock);
> +		return -1;
> +	}
> +
> +	return sock;
> +}
> +
> static struct {
> 	char *cmd;
> 	void (*func)(struct bt_att *att, int argc, char **argv);
> 	char *doc;

Make them const char * if they do not allow modifications.

> } command[] = {
> +	{ "exchange-mtu",	cmd_mtu,	"\"Exchange MTU\" request." },
> 	{ }
> };

We have not done this in any tool before, but maybe consider including the struct option for the individual parameters in it. Play with it and see how it works out.

> 
> @@ -80,6 +223,13 @@ static struct option main_options[] = {
> 	{ 0, 0, 0, 0}
> };
> 
> +static void att_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	printf("%s%s\n", prefix, str);
> +}
> +
> int main(int argc, char *argv[])
> {
> 	int opt, i;
> @@ -88,6 +238,8 @@ int main(int argc, char *argv[])
> 	uint8_t dest_type = BDADDR_LE_PUBLIC;
> 	bdaddr_t src_addr, dst_addr;
> 	int fd;
> +	struct bt_att *att;
> +	int exit_status;
> 
> 	while ((opt = getopt_long(argc, argv, "+hvt:i:d:",
> 						main_options, NULL)) != -1) {
> @@ -148,5 +300,43 @@ int main(int argc, char *argv[])
> 		return EXIT_FAILURE;
> 	}
> 
> -	return 0;
> +	mainloop_init();
> +
> +	fd = l2cap_le_att_connect(&src_addr, &dst_addr, dest_type);
> +

These empty lines need to go away.

> +	if (fd < 0)
> +		return EXIT_FAILURE;
> +
> +	att = bt_att_new(fd);
> +
> +	if (!att) {
> +		fprintf(stderr, "Failed to create bt_att.");
> +		close(fd);
> +		return EXIT_FAILURE;
> +	}
> +
> +	if (verbose)
> +		bt_att_set_debug(att, att_debug, "att: ", NULL);

I wonder if the option should be just --debug and debug_enabled or something like that. And on a side not, make it ATT: here.

> +
> +	bt_att_set_close_on_unref(att, true);
> +
> +	for (i = 0; command[i].cmd; i++) {
> +		if (strcmp(command[i].cmd, argv[0]) == 0)

Step by step we are moving away from strcmp() == 0. Use !strcmp().
> +			break;
> +	}
> +
> +	if (command[i].cmd == NULL) {

Use if (!command[i].cmd) here.

> +		fprintf(stderr, "Unknown command: %s\n", argv[0]);
> +		bt_att_unref(att);
> +		return EXIT_FAILURE;

I would set exit_status to EXIT_FAILURE and goto done;

> +	}
> +
> +	command[i].func(att, argc, argv);
> +
> +	exit_status = mainloop_run();
> +
> +	bt_att_cancel_all(att);

The cancel_all is a bit useless here. The att_unref should clean that out for you.

> +	bt_att_unref(att);
> +
> +	return exit_status;
> }

Regards

Marcel

[PATCH v2 05/10] src/shared/att: Add "Find Information" request and response.

[Arman Uguray]

This patch adds support for the "Find Information" request and response
with their corresponding parameter structures.
---
 src/shared/att.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 src/shared/att.h | 14 +++++++++
 2 files changed, 99 insertions(+), 8 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 8b10c30..f124731 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -192,6 +192,8 @@ static bool handle_exchange_mtu_rsp(struct bt_att *att, const uint8_t *pdu,
 	if (pdu_length != 3)
 		return false;
 
+	memset(&param, 0, sizeof(param));
+
 	param.server_rx_mtu = get_le16(pdu + 1);
 
 	request_complete(att, BT_ATT_OP_EXCHANGE_MTU_REQ, pdu[0],
@@ -200,16 +202,67 @@ static bool handle_exchange_mtu_rsp(struct bt_att *att, const uint8_t *pdu,
 	return true;
 }
 
+static bool handle_find_info_rsp(struct bt_att *att, const uint8_t *pdu,
+							uint16_t pdu_length)
+{
+	struct bt_att_find_information_rsp_param param;
+	uint8_t *information_data;
+
+	if (pdu_length < 6)
+		return false;
+
+	memset(&param, 0, sizeof(param));
+
+	param.format = pdu[1];
+	param.length = pdu_length - 2;
+
+	/* param.length is at least 4, as checked above */
+	information_data = malloc(param.length);
+	if (!information_data)
+		return false;
+
+	memcpy(information_data, pdu + 2, param.length);
+
+	param.information_data = information_data;
+
+	request_complete(att, BT_ATT_OP_FIND_INFORMATION_REQ, pdu[0],
+							&param, sizeof(param));
+
+	free(information_data);
+
+	return true;
+}
+
 static bool handle_response(struct bt_att *att, const uint8_t *pdu,
 							uint16_t pdu_length)
 {
 	uint8_t opcode = pdu[0];
+	uint8_t req_opcode;
 
-	if (opcode == BT_ATT_OP_ERROR_RESP)
+	switch (opcode) {
+	case BT_ATT_OP_ERROR_RESP:
 		return handle_error_rsp(att, pdu, pdu_length);
 
-	if (opcode == BT_ATT_OP_EXCHANGE_MTU_RESP)
-		return handle_exchange_mtu_rsp(att, pdu, pdu_length);
+	case BT_ATT_OP_EXCHANGE_MTU_RESP:
+		if (!handle_exchange_mtu_rsp(att, pdu, pdu_length)) {
+			req_opcode = BT_ATT_OP_EXCHANGE_MTU_REQ;
+			goto fail;
+		}
+		return true;
+
+	case BT_ATT_OP_FIND_INFORMATION_RESP:
+		if (!handle_find_info_rsp(att, pdu, pdu_length)) {
+			req_opcode = BT_ATT_OP_FIND_INFORMATION_REQ;
+			goto fail;
+		}
+		return true;
+
+	default:
+		break;
+	}
+
+fail:
+	request_complete(att, req_opcode, BT_ATT_OP_ERROR_RESP, NULL, 0);
 
 	return false;
 }
@@ -405,14 +458,36 @@ static bool encode_mtu_req(const void *param, uint16_t length, void *buf,
 {
 	const struct bt_att_exchange_mtu_req_param *cp = param;
 	const uint16_t len = 3;
+	uint8_t *bytes = buf;
 
-	if (length != sizeof(struct bt_att_exchange_mtu_req_param))
+	if (length != sizeof(*cp))
 		return false;
 
 	if (len > mtu)
 		return false;
 
-	put_le16(cp->client_rx_mtu, buf);
+	put_le16(cp->client_rx_mtu, bytes + 1);
+	*pdu_length = len;
+
+	return true;
+}
+
+static bool encode_find_info_req(const void *param, uint16_t length, void *buf,
+						uint16_t mtu, uint16_t *pdu_length)
+{
+	const struct bt_att_find_information_req_param *cp = param;
+	const uint16_t len = 5;
+	uint8_t *bytes = buf;
+
+	if (length != sizeof(*cp))
+		return false;
+
+	if (len > mtu)
+		return false;
+
+	put_le16(cp->start_handle, bytes + 1);
+	put_le16(cp->end_handle, bytes + 3);
+
 	*pdu_length = len;
 
 	return true;
@@ -439,10 +514,12 @@ static bool encode_pdu(uint8_t opcode, const void *param, uint16_t length,
 	if (!param)
 		return false;
 
-	if (opcode == BT_ATT_OP_EXCHANGE_MTU_REQ) {
-		return encode_mtu_req(param, length, bytes + 1,
+	if (opcode == BT_ATT_OP_EXCHANGE_MTU_REQ)
+		return encode_mtu_req(param, length, bytes, mtu, pdu_length);
+
+	if (opcode == BT_ATT_OP_FIND_INFORMATION_REQ)
+		return encode_find_info_req(param, length, bytes,
 							mtu, pdu_length);
-	}
 
 	return false;
 }
diff --git a/src/shared/att.h b/src/shared/att.h
index 8fe8805..16d852d 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -43,6 +43,20 @@ struct bt_att_exchange_mtu_rsp_param {
 	uint16_t server_rx_mtu;
 };
 
+/* Find Information */
+#define BT_ATT_OP_FIND_INFORMATION_REQ		0x04
+struct bt_att_find_information_req_param {
+	uint16_t start_handle;
+	uint16_t end_handle;
+};
+
+#define BT_ATT_OP_FIND_INFORMATION_RESP		0x05
+struct bt_att_find_information_rsp_param {
+	uint8_t format;
+	const uint8_t *information_data;
+	uint16_t length;
+};
+
 /* Error codes for Error response PDU */
 #define BT_ATT_ERROR_INVALID_HANDLE			0x01
 #define BT_ATT_ERROR_READ_NOT_PERMITTED			0x02
-- 
1.8.3.2

Re: [PATCH v2 05/10] src/shared/att: Add "Find Information" request and response.

[Marcel Holtmann]

Hi Arman,

> This patch adds support for the "Find Information" request and response
> with their corresponding parameter structures.
> ---
> src/shared/att.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
> src/shared/att.h | 14 +++++++++
> 2 files changed, 99 insertions(+), 8 deletions(-)
> 
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 8b10c30..f124731 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -192,6 +192,8 @@ static bool handle_exchange_mtu_rsp(struct bt_att *att, const uint8_t *pdu,
> 	if (pdu_length != 3)
> 		return false;
> 
> +	memset(&param, 0, sizeof(param));
> +

skip the empty line here.

> 	param.server_rx_mtu = get_le16(pdu + 1);
> 
> 	request_complete(att, BT_ATT_OP_EXCHANGE_MTU_REQ, pdu[0],
> @@ -200,16 +202,67 @@ static bool handle_exchange_mtu_rsp(struct bt_att *att, const uint8_t *pdu,
> 	return true;
> }
> 
> +static bool handle_find_info_rsp(struct bt_att *att, const uint8_t *pdu,
> +							uint16_t pdu_length)
> +{
> +	struct bt_att_find_information_rsp_param param;
> +	uint8_t *information_data;

Use info_data please. Spelling out information in code makes my brain hurt ;)

> +
> +	if (pdu_length < 6)
> +		return false;
> +
> +	memset(&param, 0, sizeof(param));
> +
> +	param.format = pdu[1];
> +	param.length = pdu_length - 2;
> +
> +	/* param.length is at least 4, as checked above */
> +	information_data = malloc(param.length);
> +	if (!information_data)
> +		return false;
> +
> +	memcpy(information_data, pdu + 2, param.length);
> +
> +	param.information_data = information_data;
> +
> +	request_complete(att, BT_ATT_OP_FIND_INFORMATION_REQ, pdu[0],
> +							&param, sizeof(param));

On a side note, INFO_REQ is always better. Check monitor/bt.h for some ideas on when to shortcut and when not. I tried to be consistent as much I can be when I re-did all the constants. Sometimes it is a taste thing, but the long names really get in your way in the long run. I learned that the hard way.

> +
> +	free(information_data);
> +
> +	return true;
> +}
> +
> static bool handle_response(struct bt_att *att, const uint8_t *pdu,
> 							uint16_t pdu_length)
> {
> 	uint8_t opcode = pdu[0];
> +	uint8_t req_opcode;
> 
> -	if (opcode == BT_ATT_OP_ERROR_RESP)
> +	switch (opcode) {
> +	case BT_ATT_OP_ERROR_RESP:
> 		return handle_error_rsp(att, pdu, pdu_length);
> 
> -	if (opcode == BT_ATT_OP_EXCHANGE_MTU_RESP)
> -		return handle_exchange_mtu_rsp(att, pdu, pdu_length);
> +	case BT_ATT_OP_EXCHANGE_MTU_RESP:
> +		if (!handle_exchange_mtu_rsp(att, pdu, pdu_length)) {
> +			req_opcode = BT_ATT_OP_EXCHANGE_MTU_REQ;
> +			goto fail;
> +		}
> +		return true;
> +
> +	case BT_ATT_OP_FIND_INFORMATION_RESP:
> +		if (!handle_find_info_rsp(att, pdu, pdu_length)) {
> +			req_opcode = BT_ATT_OP_FIND_INFORMATION_REQ;
> +			goto fail;
> +		}
> +		return true;
> +
> +	default:
> +		break;
> +	}
> +
> +fail:
> +	request_complete(att, req_opcode, BT_ATT_OP_ERROR_RESP, NULL, 0);
> 
> 	return false;
> }
> @@ -405,14 +458,36 @@ static bool encode_mtu_req(const void *param, uint16_t length, void *buf,
> {
> 	const struct bt_att_exchange_mtu_req_param *cp = param;
> 	const uint16_t len = 3;
> +	uint8_t *bytes = buf;
> 
> -	if (length != sizeof(struct bt_att_exchange_mtu_req_param))
> +	if (length != sizeof(*cp))
> 		return false;
> 
> 	if (len > mtu)
> 		return false;
> 
> -	put_le16(cp->client_rx_mtu, buf);
> +	put_le16(cp->client_rx_mtu, bytes + 1);
> +	*pdu_length = len;
> +
> +	return true;
> +}
> +
> +static bool encode_find_info_req(const void *param, uint16_t length, void *buf,
> +						uint16_t mtu, uint16_t *pdu_length)
> +{
> +	const struct bt_att_find_information_req_param *cp = param;
> +	const uint16_t len = 5;
> +	uint8_t *bytes = buf;
> +
> +	if (length != sizeof(*cp))
> +		return false;
> +
> +	if (len > mtu)
> +		return false;
> +
> +	put_le16(cp->start_handle, bytes + 1);
> +	put_le16(cp->end_handle, bytes + 3);
> +
> 	*pdu_length = len;
> 
> 	return true;
> @@ -439,10 +514,12 @@ static bool encode_pdu(uint8_t opcode, const void *param, uint16_t length,
> 	if (!param)
> 		return false;
> 
> -	if (opcode == BT_ATT_OP_EXCHANGE_MTU_REQ) {
> -		return encode_mtu_req(param, length, bytes + 1,
> +	if (opcode == BT_ATT_OP_EXCHANGE_MTU_REQ)
> +		return encode_mtu_req(param, length, bytes, mtu, pdu_length);
> +
> +	if (opcode == BT_ATT_OP_FIND_INFORMATION_REQ)
> +		return encode_find_info_req(param, length, bytes,
> 							mtu, pdu_length);
> -	}
> 
> 	return false;
> }
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 8fe8805..16d852d 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -43,6 +43,20 @@ struct bt_att_exchange_mtu_rsp_param {
> 	uint16_t server_rx_mtu;
> };
> 
> +/* Find Information */
> +#define BT_ATT_OP_FIND_INFORMATION_REQ		0x04
> +struct bt_att_find_information_req_param {
> +	uint16_t start_handle;
> +	uint16_t end_handle;
> +};
> +
> +#define BT_ATT_OP_FIND_INFORMATION_RESP		0x05
> +struct bt_att_find_information_rsp_param {
> +	uint8_t format;
> +	const uint8_t *information_data;
> +	uint16_t length;
> +};

When you have wire format structs, use attribute packed with the struct. See monitor/bt.h.

I also think that is where these should go anyway.

> +
> /* Error codes for Error response PDU */
> #define BT_ATT_ERROR_INVALID_HANDLE			0x01
> #define BT_ATT_ERROR_READ_NOT_PERMITTED			0x02

Regards

Marcel

[PATCH v2 06/10] unit/test-att: Add unit test for "Find Information" request/response.

[Arman Uguray]

Added test case for the "Find Information" request/response.
---
 unit/test-att.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/unit/test-att.c b/unit/test-att.c
index 9d631fb..f208a45 100644
--- a/unit/test-att.c
+++ b/unit/test-att.c
@@ -204,6 +204,7 @@ struct request_test_data {
 	uint16_t req_size;
 	const  void *resp_data;
 	uint16_t resp_size;
+	bool (*compare_func)(const void *a, const void *b);
 };
 
 struct request_test {
@@ -259,6 +260,67 @@ static const struct request_test_data request_test_2 = {
 	.resp_size = sizeof(exchange_mtu_resp_bytes_2),
 };
 
+/* Find Information request <-> response */
+static bool compare_find_info_rsp(const void *a, const void *b)
+{
+	const struct bt_att_find_information_rsp_param *p1 = a;
+	const struct bt_att_find_information_rsp_param *p2 = b;
+
+	if (p1->format != p2->format || p1->length != p2->length)
+		return false;
+
+	return memcmp(p1->information_data, p2->information_data,
+							p2->length) == 0;
+}
+
+static const unsigned char find_info_req_bytes[] = {
+	0x04, 0x01, 0x00, 0xff, 0xff
+};
+static const struct bt_att_find_information_req_param find_info_req_param = {
+	.start_handle = 0x0001,
+	.end_handle = 0xffff,
+};
+static const unsigned char find_info_resp_bytes_1[] = {
+	0x05, 0x01, 0x01, 0x00, 0x0d, 0x18
+};
+static const uint8_t find_info_resp_information_data_1[] = {
+	0x01, 0x00, 0x0d, 0x18
+};
+static const struct bt_att_find_information_rsp_param find_info_rsp_param = {
+	.format = 0x01,
+	.information_data = find_info_resp_information_data_1,
+	.length = sizeof(find_info_resp_information_data_1),
+};
+static const struct request_test_data request_test_3 = {
+	.req_opcode = BT_ATT_OP_FIND_INFORMATION_REQ,
+	.req_param = &find_info_req_param,
+	.req_param_length = sizeof(find_info_req_param),
+	.resp_opcode = BT_ATT_OP_FIND_INFORMATION_RESP,
+	.resp_param = &find_info_rsp_param,
+	.resp_param_length = sizeof(find_info_rsp_param),
+	.req_data = find_info_req_bytes,
+	.req_size = sizeof(find_info_req_bytes),
+	.resp_data = find_info_resp_bytes_1,
+	.resp_size = sizeof(find_info_resp_bytes_1),
+	.compare_func = compare_find_info_rsp,
+};
+
+/* Find Information request <-> invalid response */
+static const unsigned char find_info_resp_bytes_2[] = {
+	0x05, 0x01
+};
+
+static const struct request_test_data request_test_4 = {
+	.req_opcode = BT_ATT_OP_FIND_INFORMATION_REQ,
+	.req_param = &find_info_req_param,
+	.req_param_length = sizeof(find_info_req_param),
+	.resp_opcode = BT_ATT_OP_ERROR_RESP,
+	.req_data = find_info_req_bytes,
+	.req_size = sizeof(find_info_req_bytes),
+	.resp_data = find_info_resp_bytes_2,
+	.resp_size = sizeof(find_info_resp_bytes_2),
+};
+
 static void test_request_callback(uint8_t opcode, const void *param,
 					uint16_t length, void *user_data)
 {
@@ -268,7 +330,16 @@ static void test_request_callback(uint8_t opcode, const void *param,
 	g_assert(opcode == test_data->resp_opcode);
 	g_assert(length == test_data->resp_param_length);
 
-	g_assert(0 == memcmp(param, test_data->resp_param, length));
+	if (length == 0 || !param) {
+		g_assert(length == 0 && !param);
+		context_quit(test->context);
+		return;
+	}
+
+	if (test_data->compare_func)
+		g_assert(test_data->compare_func(param, test_data->resp_param));
+	else
+		g_assert(0 == memcmp(param, test_data->resp_param, length));
 
 	context_quit(test->context);
 }
@@ -303,6 +374,8 @@ int main(int argc, char *argv[])
 
 	g_test_add_data_func("/att/request/1", &request_test_1, test_request);
 	g_test_add_data_func("/att/request/2", &request_test_2, test_request);
+	g_test_add_data_func("/att/request/3", &request_test_3, test_request);
+	g_test_add_data_func("/att/request/4", &request_test_4, test_request);
 
 	return g_test_run();
 }
-- 
1.8.3.2

Re: [PATCH v2 06/10] unit/test-att: Add unit test for "Find Information" request/response.

[Marcel Holtmann]

Hi Arman,

> Added test case for the "Find Information" request/response.
> ---
> unit/test-att.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/unit/test-att.c b/unit/test-att.c
> index 9d631fb..f208a45 100644
> --- a/unit/test-att.c
> +++ b/unit/test-att.c
> @@ -204,6 +204,7 @@ struct request_test_data {
> 	uint16_t req_size;
> 	const  void *resp_data;
> 	uint16_t resp_size;
> +	bool (*compare_func)(const void *a, const void *b);
> };
> 
> struct request_test {
> @@ -259,6 +260,67 @@ static const struct request_test_data request_test_2 = {
> 	.resp_size = sizeof(exchange_mtu_resp_bytes_2),
> };
> 
> +/* Find Information request <-> response */
> +static bool compare_find_info_rsp(const void *a, const void *b)
> +{
> +	const struct bt_att_find_information_rsp_param *p1 = a;
> +	const struct bt_att_find_information_rsp_param *p2 = b;
> +
> +	if (p1->format != p2->format || p1->length != p2->length)
> +		return false;
> +
> +	return memcmp(p1->information_data, p2->information_data,
> +							p2->length) == 0;

	return !memcmp().

> +}
> +
> +static const unsigned char find_info_req_bytes[] = {
> +	0x04, 0x01, 0x00, 0xff, 0xff
> +};
> +static const struct bt_att_find_information_req_param find_info_req_param = {
> +	.start_handle = 0x0001,
> +	.end_handle = 0xffff,
> +};
> +static const unsigned char find_info_resp_bytes_1[] = {
> +	0x05, 0x01, 0x01, 0x00, 0x0d, 0x18
> +};
> +static const uint8_t find_info_resp_information_data_1[] = {
> +	0x01, 0x00, 0x0d, 0x18
> +};
> +static const struct bt_att_find_information_rsp_param find_info_rsp_param = {
> +	.format = 0x01,
> +	.information_data = find_info_resp_information_data_1,
> +	.length = sizeof(find_info_resp_information_data_1),
> +};
> +static const struct request_test_data request_test_3 = {
> +	.req_opcode = BT_ATT_OP_FIND_INFORMATION_REQ,
> +	.req_param = &find_info_req_param,
> +	.req_param_length = sizeof(find_info_req_param),
> +	.resp_opcode = BT_ATT_OP_FIND_INFORMATION_RESP,
> +	.resp_param = &find_info_rsp_param,
> +	.resp_param_length = sizeof(find_info_rsp_param),
> +	.req_data = find_info_req_bytes,
> +	.req_size = sizeof(find_info_req_bytes),
> +	.resp_data = find_info_resp_bytes_1,
> +	.resp_size = sizeof(find_info_resp_bytes_1),
> +	.compare_func = compare_find_info_rsp,
> +};
> +
> +/* Find Information request <-> invalid response */
> +static const unsigned char find_info_resp_bytes_2[] = {
> +	0x05, 0x01
> +};
> +
> +static const struct request_test_data request_test_4 = {
> +	.req_opcode = BT_ATT_OP_FIND_INFORMATION_REQ,
> +	.req_param = &find_info_req_param,
> +	.req_param_length = sizeof(find_info_req_param),
> +	.resp_opcode = BT_ATT_OP_ERROR_RESP,
> +	.req_data = find_info_req_bytes,
> +	.req_size = sizeof(find_info_req_bytes),
> +	.resp_data = find_info_resp_bytes_2,
> +	.resp_size = sizeof(find_info_resp_bytes_2),
> +};
> +
> static void test_request_callback(uint8_t opcode, const void *param,
> 					uint16_t length, void *user_data)
> {
> @@ -268,7 +330,16 @@ static void test_request_callback(uint8_t opcode, const void *param,
> 	g_assert(opcode == test_data->resp_opcode);
> 	g_assert(length == test_data->resp_param_length);
> 
> -	g_assert(0 == memcmp(param, test_data->resp_param, length));
> +	if (length == 0 || !param) {
> +		g_assert(length == 0 && !param);
> +		context_quit(test->context);
> +		return;
> +	}
> +
> +	if (test_data->compare_func)
> +		g_assert(test_data->compare_func(param, test_data->resp_param));
> +	else
> +		g_assert(0 == memcmp(param, test_data->resp_param, length));

Never ever 0 == blah. I really dislike that syntax. Also here !memcmp() please.

> 
> 	context_quit(test->context);
> }
> @@ -303,6 +374,8 @@ int main(int argc, char *argv[])
> 
> 	g_test_add_data_func("/att/request/1", &request_test_1, test_request);
> 	g_test_add_data_func("/att/request/2", &request_test_2, test_request);
> +	g_test_add_data_func("/att/request/3", &request_test_3, test_request);
> +	g_test_add_data_func("/att/request/4", &request_test_4, test_request);
> 
> 	return g_test_run();
> }

Regards

Marcel

[PATCH v2 07/10] tools/btatt: Add "find-information" command.

[Arman Uguray]

Added command for the "Find Information" request. Sample run:

$ btatt -d 00:02:5B:00:15:10 -v find-information 0x0001 0xffff
att: ATT command 0x04
att: < 04 01 00 ff ff
att: > 05 01 01 00 00 28 02 00 03 28 03 00 00 2a 04 00 03 28 05 00 01 2a
Find Information response:
	Format: 0x01
	Information Data:
		handle: 0x0001, uuid: 2800
		handle: 0x0002, uuid: 2803
		handle: 0x0003, uuid: 2a00
		handle: 0x0004, uuid: 2803
		handle: 0x0005, uuid: 2a01
---
 tools/btatt.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 129 insertions(+), 2 deletions(-)

diff --git a/tools/btatt.c b/tools/btatt.c
index da1eb3d..f65f4ea 100644
--- a/tools/btatt.c
+++ b/tools/btatt.c
@@ -34,6 +34,8 @@
 #include <bluetooth/l2cap.h>
 
 #include "monitor/mainloop.h"
+#include "lib/uuid.h"
+#include "src/shared/util.h"
 #include "src/shared/att.h"
 
 #define ATT_CID 4
@@ -64,7 +66,7 @@ static void exchange_mtu_cb(uint8_t opcode, const void *param,
 	}
 
 	if (opcode != BT_ATT_OP_EXCHANGE_MTU_RESP) {
-		fprintf(stderr, "Invalid response opcode (%u)\n", opcode);
+		fprintf(stderr, "Invalid response opcode (0x%02x)\n", opcode);
 		goto done;
 	}
 
@@ -132,6 +134,130 @@ static void cmd_mtu(struct bt_att *att, int argc, char **argv)
 	}
 }
 
+static void find_info_cb(uint8_t opcode, const void *param,
+						uint16_t len, void *user_data)
+{
+	const struct bt_att_find_information_rsp_param *rp;
+	bt_uuid_t uuid;
+	char uuidstr[MAX_LEN_UUID_STR];
+	int pair_count, pair_size;
+	uint128_t u128;
+	const uint8_t *data_ptr;
+	int i;
+
+	if (opcode == BT_ATT_OP_ERROR_RESP) {
+		handle_error(param, len);
+		goto done;
+	}
+
+	if (opcode != BT_ATT_OP_FIND_INFORMATION_RESP) {
+		fprintf(stderr, "Invalid response opcode (0x%02x)\n", opcode);
+		goto done;
+	}
+
+	rp = param;
+
+	if (len != sizeof(*rp)) {
+		fprintf(stderr, "Invalid \"Find Information\" response length "
+								"(%u)\n", len);
+		goto done;
+	}
+
+	pair_size = 2;
+	if (rp->format == 0x01)
+		pair_size += 2;
+	else if (rp->format == 0x02)
+		pair_size += 16;
+	else {
+		fprintf(stderr, "Invalid \"Find Information\" response format "
+						"(0x%02x)\n", rp->format);
+		goto done;
+	}
+
+	pair_count = rp->length / pair_size;
+
+	printf("Find Information response:\n" "\tFormat: 0x%02x\n", rp->format);
+	printf("\tInformation Data:\n");
+
+	data_ptr = rp->information_data;
+	for (i = 0; i < pair_count; i++) {
+		printf("\t\thandle: 0x%04x", get_le16(data_ptr));
+
+		if (rp->format == 0x01) {
+			bt_uuid16_create(&uuid, get_le16(data_ptr + 2));
+		} else {
+			bswap_128(data_ptr + 2, &u128);
+			bt_uuid128_create(&uuid, u128);
+		}
+
+		bt_uuid_to_string(&uuid, uuidstr, MAX_LEN_UUID_STR);
+		printf(", uuid: %s\n", uuidstr);
+
+		data_ptr += pair_size;
+	}
+
+done:
+	mainloop_quit();
+}
+
+static void find_info_usage(void)
+{
+	printf("Usage: btatt find-information <start handle> <end handle>\n");
+}
+
+static struct option find_info_options[] = {
+	{ "help", 	0, 0, 'h'},
+	{}
+};
+
+static void cmd_find_info(struct bt_att *att, int argc, char **argv)
+{
+	struct bt_att_find_information_req_param param;
+	uint16_t start, end;
+	int opt;
+
+	while ((opt = getopt_long(argc, argv, "+h", find_info_options,
+								NULL)) != -1) {
+		switch (opt) {
+		case 'h':
+		default:
+			find_info_usage();
+			exit(EXIT_SUCCESS);
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+	optind = 0;
+
+	if (argc < 2) {
+		find_info_usage();
+		exit(EXIT_FAILURE);
+	}
+
+	start = strtol(argv[0], NULL, 0);
+	if (!start) {
+		fprintf(stderr, "Invalid start handle: %s\n", argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	end = strtol(argv[1], NULL, 0);
+	if (!end) {
+		fprintf(stderr, "Invalid end handle: %s\n", argv[1]);
+		exit(EXIT_FAILURE);
+	}
+
+	memset(&param, 0, sizeof(param));
+	param.start_handle = start;
+	param.end_handle = end;
+
+	if (bt_att_send_sequential(att, BT_ATT_OP_FIND_INFORMATION_REQ, &param,
+				sizeof(param), find_info_cb, NULL, NULL) == 0) {
+		fprintf(stderr, "Unable to send \"Find Information\" request\n");
+		exit(EXIT_FAILURE);
+	}
+}
+
 static int l2cap_le_att_connect(bdaddr_t *src, bdaddr_t *dst, uint8_t dst_type)
 {
 	int sock;
@@ -186,7 +312,8 @@ static struct {
 	void (*func)(struct bt_att *att, int argc, char **argv);
 	char *doc;
 } command[] = {
-	{ "exchange-mtu",	cmd_mtu,	"\"Exchange MTU\" request." },
+	{ "exchange-mtu", cmd_mtu, "\"Exchange MTU\" request" },
+	{ "find-information", cmd_find_info, "\"Find Information\" request" },
 	{ }
 };
 
-- 
1.8.3.2

[PATCH v2 08/10] src/shared/att: Add "Find By Type Value" request and response.

[Arman Uguray]

This patch adds support for the "Find By Type Value" request and
response with their corresponding parameter structures.
---
 src/shared/att.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/att.h | 16 +++++++++++
 2 files changed, 102 insertions(+)

diff --git a/src/shared/att.c b/src/shared/att.c
index f124731..23af33c 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -208,6 +208,11 @@ static bool handle_find_info_rsp(struct bt_att *att, const uint8_t *pdu,
 	struct bt_att_find_information_rsp_param param;
 	uint8_t *information_data;
 
+	/* PDU must contain at least the following:
+	 * - Attribute Opcode (1 octet)
+	 * - Format (1 octet)
+	 * - Information Data (4 to MTU-2 octets)
+	 */
 	if (pdu_length < 6)
 		return false;
 
@@ -233,6 +238,45 @@ static bool handle_find_info_rsp(struct bt_att *att, const uint8_t *pdu,
 	return true;
 }
 
+static bool handle_find_by_type_value_rsp(struct bt_att *att,
+							const uint8_t *pdu,
+							uint16_t pdu_length)
+{
+	struct bt_att_find_by_type_value_rsp_param param;
+	uint8_t *handles_info_list;
+
+	/* PDU must contain at least the following:
+	 * - Attribute Opcode (1 octet)
+	 * - Handles Information List (4 to MTU-1 octets)
+	 */
+	if (pdu_length < 5)
+		return false;
+
+	/* Each Handle Information field is composed of 4 octets. Return error,
+	 * if the length of the field is not a multiple of 4.
+	 */
+	if ((pdu_length - 1) % 4)
+		return false;
+
+	memset(&param, 0, sizeof(param));
+
+	param.length = pdu_length - 1;
+	handles_info_list = malloc(param.length);
+	if (!handles_info_list)
+		return false;
+
+	memcpy(handles_info_list, pdu + 1, param.length);
+
+	param.handles_information_list = handles_info_list;
+
+	request_complete(att, BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ, pdu[0], &param,
+								sizeof(param));
+
+	free(handles_info_list);
+
+	return true;
+}
+
 static bool handle_response(struct bt_att *att, const uint8_t *pdu,
 							uint16_t pdu_length)
 {
@@ -257,6 +301,13 @@ static bool handle_response(struct bt_att *att, const uint8_t *pdu,
 		}
 		return true;
 
+	case BT_ATT_OP_FIND_BY_TYPE_VALUE_RESP:
+		if (!handle_find_by_type_value_rsp(att, pdu, pdu_length)) {
+			req_opcode = BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ;
+			goto fail;
+		}
+		return true;
+
 	default:
 		break;
 	}
@@ -493,6 +544,37 @@ static bool encode_find_info_req(const void *param, uint16_t length, void *buf,
 	return true;
 }
 
+static bool encode_find_by_type_value_req(const void *param, uint16_t length,
+							void *buf, uint16_t mtu,
+							uint16_t *pdu_length)
+{
+	const struct bt_att_find_by_type_value_req_param *cp = param;
+	const uint16_t len =  7 + cp->length;
+	uint8_t *bytes = buf;
+
+	if (length != sizeof(*cp))
+		return false;
+
+	if (len > mtu)
+		return false;
+
+	put_le16(cp->start_handle, bytes + 1);
+	put_le16(cp->end_handle, bytes + 3);
+	put_le16(cp->type, bytes + 5);
+
+	*pdu_length = len;
+
+	if (cp->length == 0)
+		return true;
+
+	if (!cp->value)
+		return false;
+
+	memcpy(bytes + 7, cp->value, cp->length);
+
+	return true;
+}
+
 static bool encode_pdu(uint8_t opcode, const void *param, uint16_t length,
 				uint16_t mtu, void *pdu, uint16_t *pdu_length)
 {
@@ -521,6 +603,10 @@ static bool encode_pdu(uint8_t opcode, const void *param, uint16_t length,
 		return encode_find_info_req(param, length, bytes,
 							mtu, pdu_length);
 
+	if (opcode == BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ)
+		return encode_find_by_type_value_req(param, length, bytes,
+							mtu, pdu_length);
+
 	return false;
 }
 
diff --git a/src/shared/att.h b/src/shared/att.h
index 16d852d..8649c25 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -57,6 +57,22 @@ struct bt_att_find_information_rsp_param {
 	uint16_t length;
 };
 
+/* Find By Type Value */
+#define BT_ATT_OP_FIND_BY_TYPE_VALUE_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_VALUE_RESP	0x07
+struct bt_att_find_by_type_value_rsp_param {
+	const uint8_t *handles_information_list;
+	uint16_t length;
+};
+
 /* Error codes for Error response PDU */
 #define BT_ATT_ERROR_INVALID_HANDLE			0x01
 #define BT_ATT_ERROR_READ_NOT_PERMITTED			0x02
-- 
1.8.3.2

[PATCH v2 09/10] unit/test-att: Add unit test for "Find By Type Value" request/response.

[Arman Uguray]

Added test cases for the "Find By Type Value" request/response.
---
 unit/test-att.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 99 insertions(+), 6 deletions(-)

diff --git a/unit/test-att.c b/unit/test-att.c
index f208a45..b9c0bcf 100644
--- a/unit/test-att.c
+++ b/unit/test-att.c
@@ -202,7 +202,7 @@ struct request_test_data {
 	uint16_t resp_param_length;
 	const void *req_data;
 	uint16_t req_size;
-	const  void *resp_data;
+	const void *resp_data;
 	uint16_t resp_size;
 	bool (*compare_func)(const void *a, const void *b);
 };
@@ -235,11 +235,11 @@ static const struct request_test_data request_test_1 = {
 };
 
 /* Exchange MTU request <-> error response test */
-static const unsigned char exchange_mtu_req_bytes_2[] = { 0x02, 0x32, 0x00 };
+static const uint8_t exchange_mtu_req_bytes_2[] = { 0x02, 0x32, 0x00 };
 static const struct bt_att_exchange_mtu_req_param exchange_mtu_req_param_2 = {
 	.client_rx_mtu = 50,
 };
-static const unsigned char exchange_mtu_resp_bytes_2[] = {
+static const uint8_t exchange_mtu_resp_bytes_2[] = {
 	0x01, 0x02, 0x00, 0x00, 0x06
 };
 static const struct bt_att_error_rsp_param exchange_mtu_rsp_param_2 = {
@@ -273,14 +273,14 @@ static bool compare_find_info_rsp(const void *a, const void *b)
 							p2->length) == 0;
 }
 
-static const unsigned char find_info_req_bytes[] = {
+static const uint8_t find_info_req_bytes[] = {
 	0x04, 0x01, 0x00, 0xff, 0xff
 };
 static const struct bt_att_find_information_req_param find_info_req_param = {
 	.start_handle = 0x0001,
 	.end_handle = 0xffff,
 };
-static const unsigned char find_info_resp_bytes_1[] = {
+static const uint8_t find_info_resp_bytes_1[] = {
 	0x05, 0x01, 0x01, 0x00, 0x0d, 0x18
 };
 static const uint8_t find_info_resp_information_data_1[] = {
@@ -306,7 +306,7 @@ static const struct request_test_data request_test_3 = {
 };
 
 /* Find Information request <-> invalid response */
-static const unsigned char find_info_resp_bytes_2[] = {
+static const uint8_t find_info_resp_bytes_2[] = {
 	0x05, 0x01
 };
 
@@ -321,6 +321,96 @@ static const struct request_test_data request_test_4 = {
 	.resp_size = sizeof(find_info_resp_bytes_2),
 };
 
+/* Find By Type Value request <-> response */
+static bool compare_find_by_type_val_rsp(const void *a, const void *b)
+{
+	const struct bt_att_find_by_type_value_rsp_param *p1 = a;
+	const struct bt_att_find_by_type_value_rsp_param *p2 = b;
+
+	if (p1->length != p2->length)
+		return false;
+
+	return memcmp(p1->handles_information_list,
+				p2->handles_information_list, p2->length) == 0;
+}
+
+static const uint8_t find_by_type_val_req_bytes_1[] = {
+	0x06, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28, 0x01, 0x02, 0x03, 0x04
+};
+static const uint8_t find_by_type_val_req_att_value[] = {
+	0x01, 0x02, 0x03, 0x04
+};
+static const struct bt_att_find_by_type_value_req_param
+						find_by_type_val_rqp_1 = {
+	.start_handle = 0x0001,
+	.end_handle = 0xffff,
+	.type = 0x2800,
+	.value = find_by_type_val_req_att_value,
+	.length = sizeof(find_by_type_val_req_att_value),
+};
+static const uint8_t find_by_type_val_rsp_bytes_1[] = {
+	0x07, 0x01, 0x00, 0x02, 0x00, 0x02, 0x00, 0x03, 0x00
+};
+static const struct bt_att_find_by_type_value_rsp_param find_by_type_val_rpp = {
+	.handles_information_list = find_by_type_val_rsp_bytes_1 + 1,
+	.length = sizeof(find_by_type_val_rsp_bytes_1) - 1,
+};
+
+static const struct request_test_data request_test_5 = {
+	.req_opcode = BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ,
+	.req_param = &find_by_type_val_rqp_1,
+	.req_param_length = sizeof(find_by_type_val_rqp_1),
+	.resp_opcode = BT_ATT_OP_FIND_BY_TYPE_VALUE_RESP,
+	.resp_param = &find_by_type_val_rpp,
+	.resp_param_length = sizeof(find_by_type_val_rpp),
+	.req_data = find_by_type_val_req_bytes_1,
+	.req_size = sizeof(find_by_type_val_req_bytes_1),
+	.resp_data = find_by_type_val_rsp_bytes_1,
+	.resp_size = sizeof(find_by_type_val_rsp_bytes_1),
+	.compare_func = compare_find_by_type_val_rsp,
+};
+
+/* Find By Type Value request <-> invalid response */
+static const uint8_t find_by_type_val_rsp_bytes_2[] = {
+	0x07, 0x01, 0x00, 0x02
+};
+
+static const struct request_test_data request_test_6 = {
+	.req_opcode = BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ,
+	.req_param = &find_by_type_val_rqp_1,
+	.req_param_length = sizeof(find_by_type_val_rqp_1),
+	.resp_opcode = BT_ATT_OP_ERROR_RESP,
+	.req_data = find_by_type_val_req_bytes_1,
+	.req_size = sizeof(find_by_type_val_req_bytes_1),
+	.resp_data = find_by_type_val_rsp_bytes_2,
+	.resp_size = sizeof(find_by_type_val_rsp_bytes_2),
+};
+
+/* Find By Type Value request no value <-> response */
+static const uint8_t find_by_type_val_req_bytes_2[] = {
+	0x06, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28
+};
+static const struct bt_att_find_by_type_value_req_param
+						find_by_type_val_rqp_2 = {
+	.start_handle = 0x0001,
+	.end_handle = 0xffff,
+	.type = 0x2800,
+};
+
+static const struct request_test_data request_test_7 = {
+	.req_opcode = BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ,
+	.req_param = &find_by_type_val_rqp_2,
+	.req_param_length = sizeof(find_by_type_val_rqp_2),
+	.resp_opcode = BT_ATT_OP_FIND_BY_TYPE_VALUE_RESP,
+	.resp_param = &find_by_type_val_rpp,
+	.resp_param_length = sizeof(find_by_type_val_rpp),
+	.req_data = find_by_type_val_req_bytes_2,
+	.req_size = sizeof(find_by_type_val_req_bytes_2),
+	.resp_data = find_by_type_val_rsp_bytes_1,
+	.resp_size = sizeof(find_by_type_val_rsp_bytes_1),
+	.compare_func = compare_find_by_type_val_rsp,
+};
+
 static void test_request_callback(uint8_t opcode, const void *param,
 					uint16_t length, void *user_data)
 {
@@ -376,6 +466,9 @@ int main(int argc, char *argv[])
 	g_test_add_data_func("/att/request/2", &request_test_2, test_request);
 	g_test_add_data_func("/att/request/3", &request_test_3, test_request);
 	g_test_add_data_func("/att/request/4", &request_test_4, test_request);
+	g_test_add_data_func("/att/request/5", &request_test_5, test_request);
+	g_test_add_data_func("/att/request/6", &request_test_6, test_request);
+	g_test_add_data_func("/att/request/7", &request_test_7, test_request);
 
 	return g_test_run();
 }
-- 
1.8.3.2

[PATCH v2 10/10] tools/btatt: Add "find-by-type-value" command.

[Arman Uguray]

Added command for the "Find By Type Value" request. Sample runs:

$ btatt -d 00:02:5B:00:15:10 -v find-by-type-value 0x0001 0xffff
att: ATT command 0x06
att: < 06 01 00 ff ff 00 28
att: > 01 06 01 00 0a
Error response:
	opcode: 0x06
	handle: 0x0001
	error: 0x0a

$ btatt -d 00:02:5B:00:15:10 -v find-by-type-value 0x0001 0xffff 01 18
att: ATT command 0x06
att: < 06 01 00 ff ff 00 28 01 08
att: > 07 08 00 08 00
Find By Type Value response:
	Handle Information List:
		Attr Handle: 0x0008, Grp End Handle: 0x0008
---
 tools/btatt.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 162 insertions(+), 2 deletions(-)

diff --git a/tools/btatt.c b/tools/btatt.c
index f65f4ea..d1d8cfe 100644
--- a/tools/btatt.c
+++ b/tools/btatt.c
@@ -29,6 +29,7 @@
 #include <stdio.h>
 #include <getopt.h>
 #include <string.h>
+#include <errno.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/l2cap.h>
@@ -51,8 +52,11 @@ static void handle_error(const void *param, uint16_t len)
 		return;
 	}
 
-	printf("Request failed:\n\topcode:\t%u\n\thandle:\t%u\n\terror:\t%u\n",
-			rp->request_opcode, rp->handle, rp->error_code);
+	printf("Error response:\n"
+				"\topcode:\t0x%02x\n"
+				"\thandle:\t0x%04x\n"
+				"\terror:\t0x%02x\n",
+				rp->request_opcode, rp->handle, rp->error_code);
 }
 
 static void exchange_mtu_cb(uint8_t opcode, const void *param,
@@ -258,6 +262,160 @@ static void cmd_find_info(struct bt_att *att, int argc, char **argv)
 	}
 }
 
+static void find_by_type_val_cb(uint8_t opcode, const void *param,
+						uint16_t len, void *user_data)
+{
+	const struct bt_att_find_by_type_value_rsp_param *rp;
+	uint16_t att_handle, grp_handle;
+	const uint8_t *data_ptr;
+	int num_handle_infos;
+	int i;
+
+	if (opcode == BT_ATT_OP_ERROR_RESP) {
+		handle_error(param, len);
+		goto done;
+	}
+
+	if (opcode != BT_ATT_OP_FIND_BY_TYPE_VALUE_RESP) {
+		fprintf(stderr, "Invalid response opcode (0x%02x)\n", opcode);
+		goto done;
+	}
+
+	rp = param;
+
+	if (len != sizeof(*rp)) {
+		fprintf(stderr, "Invalid \"Find By Type Value\" response length"
+								" (%u)\n", len);
+		goto done;
+	}
+
+	if (rp->length == 0) {
+		fprintf(stderr, "\"Handle Info. List\" field is empty!\n");
+		goto done;
+	}
+
+	if (rp->length % 4) {
+		fprintf(stderr, "Malformed response\n");
+		goto done;
+	}
+
+	num_handle_infos = rp->length / 4;
+
+	printf("Find By Type Value response:\n");
+	printf("\tHandle Information List:\n");
+
+	data_ptr = rp->handles_information_list;
+	for (i = 0; i < num_handle_infos; i++) {
+		printf("\t\tAttr Handle: 0x%04x, Grp End Handle: 0x%04x\n",
+							get_le16(data_ptr),
+							get_le16(data_ptr + 2));
+
+		data_ptr += 4;
+	}
+
+done:
+	mainloop_quit();
+}
+
+static void find_by_type_val_usage(void)
+{
+	printf("Usage: btatt find-by-type-value <start handle> <end handle> "
+							"<uuid> <value>\n");
+
+	printf("e.g. btatt find-by-type-value 0x0001 0xFFFF 0x280C 00 01\n");
+}
+
+static struct option find_by_type_val_options[] = {
+	{ "help",	0, 0, 'h'},
+	{}
+};
+
+static void cmd_find_by_type_val(struct bt_att *att, int argc, char **argv)
+{
+	struct bt_att_find_by_type_value_req_param param;
+	uint16_t start, end, type, length;
+	uint8_t *value = NULL;
+	int opt;
+
+	while ((opt = getopt_long(argc, argv, "+h", find_by_type_val_options,
+								NULL)) != -1) {
+		switch (opt) {
+		case 'h':
+		default:
+			find_by_type_val_usage();
+			exit(EXIT_SUCCESS);
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	if (argc < 3) {
+		find_by_type_val_usage();
+		exit(EXIT_FAILURE);
+	}
+
+	start = strtol(argv[0], NULL, 0);
+	if (!start) {
+		fprintf(stderr, "Invalid start handle: %s\n", argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	end = strtol(argv[1], NULL, 0);
+	if (!end) {
+		fprintf(stderr, "Invalid end handle: %s\n", argv[1]);
+		exit(EXIT_FAILURE);
+	}
+
+	type = strtol(argv[2], NULL, 0);
+	if (!type) {
+		fprintf(stderr, "Invalid 16-bit UUID: %s\n", argv[2]);
+		exit(EXIT_FAILURE);
+	}
+
+	length = argc - 3;
+
+	if (length > 0) {
+		int i;
+
+		value = malloc(length);
+
+		for (i = 3; i < argc; i++) {
+			if (strlen(argv[i]) != 2) {
+				fprintf(stderr, "Invalid value byte: %s\n",
+								argv[i]);
+				free(value);
+				exit(EXIT_FAILURE);
+			}
+
+			value[i-3] = strtol(argv[i], NULL, 16);
+			if (errno) {
+				perror("Error parsing value");
+				free(value);
+				exit(EXIT_FAILURE);
+			}
+		}
+	}
+
+	memset(&param, 0, sizeof(param));
+	param.start_handle = start;
+	param.end_handle = end;
+	param.type = type;
+	param.value = value;
+	param.length = length;
+
+	if (!bt_att_send_sequential(att, BT_ATT_OP_FIND_BY_TYPE_VALUE_REQ,
+					&param, sizeof(param),
+					find_by_type_val_cb, NULL, NULL)) {
+		fprintf(stderr, "Unable to send \"Find By Type Value\" "
+								"request\n");
+		free(value);
+		exit(EXIT_FAILURE);
+	}
+
+	free(value);
+}
+
 static int l2cap_le_att_connect(bdaddr_t *src, bdaddr_t *dst, uint8_t dst_type)
 {
 	int sock;
@@ -314,6 +472,8 @@ static struct {
 } command[] = {
 	{ "exchange-mtu", cmd_mtu, "\"Exchange MTU\" request" },
 	{ "find-information", cmd_find_info, "\"Find Information\" request" },
+	{ "find-by-type-value", cmd_find_by_type_val,
+						"\"Find By Type Value\" request" },
 	{ }
 };
 
-- 
1.8.3.2