[PATCH BlueZ 1/4] shared/gatt-client: Simplify bt_gatt_client_new

[PATCH BlueZ 1/4] shared/gatt-client: Simplify bt_gatt_client_new

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This split bt_gatt_client_unref into bt_gatt_client_free so it can be
used within bt_gatt_client_new when freeing the data.
---
 src/shared/gatt-client.c | 87 +++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 49 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 6dc8e95..782e6b3 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1211,6 +1211,29 @@ static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
 	bt_gatt_client_unref(client);
 }
 
+static void long_write_op_unref(void *data);
+
+static void bt_gatt_client_free(struct bt_gatt_client *client)
+{
+	if (client->ready_destroy)
+		client->ready_destroy(client->ready_data);
+
+	if (client->debug_destroy)
+		client->debug_destroy(client->debug_data);
+
+	bt_att_unregister(client->att, client->notify_id);
+	bt_att_unregister(client->att, client->ind_id);
+
+	queue_destroy(client->svc_chngd_queue, free);
+	queue_destroy(client->long_write_queue, long_write_op_unref);
+	queue_destroy(client->notify_list, notify_data_unref);
+
+	gatt_client_clear_services(client);
+
+	bt_att_unref(client->att);
+	free(client);
+}
+
 struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 {
 	struct bt_gatt_client *client;
@@ -1223,52 +1246,36 @@ struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 		return NULL;
 
 	client->long_write_queue = queue_new();
-	if (!client->long_write_queue) {
-		free(client);
-		return NULL;
-	}
+	if (!client->long_write_queue)
+		goto fail;
 
 	client->svc_chngd_queue = queue_new();
-	if (!client->svc_chngd_queue) {
-		queue_destroy(client->long_write_queue, NULL);
-		free(client);
-		return NULL;
-	}
+	if (!client->svc_chngd_queue)
+		goto fail;
 
 	client->notify_list = queue_new();
-	if (!client->notify_list) {
-		queue_destroy(client->svc_chngd_queue, NULL);
-		queue_destroy(client->long_write_queue, NULL);
-		free(client);
-		return NULL;
-	}
+	if (!client->notify_list)
+		goto fail;
 
 	client->notify_id = bt_att_register(att, BT_ATT_OP_HANDLE_VAL_NOT,
 						notify_cb, client, NULL);
-	if (!client->notify_id) {
-		queue_destroy(client->notify_list, NULL);
-		queue_destroy(client->svc_chngd_queue, NULL);
-		queue_destroy(client->long_write_queue, NULL);
-		free(client);
-		return NULL;
-	}
+	if (!client->notify_id)
+		goto fail;
 
 	client->ind_id = bt_att_register(att, BT_ATT_OP_HANDLE_VAL_IND,
 						notify_cb, client, NULL);
-	if (!client->ind_id) {
-		bt_att_unregister(att, client->notify_id);
-		queue_destroy(client->notify_list, NULL);
-		queue_destroy(client->svc_chngd_queue, NULL);
-		queue_destroy(client->long_write_queue, NULL);
-		free(client);
-		return NULL;
-	}
+	if (!client->ind_id)
+		goto fail;
 
 	client->att = bt_att_ref(att);
 
 	gatt_client_init(client, mtu);
 
 	return bt_gatt_client_ref(client);
+
+fail:
+	bt_gatt_client_free(client);
+	return NULL;
 }
 
 struct bt_gatt_client *bt_gatt_client_ref(struct bt_gatt_client *client)
@@ -1281,8 +1288,6 @@ struct bt_gatt_client *bt_gatt_client_ref(struct bt_gatt_client *client)
 	return client;
 }
 
-static void long_write_op_unref(void *data);
-
 void bt_gatt_client_unref(struct bt_gatt_client *client)
 {
 	if (!client)
@@ -1291,23 +1296,7 @@ void bt_gatt_client_unref(struct bt_gatt_client *client)
 	if (__sync_sub_and_fetch(&client->ref_count, 1))
 		return;
 
-	if (client->ready_destroy)
-		client->ready_destroy(client->ready_data);
-
-	if (client->debug_destroy)
-		client->debug_destroy(client->debug_data);
-
-	bt_att_unregister(client->att, client->notify_id);
-	bt_att_unregister(client->att, client->ind_id);
-
-	queue_destroy(client->svc_chngd_queue, free);
-	queue_destroy(client->long_write_queue, long_write_op_unref);
-	queue_destroy(client->notify_list, notify_data_unref);
-
-	gatt_client_clear_services(client);
-
-	bt_att_unref(client->att);
-	free(client);
+	bt_gatt_client_free(client);
 }
 
 bool bt_gatt_client_is_ready(struct bt_gatt_client *client)
-- 
1.9.3

[PATCH BlueZ 2/4] shared/gatt-client: Take fd in bt_gatt_client_new

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This simplify the instance creation and also drop automatically the
reference in case of disconnection.
---
 src/shared/gatt-client.c | 52 +++++++++++++++++++++++++++++++++++++-----------
 src/shared/gatt-client.h |  3 ++-
 tools/btgatt-client.c    | 29 ++++++---------------------
 3 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 782e6b3..d884185 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -104,7 +104,7 @@ struct bt_gatt_client {
 	/* List of registered notification/indication callbacks */
 	struct queue *notify_list;
 	int next_reg_id;
-	unsigned int notify_id, ind_id;
+	unsigned int disc_id, notify_id, ind_id;
 	bool in_notify;
 	bool need_notify_cleanup;
 
@@ -1221,8 +1221,12 @@ static void bt_gatt_client_free(struct bt_gatt_client *client)
 	if (client->debug_destroy)
 		client->debug_destroy(client->debug_data);
 
-	bt_att_unregister(client->att, client->notify_id);
-	bt_att_unregister(client->att, client->ind_id);
+	if (client->att) {
+		bt_att_unregister_disconnect(client->att, client->disc_id);
+		bt_att_unregister(client->att, client->notify_id);
+		bt_att_unregister(client->att, client->ind_id);
+		bt_att_unref(client->att);
+	}
 
 	queue_destroy(client->svc_chngd_queue, free);
 	queue_destroy(client->long_write_queue, long_write_op_unref);
@@ -1230,21 +1234,38 @@ static void bt_gatt_client_free(struct bt_gatt_client *client)
 
 	gatt_client_clear_services(client);
 
-	bt_att_unref(client->att);
 	free(client);
 }
 
-struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
+static void att_disconnect_cb(void *user_data)
 {
-	struct bt_gatt_client *client;
+	struct bt_gatt_client *client = user_data;
 
-	if (!att)
-		return NULL;
+	bt_att_unref(client->att);
+	client->att = NULL;
+}
+
+struct bt_gatt_client *bt_gatt_client_new(int fd, uint16_t mtu)
+{
+	struct bt_gatt_client *client;
 
 	client = new0(struct bt_gatt_client, 1);
 	if (!client)
 		return NULL;
 
+	client->att = bt_att_new(fd);
+	if (!client->att)
+		goto fail;
+
+	if (!bt_att_set_close_on_unref(client->att, true))
+		goto fail;
+
+	client->disc_id = bt_att_register_disconnect(client->att,
+							att_disconnect_cb,
+							client, NULL);
+	if (!client->disc_id)
+		goto fail;
+
 	client->long_write_queue = queue_new();
 	if (!client->long_write_queue)
 		goto fail;
@@ -1257,18 +1278,17 @@ struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 	if (!client->notify_list)
 		goto fail;
 
-	client->notify_id = bt_att_register(att, BT_ATT_OP_HANDLE_VAL_NOT,
+	client->notify_id = bt_att_register(client->att,
+						BT_ATT_OP_HANDLE_VAL_NOT,
 						notify_cb, client, NULL);
 	if (!client->notify_id)
 		goto fail;
 
-	client->ind_id = bt_att_register(att, BT_ATT_OP_HANDLE_VAL_IND,
+	client->ind_id = bt_att_register(client->att, BT_ATT_OP_HANDLE_VAL_IND,
 						notify_cb, client, NULL);
 	if (!client->ind_id)
 		goto fail;
 
-	client->att = bt_att_ref(att);
-
 	gatt_client_init(client, mtu);
 
 	return bt_gatt_client_ref(client);
@@ -1278,6 +1298,14 @@ fail:
 	return NULL;
 }
 
+struct bt_att *bt_gatt_client_get_att(struct bt_gatt_client *client)
+{
+	if (!client)
+		return NULL;
+
+	return client->att;
+}
+
 struct bt_gatt_client *bt_gatt_client_ref(struct bt_gatt_client *client)
 {
 	if (!client)
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 6807f6b..4e8da29 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -38,7 +38,8 @@
 
 struct bt_gatt_client;
 
-struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu);
+struct bt_gatt_client *bt_gatt_client_new(int fd, uint16_t mtu);
+struct bt_att *bt_gatt_client_get_att(struct bt_gatt_client *client);
 
 struct bt_gatt_client *bt_gatt_client_ref(struct bt_gatt_client *client);
 void bt_gatt_client_unref(struct bt_gatt_client *client);
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d900e08..95d395d 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -107,33 +107,19 @@ static struct client *client_create(int fd, uint16_t mtu)
 		return NULL;
 	}
 
-	att = bt_att_new(fd);
-	if (!att) {
-		fprintf(stderr, "Failed to initialze ATT transport layer\n");
-		bt_att_unref(att);
+	cli->fd = fd;
+	cli->gatt = bt_gatt_client_new(fd, mtu);
+	if (!cli->gatt) {
+		fprintf(stderr, "Failed to create GATT client\n");
 		free(cli);
 		return NULL;
 	}
 
-	if (!bt_att_set_close_on_unref(att, true)) {
-		fprintf(stderr, "Failed to set up ATT transport layer\n");
-		bt_att_unref(att);
-		free(cli);
-		return NULL;
-	}
+	att = bt_gatt_client_get_att(cli->gatt);
 
 	if (!bt_att_register_disconnect(att, att_disconnect_cb, NULL, NULL)) {
 		fprintf(stderr, "Failed to set ATT disconnect handler\n");
-		bt_att_unref(att);
-		free(cli);
-		return NULL;
-	}
-
-	cli->fd = fd;
-	cli->gatt = bt_gatt_client_new(att, mtu);
-	if (!cli->gatt) {
-		fprintf(stderr, "Failed to create GATT client\n");
-		bt_att_unref(att);
+		bt_gatt_client_unref(cli->gatt);
 		free(cli);
 		return NULL;
 	}
@@ -148,9 +134,6 @@ static struct client *client_create(int fd, uint16_t mtu)
 	bt_gatt_client_set_service_changed(cli->gatt, service_changed_cb, cli,
 									NULL);
 
-	/* bt_gatt_client already holds a reference */
-	bt_att_unref(att);
-
 	return cli;
 }
 
-- 
1.9.3

[PATCH BlueZ 3/4] unit/test-gatt: Add TP/GAC/CL/BV-01-C test

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Verify that a Generic Attribute Profile client can generate an
Exchange MTU Request command.
---
 Makefile.am      |   7 ++
 unit/test-gatt.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 267 insertions(+)
 create mode 100644 unit/test-gatt.c

diff --git a/Makefile.am b/Makefile.am
index ebd5b44..eba594d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -363,6 +363,13 @@ unit_tests += unit/test-lib
 unit_test_lib_SOURCES = unit/test-lib.c
 unit_test_lib_LDADD = lib/libbluetooth-internal.la @GLIB_LIBS@
 
+unit_tests += unit/test-gatt
+
+unit_test_gatt_SOURCES = unit/test-gatt.c
+unit_test_gatt_LDADD = src/libshared-glib.la lib/libbluetooth-internal.la \
+			@GLIB_LIBS@
+
+
 noinst_PROGRAMS += $(unit_tests)
 
 TESTS = $(unit_tests)
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
new file mode 100644
index 0000000..6b8a229
--- /dev/null
+++ b/unit/test-gatt.c
@@ -0,0 +1,260 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Intel Corporation. All rights reserved.
+ *
+ *
+ *  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 <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <inttypes.h>
+#include <string.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+
+#include <glib.h>
+
+#include "src/shared/util.h"
+#include "src/shared/gatt-client.h"
+
+struct test_pdu {
+	bool valid;
+	const uint8_t *data;
+	size_t size;
+};
+
+struct test_data {
+	char *test_name;
+	struct test_pdu *pdu_list;
+};
+
+struct context {
+	GMainLoop *main_loop;
+	struct bt_gatt_client *client;
+	guint source;
+	guint process;
+	int fd;
+	unsigned int pdu_offset;
+	const struct test_data *data;
+};
+
+#define data(args...) ((const unsigned char[]) { args })
+
+#define raw_pdu(args...)					\
+	{							\
+		.valid = true,					\
+		.data = data(args),				\
+		.size = sizeof(data(args)),			\
+	}
+
+#define define_test(name, function, args...)				\
+	do {								\
+		const struct test_pdu pdus[] = {			\
+			args, { }					\
+		};							\
+		static struct test_data data;				\
+		data.test_name = g_strdup(name);			\
+		data.pdu_list = g_malloc(sizeof(pdus));			\
+		memcpy(data.pdu_list, pdus, sizeof(pdus));		\
+		g_test_add_data_func(name, &data, function);		\
+	} while (0)
+
+static void test_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	g_print("%s%s\n", prefix, str);
+}
+
+static void test_free(gconstpointer user_data)
+{
+	const struct test_data *data = user_data;
+
+	g_free(data->test_name);
+	g_free(data->pdu_list);
+}
+
+static gboolean context_quit(gpointer user_data)
+{
+	struct context *context = user_data;
+
+	if (context->process > 0)
+		g_source_remove(context->process);
+
+	g_main_loop_quit(context->main_loop);
+
+	return FALSE;
+}
+
+static gboolean send_pdu(gpointer user_data)
+{
+	struct context *context = user_data;
+	const struct test_pdu *pdu;
+	ssize_t len;
+
+	pdu = &context->data->pdu_list[context->pdu_offset++];
+
+	len = write(context->fd, pdu->data, pdu->size);
+
+	if (g_test_verbose())
+		util_hexdump('<', pdu->data, len, test_debug, "GATT: ");
+
+	g_assert_cmpint(len, ==, pdu->size);
+
+	context->process = 0;
+	return FALSE;
+}
+
+static void context_process(struct context *context)
+{
+	if (!context->data->pdu_list[context->pdu_offset].valid) {
+		context_quit(context);
+		return;
+	}
+
+	context->process = g_idle_add(send_pdu, context);
+}
+
+static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct context *context = user_data;
+	const struct test_pdu *pdu;
+	unsigned char buf[512];
+	ssize_t len;
+	int fd;
+
+	pdu = &context->data->pdu_list[context->pdu_offset++];
+
+	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
+		context->source = 0;
+		g_print("%s: cond %x\n", __func__, cond);
+		return FALSE;
+	}
+
+	fd = g_io_channel_unix_get_fd(channel);
+
+	len = read(fd, buf, sizeof(buf));
+
+	g_assert(len > 0);
+
+	if (g_test_verbose())
+		util_hexdump('>', buf, len, test_debug, "GATT: ");
+
+	g_assert_cmpint(len, ==, pdu->size);
+
+	g_assert(memcmp(buf, pdu->data, pdu->size) == 0);
+
+	context_process(context);
+
+	return TRUE;
+}
+
+static void gatt_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	g_print("%s%s\n", prefix, str);
+}
+
+static struct context *create_context(uint16_t mtu, gconstpointer data)
+{
+	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);
+
+	context->client = bt_gatt_client_new(sv[0], mtu);
+	g_assert(context->client != NULL);
+
+	if (g_test_verbose())
+		bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:",
+									NULL);
+
+	channel = g_io_channel_unix_new(sv[1]);
+
+	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->source = g_io_add_watch(channel,
+				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				test_handler, context);
+	g_assert(context->source > 0);
+
+	g_io_channel_unref(channel);
+
+	context->fd = sv[1];
+	context->data = data;
+
+	return context;
+}
+
+static void destroy_context(struct context *context)
+{
+	if (context->source > 0)
+		g_source_remove(context->source);
+
+	bt_gatt_client_unref(context->client);
+
+	g_main_loop_unref(context->main_loop);
+
+	test_free(context->data);
+	g_free(context);
+}
+
+static void execute_context(struct context *context)
+{
+	g_main_loop_run(context->main_loop);
+
+	destroy_context(context);
+}
+
+static void test_client(gconstpointer data)
+{
+	struct context *context = create_context(512, data);
+
+	execute_context(context);
+}
+
+int main(int argc, char *argv[])
+{
+	g_test_init(&argc, &argv, NULL);
+
+	/*
+	 * Server Configuration
+	 *
+	 * The test group objective is to verify Generic Attribute Profile
+	 * Server Configuration.
+	 */
+	define_test("/TP/GAC/CL/BV-01-C", test_client,
+				raw_pdu(0x02, 0x00, 0x02));
+
+	return g_test_run();
+}
-- 
1.9.3

[PATCH BlueZ 4/4] shared/gatt-client: Fix crash on bt_gatt_client_unref

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Calling gatt_client_clear_services after notify_list is destroyed cause
the following backtrace:

Invalid read of size 8
   at 0x404CC9: queue_remove_all (queue.c:312)
   by 0x401FC6: gatt_client_remove_all_notify_in_range (gatt-client.c:350)
   by 0x403170: bt_gatt_client_free (gatt-client.c:357)
   by 0x401A93: test_client (test-gatt.c:224)
   by 0x4E9E5E0: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9E7A5: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9E7A5: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9E7A5: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9EB1A: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4015EE: main (test-gatt.c:259)
 Address 0x5752718 is 8 bytes inside a block of size 32 free'd
   at 0x4C28577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x40315E: bt_gatt_client_free (gatt-client.c:1233)
   by 0x401A93: test_client (test-gatt.c:224)
   by 0x4E9E5E0: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9E7A5: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9E7A5: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9E7A5: ??? (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4E9EB1A: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.3800.2)
   by 0x4015EE: main (test-gatt.c:259)
---
 src/shared/gatt-client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index d884185..832b09b 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1228,12 +1228,12 @@ static void bt_gatt_client_free(struct bt_gatt_client *client)
 		bt_att_unref(client->att);
 	}
 
+	gatt_client_clear_services(client);
+
 	queue_destroy(client->svc_chngd_queue, free);
 	queue_destroy(client->long_write_queue, long_write_op_unref);
 	queue_destroy(client->notify_list, notify_data_unref);
 
-	gatt_client_clear_services(client);
-
 	free(client);
 }
 
-- 
1.9.3

Re: [PATCH BlueZ 2/4] shared/gatt-client: Take fd in bt_gatt_client_new

[Johan Hedberg]

Hi Luiz,

On Wed, Oct 01, 2014, Luiz Augusto von Dentz wrote:
> -struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
> +struct bt_gatt_client *bt_gatt_client_new(int fd, uint16_t mtu)

This becomes a bit cumbersome since pretty much any time you have a
connected ATT socket you must be able to be both client and server in
order not to exhibit broken behavior.

With the above change you'd need something like the following since the
fd should really only be processed in one place (bt_att).

	client = client_new(fd);
	att = client_get_att(client);
	server = server_new(att);

Without your change I assume the code has been something like:

	att = att_new(fd);
	client = client_new(att);
	server = server_new(att);

The above is fine except I don't really see the point in having separate
client and server objects to begin with. I might have missed the
reasoning behind it (in which case please enlighten me), but why don't
we hide both roles behind the same object, since they're anyway tied to
the same connection?

I have no objections to having clearly name-spaced functions for client
and server side behavior, and even keeping these in separate c-files,
but having independent objects for them seems a bit pointless. So why
couldn't we have server_foo(att) and client_foo(att) while keeping just
this one "att" context?

Johan

Re: [PATCH BlueZ 2/4] shared/gatt-client: Take fd in bt_gatt_client_new

[Luiz Augusto von Dentz]

Hi Johan,

On Wed, Oct 1, 2014 at 3:34 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Luiz,
>
> On Wed, Oct 01, 2014, Luiz Augusto von Dentz wrote:
>> -struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
>> +struct bt_gatt_client *bt_gatt_client_new(int fd, uint16_t mtu)
>
> This becomes a bit cumbersome since pretty much any time you have a
> connected ATT socket you must be able to be both client and server in
> order not to exhibit broken behavior.
>
> With the above change you'd need something like the following since the
> fd should really only be processed in one place (bt_att).
>
>         client = client_new(fd);
>         att = client_get_att(client);
>         server = server_new(att);
>
> Without your change I assume the code has been something like:
>
>         att = att_new(fd);
>         client = client_new(att);
>         server = server_new(att);
>
> The above is fine except I don't really see the point in having separate
> client and server objects to begin with. I might have missed the
> reasoning behind it (in which case please enlighten me), but why don't
> we hide both roles behind the same object, since they're anyway tied to
> the same connection?
>
> I have no objections to having clearly name-spaced functions for client
> and server side behavior, and even keeping these in separate c-files,
> but having independent objects for them seems a bit pointless. So why
> couldn't we have server_foo(att) and client_foo(att) while keeping just
> this one "att" context?

I was thinking something similar, anyway I got to this point while
doing the unit tester based on the testing spec, which btw has both
client and server tests, then I realize I would have to managed bt_att
along with bt_gatt_client for no apparent reason.

So I do agree that it seems much simpler API to unify client and
server and let it self manage the ATT connection, we could even notify
disconnects via ready callback, well if we fix it because right now if
the remote does not respond or respond with something invalid it
doesn't seems to be called. For the server side it seems to be just a
matter of attaching the db, which could perhaps be passed in
bt_gatt_new or having dedicated bt_gatt_attach_db/bt_gatt_detach_cb.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/4] shared/gatt-client: Take fd in bt_gatt_client_new

[Arman Uguray]

Hi Luiz & Johan,


>> The above is fine except I don't really see the point in having separate
>> client and server objects to begin with. I might have missed the
>> reasoning behind it (in which case please enlighten me), but why don't
>> we hide both roles behind the same object, since they're anyway tied to
>> the same connection?
>>

I think eventually this is may be what we want but it seems more
natural to me for client and server roles to be in different objects,
at least internally. There's never an "overlap" between server and
client roles that I know of and these should be pretty much
orthogonal. I think eventually we can have a more global object that
owns the fd & att structure and owns the server and the client, but
it's more intuitive IMO if the server & client procedures are grouped
in their own modules. Apart from the transport, there's really nothing
that they'll share (afaik).


>> I have no objections to having clearly name-spaced functions for client
>> and server side behavior, and even keeping these in separate c-files,
>> but having independent objects for them seems a bit pointless. So why
>> couldn't we have server_foo(att) and client_foo(att) while keeping just
>> this one "att" context?

Having independent objects made sense initially, since the kind of
state they keep is mostly different. bt_gatt_server will mostly only
interact with gatt_db and bt_att, while bt_gatt_client stores a high
level client cache. And this isn't too bad from an implementation
stand-point (in fact it makes sense from an OO perspective) and it
would be quite ok for a higher-level bt_gatt object to create and own
these two.

In any case, these could in fact be unified eventually but for now
let's keep them separate and see how the server turns out without
overthinking it. I'm going to start sending out patches for the server
code soon, and once that starts taking shape, I think we can decide if
they should be unified or not.


> So I do agree that it seems much simpler API to unify client and
> server and let it self manage the ATT connection, we could even notify
> disconnects via ready callback, well if we fix it because right now if
> the remote does not respond or respond with something invalid it
> doesn't seems to be called.

Yeah this is actually a bug in the code. If the remote end doesn't
respond, then att.c:timeout_cb will be called eventually, which will
destroy the connection by destroying the io. It notifies this via a
timeout_callback to the upper layer but the disconnect_callback
doesn't automatically get called, since io simply closes the fd
instead of receiving an event (which is how the regular
disconnect_callback is currently invoked). I suppose this can be fixed
by calling all registered disconnect callbacks in att.c:timeout_cb.
I've been aware of this bug though never got around to fixing it :)

If the remote responds with something invalid, bt_att doesn't do
anything currently. If the opcode is bad, it simply drops it. We'll
have to figure out a way to correctly handle this, either at the
bt_att level or at the gatt level.


> ... For the server side it seems to be just a
> matter of attaching the db, which could perhaps be passed in
> bt_gatt_new or having dedicated bt_gatt_attach_db/bt_gatt_detach_cb.

So, the current idea is this:

   db = gatt_db_new();
   ... /* Populate DB */
   att = bt_att_new(fd);
   client = bt_gatt_client_new(att, mtu);
   server = bt_gatt_server_new(db, att, mtu);

All of this could be done by a higher-level bt_gatt object that puts
everything together but for now, let's not create bt_att inside
bt_gatt_client. I think Marcel's longer term idea is to put these all
inside an even higher-level bt_gap that basically puts SDP and GATT
together. (I saw some patches fly by for shared/gap).

I hope this all makes sense.

Cheers,
Arman

Re: [PATCH BlueZ 2/4] shared/gatt-client: Take fd in bt_gatt_client_new

[Luiz Augusto von Dentz]

Hi Arman,

On Wed, Oct 1, 2014 at 7:17 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz & Johan,
>
>
>>> The above is fine except I don't really see the point in having separate
>>> client and server objects to begin with. I might have missed the
>>> reasoning behind it (in which case please enlighten me), but why don't
>>> we hide both roles behind the same object, since they're anyway tied to
>>> the same connection?
>>>
>
> I think eventually this is may be what we want but it seems more
> natural to me for client and server roles to be in different objects,
> at least internally. There's never an "overlap" between server and
> client roles that I know of and these should be pretty much
> orthogonal. I think eventually we can have a more global object that
> owns the fd & att structure and owns the server and the client, but
> it's more intuitive IMO if the server & client procedures are grouped
> in their own modules. Apart from the transport, there's really nothing
> that they'll share (afaik).

Right, it might make the code more modular but also allow mistakes
such as creating 2 different instances of bt_att for handling client
and server separately.

>
>>> I have no objections to having clearly name-spaced functions for client
>>> and server side behavior, and even keeping these in separate c-files,
>>> but having independent objects for them seems a bit pointless. So why
>>> couldn't we have server_foo(att) and client_foo(att) while keeping just
>>> this one "att" context?
>
> Having independent objects made sense initially, since the kind of
> state they keep is mostly different. bt_gatt_server will mostly only
> interact with gatt_db and bt_att, while bt_gatt_client stores a high
> level client cache. And this isn't too bad from an implementation
> stand-point (in fact it makes sense from an OO perspective) and it
> would be quite ok for a higher-level bt_gatt object to create and own
> these two.
>
> In any case, these could in fact be unified eventually but for now
> let's keep them separate and see how the server turns out without
> overthinking it. I'm going to start sending out patches for the server
> code soon, and once that starts taking shape, I think we can decide if
> they should be unified or not.
>
>
>> So I do agree that it seems much simpler API to unify client and
>> server and let it self manage the ATT connection, we could even notify
>> disconnects via ready callback, well if we fix it because right now if
>> the remote does not respond or respond with something invalid it
>> doesn't seems to be called.
>
> Yeah this is actually a bug in the code. If the remote end doesn't
> respond, then att.c:timeout_cb will be called eventually, which will
> destroy the connection by destroying the io. It notifies this via a
> timeout_callback to the upper layer but the disconnect_callback
> doesn't automatically get called, since io simply closes the fd
> instead of receiving an event (which is how the regular
> disconnect_callback is currently invoked). I suppose this can be fixed
> by calling all registered disconnect callbacks in att.c:timeout_cb.
> I've been aware of this bug though never got around to fixing it :)

Actually we should probably notify a error response otherwise it may
call destroy which gives no hint what happened with the call itself,
but now I figure you actually have a timeout callback which can be
used but it is not per operation and I can only register one at time
so I cannot register one in gatt-client.c otherwise it may get
overwritten, perhaps we should add a timeout callback to bt_att_send
or just remove since disconnect callback should be called anyway and
that can be registered by gatt-client.c.

> If the remote responds with something invalid, bt_att doesn't do
> anything currently. If the opcode is bad, it simply drops it. We'll
> have to figure out a way to correctly handle this, either at the
> bt_att level or at the gatt level.
>
>
>> ... For the server side it seems to be just a
>> matter of attaching the db, which could perhaps be passed in
>> bt_gatt_new or having dedicated bt_gatt_attach_db/bt_gatt_detach_cb.
>
> So, the current idea is this:
>
>    db = gatt_db_new();
>    ... /* Populate DB */
>    att = bt_att_new(fd);
>    client = bt_gatt_client_new(att, mtu);
>    server = bt_gatt_server_new(db, att, mtu);
>
> All of this could be done by a higher-level bt_gatt object that puts
> everything together but for now, let's not create bt_att inside
> bt_gatt_client. I think Marcel's longer term idea is to put these all
> inside an even higher-level bt_gap that basically puts SDP and GATT
> together. (I saw some patches fly by for shared/gap).

Hmm, didn't know about this, anyway going all the way up to GAP seems
a little bit overkill but lets see what Marcel had in mind.

Btw, I resend the patches skipping this one, please check if they are
no breaking anything for you.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/4] shared/gatt-client: Take fd in bt_gatt_client_new

[Arman Uguray]

Hi Luiz,

> Right, it might make the code more modular but also allow mistakes
> such as creating 2 different instances of bt_att for handling client
> and server separately.
>

Yeah. I think overall libshared is going to be mostly a set of tools
that are flexible enough to be put together in different ways based on
platform needs. You can imagine an embedded peripheral platform that
doesn't even run bluetoothd but instead simply uses btmgmt and
bt_gatt_server. So, I like to think of the shared/ directory as a
flexible library collection of sorts, though it may be just me.


> Actually we should probably notify a error response otherwise it may
> call destroy which gives no hint what happened with the call itself,
> but now I figure you actually have a timeout callback which can be
> used but it is not per operation and I can only register one at time
> so I cannot register one in gatt-client.c otherwise it may get
> overwritten, perhaps we should add a timeout callback to bt_att_send
> or just remove since disconnect callback should be called anyway and
> that can be registered by gatt-client.c.
>

Yeah, actually in my first patches for bt_att, the
bt_att_response_func_t had a "bool timeout" parameter. I later added a
bt_att_register_timeout in the bt_att_register_disconnect fashion,
though Marcel suggested to have a single handler for now. I don't
really know what the right thing to do is here but since there isn't
really a good way to recover from a timeout other than a disconnect,
maybe just invoking disconnect_callback would be sufficient.


> Btw, I resend the patches skipping this one, please check if they are
> no breaking anything for you.
>

Just tried your patches and bt_gatt_client seems to work fine for me.

Cheers,
Arman