[PATCH BlueZ 0/5] src/gatt-dbus: use gatt-db

[PATCH BlueZ 0/5] src/gatt-dbus: use gatt-db

[Steven Walter]

This series of patches updates gatt-dbus.c to use the new gatt-db
implementation.  It also modifies btgatt-server to publish the
GattManager1 D-Bus service.

-- 
-Steven Walter <stevenrwalter@gmail.com>

[PATCH BlueZ 1/5] src/gatt-dbus.c: convert to shared/gatt-db implementation

[Steven Walter]

---
 src/gatt-dbus.c | 182 ++++++++++++++++++++++++++------------------------------
 src/gatt-dbus.h |   3 +-
 src/gatt.c      |   2 +-
 3 files changed, 86 insertions(+), 101 deletions(-)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index c22e8af..a04c38f 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -27,6 +27,7 @@
 
 #include <stdint.h>
 #include <errno.h>
+#include <assert.h>
 
 #include <glib.h>
 #include <dbus/dbus.h>
@@ -39,6 +40,9 @@
 #include "log.h"
 
 #include "error.h"
+#include "shared/queue.h"
+#include "shared/gatt-db.h"
+#include "shared/att-types.h"
 #include "attrib/gattrib.h"
 #include "attrib/att.h"
 #include "attrib/gatt.h"
@@ -56,22 +60,18 @@ struct external_service {
 	DBusMessage *reg;
 	GDBusClient *client;
 	GSList *proxies;
-	struct btd_attribute *service;
+	struct gatt_db_attribute *service;
 };
 
 struct proxy_write_data {
-	btd_attr_write_result_t result_cb;
-	void *user_data;
+	struct gatt_db_attribute *attrib;
+	unsigned int id;
 };
 
-/*
- * Attribute to Proxy hash table. Used to map incoming
- * ATT operations to its external characteristic proxy.
- */
-static GHashTable *proxy_hash;
-
 static GSList *external_services;
 
+static struct gatt_db *gatt_db;
+
 static int external_service_path_cmp(gconstpointer a, gconstpointer b)
 {
 	const struct external_service *esvc = a;
@@ -116,7 +116,7 @@ static void remove_service(DBusConnection *conn, void *user_data)
 	external_services = g_slist_remove(external_services, esvc);
 
 	if (esvc->service)
-		btd_gatt_remove_service(esvc->service);
+		gatt_db_remove_service(gatt_db, esvc->service);
 
 	/*
 	 * Do not run in the same loop, this may be a disconnect
@@ -141,19 +141,19 @@ static uint8_t flags_string2int(const char *proper)
 
 	/* Regular Properties: See core spec 4.1 page 2183 */
 	if (!strcmp("broadcast", proper))
-		value = GATT_CHR_PROP_BROADCAST;
+		value = BT_GATT_CHRC_PROP_BROADCAST;
 	else if (!strcmp("read", proper))
-		value = GATT_CHR_PROP_READ;
+		value = BT_GATT_CHRC_PROP_READ;
 	else if (!strcmp("write-without-response", proper))
-		value = GATT_CHR_PROP_WRITE_WITHOUT_RESP;
+		value = BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP;
 	else if (!strcmp("write", proper))
-		value = GATT_CHR_PROP_WRITE;
+		value = BT_GATT_CHRC_PROP_WRITE;
 	else if (!strcmp("notify", proper))
-		value = GATT_CHR_PROP_NOTIFY;
+		value = BT_GATT_CHRC_PROP_NOTIFY;
 	else if (!strcmp("indicate", proper))
-		value = GATT_CHR_PROP_INDICATE;
+		value = BT_GATT_CHRC_PROP_INDICATE;
 	else if (!strcmp("authenticated-signed-writes", proper))
-		value = GATT_CHR_PROP_AUTH;
+		value = BT_GATT_CHRC_PROP_AUTH;
 	else
 		value = 0;
 
@@ -233,11 +233,13 @@ static void proxy_removed(GDBusProxy *proxy, void *user_data)
 	esvc->proxies = g_slist_remove(esvc->proxies, proxy);
 }
 
-static void proxy_read_cb(struct btd_attribute *attr,
-				btd_attr_read_result_t result, void *user_data)
+static void proxy_read_cb(struct gatt_db_attribute *attr,
+			  unsigned int id, uint16_t offset,
+			  uint8_t opcode, bdaddr_t *bdaddr,
+			  void *user_data)
 {
 	DBusMessageIter iter, array;
-	GDBusProxy *proxy;
+	GDBusProxy *proxy = (GDBusProxy *)user_data;
 	uint8_t *value;
 	int len;
 
@@ -247,21 +249,15 @@ static void proxy_read_cb(struct btd_attribute *attr,
 	 * properties changes automatically, it is not necessary to
 	 * get the value directly from the GATT server.
 	 */
-	proxy = g_hash_table_lookup(proxy_hash, attr);
-	if (!proxy) {
-		result(-ENOENT, NULL, 0, user_data);
-		return;
-	}
-
 	if (!g_dbus_proxy_get_property(proxy, "Value", &iter)) {
 		/* Unusual situation, read property will checked earlier */
-		result(-EPERM, NULL, 0, user_data);
+		gatt_db_attribute_read_result(attr, id, -EPERM, NULL, 0);
 		return;
 	}
 
 	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY) {
-		DBG("External service inconsistent!");
-		result(-EPERM, NULL, 0, user_data);
+		//DBG("External service inconsistent!");
+		gatt_db_attribute_read_result(attr, id, -EPERM, NULL, 0);
 		return;
 	}
 
@@ -270,13 +266,13 @@ static void proxy_read_cb(struct btd_attribute *attr,
 
 	DBG("attribute: %p read %d bytes", attr, len);
 
-	result(0, value, len, user_data);
+	gatt_db_attribute_read_result(attr, id, 0, value, len);
 }
 
 static void proxy_write_reply(const DBusError *derr, void *user_data)
 {
 	struct proxy_write_data *wdata = user_data;
-	int err;
+	uint8_t ecode = 0;
 
 	/*
 	 * Security requirements shall be handled by the core. If external
@@ -285,39 +281,29 @@ static void proxy_write_reply(const DBusError *derr, void *user_data)
 	 */
 
 	if (!dbus_error_is_set(derr)) {
-		err = 0;
+		ecode = 0;
 		goto done;
 	}
 
 	DBG("Write reply: %s", derr->message);
 
-	if (dbus_error_has_name(derr, DBUS_ERROR_NO_REPLY))
-		err = -ETIMEDOUT;
-	else if (dbus_error_has_name(derr, ERROR_INTERFACE ".InvalidArguments"))
-		err = -EINVAL;
-	else
-		err = -EPROTO;
+	ecode = BT_ATT_ERROR_REQUEST_NOT_SUPPORTED;
 
 done:
-	if (wdata && wdata->result_cb)
-		wdata->result_cb(err, wdata->user_data);
+	if (wdata && wdata->attrib)
+		gatt_db_attribute_write_result(wdata->attrib, wdata->id, ecode);
 }
 
-static void proxy_write_cb(struct btd_attribute *attr,
-					const uint8_t *value, size_t len,
-					btd_attr_write_result_t result,
-					void *user_data)
+static void proxy_write_cb(struct gatt_db_attribute *attrib,
+			   unsigned int id, uint16_t offset,
+			   const uint8_t *value, size_t len,
+			   uint8_t opcode, bdaddr_t *bdaddr,
+			   void *user_data)
 {
-	GDBusProxy *proxy;
-
-	proxy = g_hash_table_lookup(proxy_hash, attr);
-	if (!proxy) {
-		result(-ENOENT, user_data);
-		return;
-	}
+	GDBusProxy *proxy = (GDBusProxy *)user_data;
 
 	/*
-	 * "result" callback defines if the core wants to receive the
+	 * opcode defines if the core wants to receive the
 	 * operation result, allowing to select ATT Write Request or Write
 	 * Command. Descriptors requires Write Request operation. For
 	 * Characteristics, the implementation will define which operations
@@ -325,19 +311,20 @@ static void proxy_write_cb(struct btd_attribute *attr,
 	 * TODO: Write Long Characteristics/Descriptors.
 	 */
 
-	if (result) {
+	if (opcode != BT_ATT_OP_WRITE_CMD) {
 		struct proxy_write_data *wdata;
 
 		wdata = g_new0(struct proxy_write_data, 1);
-		wdata->result_cb = result;
-		wdata->user_data = user_data;
+		wdata->attrib = attrib;
+		wdata->id = id;
 
 		if (!g_dbus_proxy_set_property_array(proxy, "Value",
 						DBUS_TYPE_BYTE, value, len,
 						proxy_write_reply,
 						wdata, g_free)) {
 			g_free(wdata);
-			result(-ENOENT, user_data);
+			gatt_db_attribute_write_result(attrib, id, BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND);
+
 		}
 	} else {
 		/*
@@ -356,7 +343,8 @@ static void proxy_write_cb(struct btd_attribute *attr,
 }
 
 static int register_external_service(struct external_service *esvc,
-							GDBusProxy *proxy)
+							GDBusProxy *proxy,
+							int num_handles)
 {
 	DBusMessageIter iter;
 	const char *str, *path, *iface;
@@ -379,20 +367,23 @@ static int register_external_service(struct external_service *esvc,
 	if (bt_string_to_uuid(&uuid, str) < 0)
 		return -EINVAL;
 
-	esvc->service = btd_gatt_add_service(&uuid);
+	esvc->service = gatt_db_add_service(gatt_db, &uuid, true, num_handles);
 	if (!esvc->service)
 		return -EINVAL;
 
 	return 0;
 }
 
-static int add_char(GDBusProxy *proxy, const bt_uuid_t *uuid)
+static int add_char(struct gatt_db_attribute *service,
+		    GDBusProxy *proxy, const bt_uuid_t *uuid)
 {
 	DBusMessageIter iter;
 	struct btd_attribute *attr;
-	btd_attr_write_t write_cb;
-	btd_attr_read_t read_cb;
+	gatt_db_write_t write_cb;
+	gatt_db_read_t read_cb;
 	uint8_t propmask = 0;
+	uint32_t permissions = 0;
+	struct gatt_db_attribute *res;
 
 	/*
 	 * Optional property. If is not informed, read and write
@@ -402,45 +393,46 @@ static int add_char(GDBusProxy *proxy, const bt_uuid_t *uuid)
 	if (g_dbus_proxy_get_property(proxy, "Flags", &iter))
 		propmask = flags_get_bitmask(&iter);
 	else
-		propmask = GATT_CHR_PROP_WRITE_WITHOUT_RESP
-						| GATT_CHR_PROP_WRITE
-						| GATT_CHR_PROP_READ;
+		propmask = BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP
+						| BT_GATT_CHRC_PROP_WRITE
+						| BT_GATT_CHRC_PROP_READ;
 	if (!propmask)
 		return -EINVAL;
 
-	if (propmask & GATT_CHR_PROP_READ)
+	if (propmask & BT_GATT_CHRC_PROP_READ) {
 		read_cb = proxy_read_cb;
-	else
+		permissions |= BT_ATT_PERM_READ;
+	} else {
 		read_cb = NULL;
+	}
 
-	if (propmask & (GATT_CHR_PROP_WRITE | GATT_CHR_PROP_WRITE_WITHOUT_RESP))
+	if (propmask & (BT_GATT_CHRC_PROP_WRITE | BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP)) {
 		write_cb = proxy_write_cb;
-	else
+		permissions |= BT_ATT_PERM_WRITE;
+	} else {
 		write_cb = NULL;
+	}
 
-	attr = btd_gatt_add_char(uuid, propmask, read_cb, write_cb);
-	if (!attr)
-		return -ENOMEM;
-
-	g_hash_table_insert(proxy_hash, attr, g_dbus_proxy_ref(proxy));
+	res = gatt_db_service_add_characteristic(service, uuid, permissions, propmask,
+					   read_cb, write_cb, proxy);
+	assert(res);
 
 	return 0;
 }
 
-static int add_char_desc(GDBusProxy *proxy, const bt_uuid_t *uuid)
+static int add_char_desc(struct gatt_db_attribute *service,
+			 GDBusProxy *proxy, const bt_uuid_t *uuid)
 {
-	struct btd_attribute *attr;
-
-	attr = btd_gatt_add_char_desc(uuid, proxy_read_cb, proxy_write_cb);
-	if (!attr)
-		return -ENOMEM;
-
-	g_hash_table_insert(proxy_hash, attr, g_dbus_proxy_ref(proxy));
+	struct gatt_db_attribute *attr;
 
+	attr = gatt_db_service_add_descriptor(service, uuid, BT_ATT_PERM_READ,
+				       proxy_read_cb, proxy_write_cb, proxy);
+	assert(attr);
 	return 0;
 }
 
-static int register_external_characteristics(GSList *proxies)
+static int register_external_characteristics(struct gatt_db_attribute *service,
+					     GSList *proxies)
 
 {
 	GSList *list;
@@ -468,9 +460,9 @@ static int register_external_characteristics(GSList *proxies)
 		path = g_dbus_proxy_get_path(proxy);
 
 		if (!strcmp(GATT_CHR_IFACE, iface))
-			ret = add_char(proxy, &uuid);
+			ret = add_char(service, proxy, &uuid);
 		else
-			ret = add_char_desc(proxy, &uuid);
+			ret = add_char_desc(service, proxy, &uuid);
 
 		if (ret < 0)
 			return ret;
@@ -492,13 +484,15 @@ static void client_ready(GDBusClient *client, void *user_data)
 		goto fail;
 
 	proxy = esvc->proxies->data;
-	if (register_external_service(esvc, proxy) < 0)
+	int num_handles = g_slist_length(esvc->proxies) + 1;
+	if (register_external_service(esvc, proxy, num_handles) < 0)
 		goto fail;
 
-	if (register_external_characteristics(g_slist_next(esvc->proxies)) < 0)
+	if (register_external_characteristics(esvc->service, g_slist_next(esvc->proxies)) < 0)
 		goto fail;
 
 	DBG("Added GATT service %s", esvc->path);
+	gatt_db_service_set_active(esvc->service, true);
 
 	reply = dbus_message_new_method_return(esvc->reg);
 	g_dbus_send_message(conn, reply);
@@ -627,32 +621,22 @@ static const GDBusMethodTable methods[] = {
 	{ }
 };
 
-gboolean gatt_dbus_manager_register(void)
+gboolean gatt_dbus_manager_register(struct gatt_db *db)
 {
 	if (!g_dbus_register_interface(btd_get_dbus_connection(),
 				"/org/bluez", GATT_MGR_IFACE,
 				methods, NULL, NULL, NULL, NULL))
 		return FALSE;
 
-	proxy_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal,
-				NULL, (GDestroyNotify) g_dbus_proxy_unref);
-
+	gatt_db = db;
 	return TRUE;
 }
 
 void gatt_dbus_manager_unregister(void)
 {
-	/* We might not have initialized if experimental features are
-	 * not enabled.
-	 */
-	if (!proxy_hash)
-		return;
-
-	g_hash_table_destroy(proxy_hash);
-	proxy_hash = NULL;
-
 	g_slist_free_full(external_services, external_service_free);
 
 	g_dbus_unregister_interface(btd_get_dbus_connection(), "/org/bluez",
 							GATT_MGR_IFACE);
+	gatt_db = NULL;
 }
diff --git a/src/gatt-dbus.h b/src/gatt-dbus.h
index 310cfa9..7716648 100644
--- a/src/gatt-dbus.h
+++ b/src/gatt-dbus.h
@@ -21,5 +21,6 @@
  *
  */
 
-gboolean gatt_dbus_manager_register(void);
+struct gatt_db;
+gboolean gatt_dbus_manager_register(struct gatt_db *db);
 void gatt_dbus_manager_unregister(void);
diff --git a/src/gatt.c b/src/gatt.c
index 3060462..4028919 100644
--- a/src/gatt.c
+++ b/src/gatt.c
@@ -309,7 +309,7 @@ void gatt_init(void)
 {
 	DBG("Starting GATT server");
 
-	gatt_dbus_manager_register();
+	gatt_dbus_manager_register(NULL);
 }
 
 void gatt_cleanup(void)
-- 
1.9.1

[PATCH BlueZ 2/5] gdbus/client.c: don't assume the root object path is valid

[Steven Walter]

We should look for objects at the path the client has registered, not
hard-coded at root.  In particular, python D-Bus objects will not
respond to requests directed at the root object path.
---
 gdbus/client.c  | 2 +-
 src/gatt-dbus.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbus/client.c b/gdbus/client.c
index eb68a0f..a270fc2 100644
--- a/gdbus/client.c
+++ b/gdbus/client.c
@@ -1115,7 +1115,7 @@ static void get_managed_objects(GDBusClient *client)
 	if (client->get_objects_call != NULL)
 		return;
 
-	msg = dbus_message_new_method_call(client->service_name, "/",
+	msg = dbus_message_new_method_call(client->service_name, client->base_path,
 					DBUS_INTERFACE_DBUS ".ObjectManager",
 							"GetManagedObjects");
 	if (msg == NULL)
diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index a04c38f..e61af15 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -524,7 +524,7 @@ static struct external_service *external_service_new(DBusConnection *conn,
 	GDBusClient *client;
 	const char *sender = dbus_message_get_sender(msg);
 
-	client = g_dbus_client_new(conn, sender, "/");
+	client = g_dbus_client_new(conn, sender, path);
 	if (!client)
 		return NULL;
 
-- 
1.9.1

[PATCH BlueZ 3/5] tools/btgatt-server.c: convert to glib mainloop

[Steven Walter]

Prep work for running gatt-dbus services
---
 Makefile.tools        |   4 +-
 tools/btgatt-server.c | 136 ++++++++++++++++++++++++++------------------------
 2 files changed, 72 insertions(+), 68 deletions(-)

diff --git a/Makefile.tools b/Makefile.tools
index ef4162b..7520ab5 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -277,8 +277,8 @@ tools_btgatt_client_LDADD = src/libshared-mainloop.la \
 						lib/libbluetooth-internal.la
 
 tools_btgatt_server_SOURCES = tools/btgatt-server.c src/uuid-helper.c
-tools_btgatt_server_LDADD = src/libshared-mainloop.la \
-						lib/libbluetooth-internal.la
+tools_btgatt_server_LDADD = lib/libbluetooth-internal.la src/libshared-glib.la \
+						@GLIB_LIBS@
 
 EXTRA_DIST += tools/bdaddr.1
 endif
diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
index c603b30..fa07cfb 100644
--- a/tools/btgatt-server.c
+++ b/tools/btgatt-server.c
@@ -20,14 +20,17 @@
 #include <config.h>
 #endif
 
+#include <assert.h>
 #include <stdio.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <signal.h>
 #include <time.h>
 #include <stdlib.h>
 #include <getopt.h>
 #include <unistd.h>
 #include <errno.h>
+#include <glib.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/hci.h>
@@ -35,7 +38,6 @@
 #include <bluetooth/l2cap.h>
 #include "lib/uuid.h"
 
-#include "monitor/mainloop.h"
 #include "src/shared/util.h"
 #include "src/shared/att.h"
 #include "src/shared/queue.h"
@@ -101,11 +103,13 @@ static void print_prompt(void)
 	fflush(stdout);
 }
 
+static GMainLoop *event_loop;
+
 static void att_disconnect_cb(void *user_data)
 {
 	printf("Device disconnected\n");
 
-	mainloop_quit();
+	g_main_loop_quit(event_loop);
 }
 
 static void att_debug_cb(const char *str, void *user_data)
@@ -536,18 +540,11 @@ static void populate_db(struct server *server)
 	populate_hr_service(server);
 }
 
-static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
+static struct server *server_create(struct server *server, int fd, uint16_t mtu, bool hr_visible)
 {
-	struct server *server;
 	struct bt_att *att;
 	size_t name_len = strlen(test_device_name);
 
-	server = new0(struct server, 1);
-	if (!server) {
-		fprintf(stderr, "Failed to allocate memory for server\n");
-		return NULL;
-	}
-
 	att = bt_att_new(fd);
 	if (!att) {
 		fprintf(stderr, "Failed to initialze ATT transport layer\n");
@@ -575,11 +572,6 @@ static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
 	server->device_name[name_len] = '\0';
 
 	server->fd = fd;
-	server->db = gatt_db_new();
-	if (!server->db) {
-		fprintf(stderr, "Failed to create GATT database\n");
-		goto fail;
-	}
 
 	server->gatt = bt_gatt_server_new(server->db, att, mtu);
 	if (!server->gatt) {
@@ -600,7 +592,6 @@ static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
 
 	/* bt_gatt_server already holds a reference */
 	bt_att_unref(att);
-	populate_db(server);
 
 	return server;
 
@@ -647,11 +638,9 @@ static struct option main_options[] = {
 
 static int l2cap_le_att_listen_and_accept(bdaddr_t *src, int sec)
 {
-	int sk, nsk;
-	struct sockaddr_l2 srcaddr, addr;
-	socklen_t optlen;
+	int sk;
+	struct sockaddr_l2 srcaddr;
 	struct bt_security btsec;
-	char ba[18];
 
 	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP);
 	if (sk < 0) {
@@ -687,19 +676,7 @@ static int l2cap_le_att_listen_and_accept(bdaddr_t *src, int sec)
 
 	printf("Started listening on ATT channel. Waiting for connections\n");
 
-	memset(&addr, 0, sizeof(addr));
-	optlen = sizeof(addr);
-	nsk = accept(sk, (struct sockaddr *) &addr, &optlen);
-	if (nsk < 0) {
-		perror("Accept failed");
-		goto fail;
-	}
-
-	ba2str(&addr.l2_bdaddr, ba);
-	printf("Connect from %s\n", ba);
-	close(sk);
-
-	return nsk;
+	return sk;
 
 fail:
 	close(sk);
@@ -907,8 +884,9 @@ static void cmd_help(struct server *server, char *cmd_str)
 		printf("\t%-15s\t%s\n", command[i].cmd, command[i].doc);
 }
 
-static void prompt_read_cb(int fd, uint32_t events, void *user_data)
+static gboolean prompt_read_cb(GIOChannel *channel, GIOCondition events, gpointer user_data)
 {
+	int fd = g_io_channel_unix_get_fd(channel);
 	ssize_t read;
 	size_t len = 0;
 	char *line = NULL;
@@ -916,19 +894,19 @@ static void prompt_read_cb(int fd, uint32_t events, void *user_data)
 	struct server *server = user_data;
 	int i;
 
-	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) {
-		mainloop_quit();
-		return;
+	if (events & (G_IO_HUP | G_IO_ERR)) {
+		g_main_loop_quit(event_loop);
+		return FALSE;
 	}
 
 	read = getline(&line, &len, stdin);
 	if (read < 0)
-		return;
+		return FALSE;
 
 	if (read <= 1) {
 		cmd_help(server, NULL);
 		print_prompt();
-		return;
+		return TRUE;
 	}
 
 	line[read-1] = '\0';
@@ -955,18 +933,46 @@ failed:
 	print_prompt();
 
 	free(line);
+	return TRUE;
+}
+
+static gboolean signal_cb(void *user_data)
+{
+	g_main_loop_quit(event_loop);
+	return FALSE;
 }
 
-static void signal_cb(int signum, void *user_data)
+static struct server *server;
+static uint16_t mtu = 0;
+static bool hr_visible = false;
+
+static gboolean new_connection_cb(GIOChannel *channel, GIOCondition events, gpointer user_data)
 {
-	switch (signum) {
-	case SIGINT:
-	case SIGTERM:
-		mainloop_quit();
-		break;
-	default:
-		break;
+	int sk = g_io_channel_unix_get_fd(channel);
+	int nsk;
+	socklen_t optlen;
+	struct sockaddr_l2 addr;
+	char ba[18];
+
+	memset(&addr, 0, sizeof(addr));
+	optlen = sizeof(addr);
+	nsk = accept(sk, (struct sockaddr *) &addr, &optlen);
+	if (nsk < 0) {
+		perror("Accept failed");
+		return TRUE;
+	}
+
+	ba2str(&addr.l2_bdaddr, ba);
+	printf("Connect from %s\n", ba);
+	close(sk);
+
+	server = server_create(server, nsk, mtu, hr_visible);
+	if (!server) {
+		close(nsk);
+		return EXIT_FAILURE;
 	}
+
+	return TRUE;
 }
 
 int main(int argc, char *argv[])
@@ -976,10 +982,6 @@ int main(int argc, char *argv[])
 	int dev_id = -1;
 	int fd;
 	int sec = BT_SECURITY_LOW;
-	uint16_t mtu = 0;
-	sigset_t mask;
-	bool hr_visible = false;
-	struct server *server;
 
 	while ((opt = getopt_long(argc, argv, "+hvrs:m:i:",
 						main_options, NULL)) != -1) {
@@ -1057,35 +1059,37 @@ int main(int argc, char *argv[])
 		fprintf(stderr, "Failed to accept L2CAP ATT connection\n");
 		return EXIT_FAILURE;
 	}
+	GIOChannel *channel = g_io_channel_unix_new(fd);
+	g_io_add_watch(channel, G_IO_IN, new_connection_cb, NULL);
 
-	mainloop_init();
+	event_loop = g_main_loop_new(NULL, FALSE);
 
-	server = server_create(fd, mtu, hr_visible);
+	server = new0(struct server, 1);
 	if (!server) {
-		close(fd);
+		fprintf(stderr, "Failed to allocate memory for server\n");
 		return EXIT_FAILURE;
 	}
+	server->db = gatt_db_new();
+	populate_db(server);
 
-	if (mainloop_add_fd(fileno(stdin),
-				EPOLLIN | EPOLLRDHUP | EPOLLHUP | EPOLLERR,
-				prompt_read_cb, server, NULL) < 0) {
-		fprintf(stderr, "Failed to initialize console\n");
-		server_destroy(server);
-
+	if (!server->db) {
+		fprintf(stderr, "Failed to create GATT database\n");
 		return EXIT_FAILURE;
 	}
 
-	printf("Running GATT server\n");
+	channel = g_io_channel_unix_new(fileno(stdin));
+	int source = g_io_add_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP,
+				prompt_read_cb, server);
+
 
-	sigemptyset(&mask);
-	sigaddset(&mask, SIGINT);
-	sigaddset(&mask, SIGTERM);
+	printf("Running GATT server\n");
 
-	mainloop_set_signal(&mask, signal_cb, NULL, NULL);
+	g_unix_signal_add_full(G_PRIORITY_DEFAULT, SIGINT, signal_cb, NULL, NULL);
+	g_unix_signal_add_full(G_PRIORITY_DEFAULT, SIGTERM, signal_cb, NULL, NULL);
 
 	print_prompt();
 
-	mainloop_run();
+	g_main_loop_run(event_loop);
 
 	printf("\n\nShutting down...\n");
 
-- 
1.9.1

[PATCH BlueZ 4/5] tools/btgatt-server.c: publish the GattManager D-Bus services

[Steven Walter]

---
 Makefile.tools        |  5 +++--
 tools/btgatt-server.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/Makefile.tools b/Makefile.tools
index 7520ab5..f7335df 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -276,9 +276,10 @@ tools_btgatt_client_SOURCES = tools/btgatt-client.c src/uuid-helper.c
 tools_btgatt_client_LDADD = src/libshared-mainloop.la \
 						lib/libbluetooth-internal.la
 
-tools_btgatt_server_SOURCES = tools/btgatt-server.c src/uuid-helper.c
+tools_btgatt_server_SOURCES = tools/btgatt-server.c src/uuid-helper.c src/gatt-dbus.c src/gatt-dbus.h \
+			      src/error.c src/dbus-common.c src/log.c src/log.h
 tools_btgatt_server_LDADD = lib/libbluetooth-internal.la src/libshared-glib.la \
-						@GLIB_LIBS@
+						gdbus/libgdbus-internal.la @GLIB_LIBS@ @DBUS_LIBS@
 
 EXTRA_DIST += tools/bdaddr.1
 endif
diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
index fa07cfb..a0c87cf 100644
--- a/tools/btgatt-server.c
+++ b/tools/btgatt-server.c
@@ -31,6 +31,8 @@
 #include <unistd.h>
 #include <errno.h>
 #include <glib.h>
+#include <dbus/dbus.h>
+#include <dbus/dbus-glib.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/hci.h>
@@ -38,6 +40,9 @@
 #include <bluetooth/l2cap.h>
 #include "lib/uuid.h"
 
+#include "gdbus/gdbus.h"
+#include "src/log.h"
+#include "src/dbus-common.h"
 #include "src/shared/util.h"
 #include "src/shared/att.h"
 #include "src/shared/queue.h"
@@ -45,6 +50,8 @@
 #include "src/shared/gatt-db.h"
 #include "src/shared/gatt-server.h"
 
+#define BLUEZ_NAME "org.bluez"
+
 #define UUID_GAP			0x1800
 #define UUID_GATT			0x1801
 #define UUID_HEART_RATE			0x180d
@@ -942,6 +949,37 @@ static gboolean signal_cb(void *user_data)
 	return FALSE;
 }
 
+static void disconnected_dbus(DBusConnection *conn, void *data)
+{
+	info("Disconnected from D-Bus. Exiting.");
+	g_main_loop_quit(event_loop);
+}
+
+static int connect_dbus(void)
+{
+	DBusConnection *conn;
+	DBusError err;
+
+	dbus_error_init(&err);
+
+	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, BLUEZ_NAME, &err);
+	if (!conn) {
+		if (dbus_error_is_set(&err)) {
+			g_printerr("D-Bus setup failed: %s\n", err.message);
+			dbus_error_free(&err);
+			return -EIO;
+		}
+		return -EALREADY;
+	}
+
+	set_dbus_connection(conn);
+
+	g_dbus_set_disconnect_function(conn, disconnected_dbus, NULL, NULL);
+	g_dbus_attach_object_manager(conn);
+
+	return 0;
+}
+
 static struct server *server;
 static uint16_t mtu = 0;
 static bool hr_visible = false;
@@ -1062,6 +1100,7 @@ int main(int argc, char *argv[])
 	GIOChannel *channel = g_io_channel_unix_new(fd);
 	g_io_add_watch(channel, G_IO_IN, new_connection_cb, NULL);
 
+	g_dbus_set_flags(G_DBUS_FLAG_ENABLE_EXPERIMENTAL);
 	event_loop = g_main_loop_new(NULL, FALSE);
 
 	server = new0(struct server, 1);
@@ -1077,6 +1116,10 @@ int main(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
+	connect_dbus();
+	gboolean result = gatt_dbus_manager_register(server->db);
+	assert(result);
+
 	channel = g_io_channel_unix_new(fileno(stdin));
 	int source = g_io_add_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP,
 				prompt_read_cb, server);
-- 
1.9.1

[PATCH BlueZ 5/5] src/gatt-dbus: register desciptors after the characteristic they apply to

[Steven Walter]

Ordering of these entries in the attribute database matters
---
 src/gatt-dbus.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index e61af15..39cc38a 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -374,8 +374,57 @@ static int register_external_service(struct external_service *esvc,
 	return 0;
 }
 
+static int add_char_desc(struct gatt_db_attribute *service,
+			 GDBusProxy *proxy, const bt_uuid_t *uuid);
+
+static int add_desc_for_char (struct gatt_db_attribute *service,
+			     DBusMessageIter *iter, GSList *proxies)
+{
+	DBusMessageIter desc_iter;
+	GDBusProxy *proxy = NULL;
+	GSList *list = NULL;
+	const char *path, *str;
+	bt_uuid_t uuid;
+
+	dbus_message_iter_recurse(iter, &desc_iter);
+
+	do {
+		DBusMessageIter iter2;
+
+		if (dbus_message_iter_get_arg_type(&desc_iter) != DBUS_TYPE_OBJECT_PATH)
+			return -EINVAL;
+
+		dbus_message_iter_get_basic(&desc_iter, &path);
+		/* Find the proxy matching path */
+		for (list = proxies; list; list = g_slist_next(list)) {
+			const char *iface;
+			proxy = list->data;
+			iface = g_dbus_proxy_get_interface(proxy);
+			if (strcmp(iface, GATT_DESCRIPTOR_IFACE))
+				continue;
+			if (!strcmp(g_dbus_proxy_get_path(proxy), path))
+				break;
+		}
+		if (!list) 
+			return -EINVAL;
+
+		if (!g_dbus_proxy_get_property(proxy, "UUID", &iter2))
+			return -EINVAL;
+
+		if (dbus_message_iter_get_arg_type(&iter2) != DBUS_TYPE_STRING)
+			return -EINVAL;
+
+		dbus_message_iter_get_basic(&iter2, &str);
+
+		if (bt_string_to_uuid(&uuid, str) < 0)
+			return -EINVAL;
+
+		add_char_desc(service, proxy, &uuid);
+	} while (dbus_message_iter_next(&desc_iter));
+}
+
 static int add_char(struct gatt_db_attribute *service,
-		    GDBusProxy *proxy, const bt_uuid_t *uuid)
+		    GDBusProxy *proxy, const bt_uuid_t *uuid, GSList *proxies)
 {
 	DBusMessageIter iter;
 	struct btd_attribute *attr;
@@ -417,6 +466,12 @@ static int add_char(struct gatt_db_attribute *service,
 					   read_cb, write_cb, proxy);
 	assert(res);
 
+	if (g_dbus_proxy_get_property(proxy, "Descriptors", &iter)) {
+		int ret = add_desc_for_char(service, &iter, proxies);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -442,7 +497,7 @@ static int register_external_characteristics(struct gatt_db_attribute *service,
 		DBusMessageIter iter;
 		bt_uuid_t uuid;
 		const char *path, *iface, *str;
-		int ret;
+		int ret = 0;
 
 		/* Mandatory property */
 		if (!g_dbus_proxy_get_property(proxy, "UUID", &iter))
@@ -460,9 +515,8 @@ static int register_external_characteristics(struct gatt_db_attribute *service,
 		path = g_dbus_proxy_get_path(proxy);
 
 		if (!strcmp(GATT_CHR_IFACE, iface))
-			ret = add_char(service, proxy, &uuid);
-		else
-			ret = add_char_desc(service, proxy, &uuid);
+			ret = add_char(service, proxy, &uuid, proxies);
+		/* skip descriptors, they're added by add_char */
 
 		if (ret < 0)
 			return ret;
-- 
1.9.1

Re: [PATCH BlueZ 2/5] gdbus/client.c: don't assume the root object path is valid

[Luiz Augusto von Dentz]

Hi Steven,

On Wed, Dec 3, 2014 at 5:54 PM, Steven Walter <stevenrwalter@gmail.com> wrote:
> We should look for objects at the path the client has registered, not
> hard-coded at root.  In particular, python D-Bus objects will not
> respond to requests directed at the root object path.
> ---
>  gdbus/client.c  | 2 +-
>  src/gatt-dbus.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdbus/client.c b/gdbus/client.c
> index eb68a0f..a270fc2 100644
> --- a/gdbus/client.c
> +++ b/gdbus/client.c
> @@ -1115,7 +1115,7 @@ static void get_managed_objects(GDBusClient *client)
>         if (client->get_objects_call != NULL)
>                 return;
>
> -       msg = dbus_message_new_method_call(client->service_name, "/",
> +       msg = dbus_message_new_method_call(client->service_name, client->base_path,
>                                         DBUS_INTERFACE_DBUS ".ObjectManager",
>                                                         "GetManagedObjects");
>         if (msg == NULL)
> diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
> index a04c38f..e61af15 100644
> --- a/src/gatt-dbus.c
> +++ b/src/gatt-dbus.c
> @@ -524,7 +524,7 @@ static struct external_service *external_service_new(DBusConnection *conn,
>         GDBusClient *client;
>         const char *sender = dbus_message_get_sender(msg);
>
> -       client = g_dbus_client_new(conn, sender, "/");
> +       client = g_dbus_client_new(conn, sender, path);
>         if (!client)
>                 return NULL;
>
> --
> 1.9.1

I remember the spec had something regarding '/' must be present and
the ObjectManager was only allowed on that because of that otherwise
the clients have to know before hand what path are available which is
the whole point of having ObjectManager to discover that.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 3/5] tools/btgatt-server.c: convert to glib mainloop

[Arman Uguray]

Hi Steven,

> On Wed, Dec 3, 2014 at 7:54 AM, Steven Walter <stevenrwalter@gmail.com> wrote:
> Prep work for running gatt-dbus services
> ---
>  Makefile.tools        |   4 +-
>  tools/btgatt-server.c | 136 ++++++++++++++++++++++++++------------------------
>  2 files changed, 72 insertions(+), 68 deletions(-)
>
> diff --git a/Makefile.tools b/Makefile.tools
> index ef4162b..7520ab5 100644
> --- a/Makefile.tools
> +++ b/Makefile.tools
> @@ -277,8 +277,8 @@ tools_btgatt_client_LDADD = src/libshared-mainloop.la \
>                                                 lib/libbluetooth-internal.la
>
>  tools_btgatt_server_SOURCES = tools/btgatt-server.c src/uuid-helper.c
> -tools_btgatt_server_LDADD = src/libshared-mainloop.la \
> -                                               lib/libbluetooth-internal.la
> +tools_btgatt_server_LDADD = lib/libbluetooth-internal.la src/libshared-glib.la \
> +                                               @GLIB_LIBS@
>

I'm not sure if this is something that we want. We've been using
monitor/mainloop in the newer tools as its more lightweight than glib.
I also don't see how gatt-dbus really fits into the scope of this
tool.

>  EXTRA_DIST += tools/bdaddr.1
>  endif
> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
> index c603b30..fa07cfb 100644
> --- a/tools/btgatt-server.c
> +++ b/tools/btgatt-server.c
> @@ -20,14 +20,17 @@
>  #include <config.h>
>  #endif
>
> +#include <assert.h>
>  #include <stdio.h>
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <signal.h>
>  #include <time.h>
>  #include <stdlib.h>
>  #include <getopt.h>
>  #include <unistd.h>
>  #include <errno.h>
> +#include <glib.h>
>
>  #include <bluetooth/bluetooth.h>
>  #include <bluetooth/hci.h>
> @@ -35,7 +38,6 @@
>  #include <bluetooth/l2cap.h>
>  #include "lib/uuid.h"
>
> -#include "monitor/mainloop.h"
>  #include "src/shared/util.h"
>  #include "src/shared/att.h"
>  #include "src/shared/queue.h"
> @@ -101,11 +103,13 @@ static void print_prompt(void)
>         fflush(stdout);
>  }
>
> +static GMainLoop *event_loop;
> +
>  static void att_disconnect_cb(void *user_data)
>  {
>         printf("Device disconnected\n");
>
> -       mainloop_quit();
> +       g_main_loop_quit(event_loop);
>  }
>
>  static void att_debug_cb(const char *str, void *user_data)
> @@ -536,18 +540,11 @@ static void populate_db(struct server *server)
>         populate_hr_service(server);
>  }
>
> -static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
> +static struct server *server_create(struct server *server, int fd, uint16_t mtu, bool hr_visible)
>  {
> -       struct server *server;
>         struct bt_att *att;
>         size_t name_len = strlen(test_device_name);
>
> -       server = new0(struct server, 1);
> -       if (!server) {
> -               fprintf(stderr, "Failed to allocate memory for server\n");
> -               return NULL;
> -       }
> -
>         att = bt_att_new(fd);
>         if (!att) {
>                 fprintf(stderr, "Failed to initialze ATT transport layer\n");
> @@ -575,11 +572,6 @@ static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
>         server->device_name[name_len] = '\0';
>
>         server->fd = fd;
> -       server->db = gatt_db_new();
> -       if (!server->db) {
> -               fprintf(stderr, "Failed to create GATT database\n");
> -               goto fail;
> -       }
>
>         server->gatt = bt_gatt_server_new(server->db, att, mtu);
>         if (!server->gatt) {
> @@ -600,7 +592,6 @@ static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
>
>         /* bt_gatt_server already holds a reference */
>         bt_att_unref(att);
> -       populate_db(server);
>
>         return server;
>
> @@ -647,11 +638,9 @@ static struct option main_options[] = {
>
>  static int l2cap_le_att_listen_and_accept(bdaddr_t *src, int sec)
>  {
> -       int sk, nsk;
> -       struct sockaddr_l2 srcaddr, addr;
> -       socklen_t optlen;
> +       int sk;
> +       struct sockaddr_l2 srcaddr;
>         struct bt_security btsec;
> -       char ba[18];
>
>         sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP);
>         if (sk < 0) {
> @@ -687,19 +676,7 @@ static int l2cap_le_att_listen_and_accept(bdaddr_t *src, int sec)
>
>         printf("Started listening on ATT channel. Waiting for connections\n");
>
> -       memset(&addr, 0, sizeof(addr));
> -       optlen = sizeof(addr);
> -       nsk = accept(sk, (struct sockaddr *) &addr, &optlen);
> -       if (nsk < 0) {
> -               perror("Accept failed");
> -               goto fail;
> -       }
> -
> -       ba2str(&addr.l2_bdaddr, ba);
> -       printf("Connect from %s\n", ba);
> -       close(sk);
> -
> -       return nsk;
> +       return sk;
>
>  fail:
>         close(sk);
> @@ -907,8 +884,9 @@ static void cmd_help(struct server *server, char *cmd_str)
>                 printf("\t%-15s\t%s\n", command[i].cmd, command[i].doc);
>  }
>
> -static void prompt_read_cb(int fd, uint32_t events, void *user_data)
> +static gboolean prompt_read_cb(GIOChannel *channel, GIOCondition events, gpointer user_data)
>  {
> +       int fd = g_io_channel_unix_get_fd(channel);
>         ssize_t read;
>         size_t len = 0;
>         char *line = NULL;
> @@ -916,19 +894,19 @@ static void prompt_read_cb(int fd, uint32_t events, void *user_data)
>         struct server *server = user_data;
>         int i;
>
> -       if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) {
> -               mainloop_quit();
> -               return;
> +       if (events & (G_IO_HUP | G_IO_ERR)) {
> +               g_main_loop_quit(event_loop);
> +               return FALSE;
>         }
>
>         read = getline(&line, &len, stdin);
>         if (read < 0)
> -               return;
> +               return FALSE;
>
>         if (read <= 1) {
>                 cmd_help(server, NULL);
>                 print_prompt();
> -               return;
> +               return TRUE;
>         }
>
>         line[read-1] = '\0';
> @@ -955,18 +933,46 @@ failed:
>         print_prompt();
>
>         free(line);
> +       return TRUE;
> +}
> +
> +static gboolean signal_cb(void *user_data)
> +{
> +       g_main_loop_quit(event_loop);
> +       return FALSE;
>  }
>
> -static void signal_cb(int signum, void *user_data)
> +static struct server *server;
> +static uint16_t mtu = 0;
> +static bool hr_visible = false;
> +
> +static gboolean new_connection_cb(GIOChannel *channel, GIOCondition events, gpointer user_data)
>  {
> -       switch (signum) {
> -       case SIGINT:
> -       case SIGTERM:
> -               mainloop_quit();
> -               break;
> -       default:
> -               break;
> +       int sk = g_io_channel_unix_get_fd(channel);
> +       int nsk;
> +       socklen_t optlen;
> +       struct sockaddr_l2 addr;
> +       char ba[18];
> +
> +       memset(&addr, 0, sizeof(addr));
> +       optlen = sizeof(addr);
> +       nsk = accept(sk, (struct sockaddr *) &addr, &optlen);
> +       if (nsk < 0) {
> +               perror("Accept failed");
> +               return TRUE;
> +       }
> +
> +       ba2str(&addr.l2_bdaddr, ba);
> +       printf("Connect from %s\n", ba);
> +       close(sk);
> +
> +       server = server_create(server, nsk, mtu, hr_visible);
> +       if (!server) {
> +               close(nsk);
> +               return EXIT_FAILURE;
>         }
> +
> +       return TRUE;
>  }
>
>  int main(int argc, char *argv[])
> @@ -976,10 +982,6 @@ int main(int argc, char *argv[])
>         int dev_id = -1;
>         int fd;
>         int sec = BT_SECURITY_LOW;
> -       uint16_t mtu = 0;
> -       sigset_t mask;
> -       bool hr_visible = false;
> -       struct server *server;
>
>         while ((opt = getopt_long(argc, argv, "+hvrs:m:i:",
>                                                 main_options, NULL)) != -1) {
> @@ -1057,35 +1059,37 @@ int main(int argc, char *argv[])
>                 fprintf(stderr, "Failed to accept L2CAP ATT connection\n");
>                 return EXIT_FAILURE;
>         }
> +       GIOChannel *channel = g_io_channel_unix_new(fd);
> +       g_io_add_watch(channel, G_IO_IN, new_connection_cb, NULL);
>
> -       mainloop_init();
> +       event_loop = g_main_loop_new(NULL, FALSE);
>
> -       server = server_create(fd, mtu, hr_visible);
> +       server = new0(struct server, 1);
>         if (!server) {
> -               close(fd);
> +               fprintf(stderr, "Failed to allocate memory for server\n");
>                 return EXIT_FAILURE;
>         }
> +       server->db = gatt_db_new();
> +       populate_db(server);
>
> -       if (mainloop_add_fd(fileno(stdin),
> -                               EPOLLIN | EPOLLRDHUP | EPOLLHUP | EPOLLERR,
> -                               prompt_read_cb, server, NULL) < 0) {
> -               fprintf(stderr, "Failed to initialize console\n");
> -               server_destroy(server);
> -
> +       if (!server->db) {
> +               fprintf(stderr, "Failed to create GATT database\n");
>                 return EXIT_FAILURE;
>         }
>
> -       printf("Running GATT server\n");
> +       channel = g_io_channel_unix_new(fileno(stdin));
> +       int source = g_io_add_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP,
> +                               prompt_read_cb, server);
> +
>
> -       sigemptyset(&mask);
> -       sigaddset(&mask, SIGINT);
> -       sigaddset(&mask, SIGTERM);
> +       printf("Running GATT server\n");
>
> -       mainloop_set_signal(&mask, signal_cb, NULL, NULL);
> +       g_unix_signal_add_full(G_PRIORITY_DEFAULT, SIGINT, signal_cb, NULL, NULL);
> +       g_unix_signal_add_full(G_PRIORITY_DEFAULT, SIGTERM, signal_cb, NULL, NULL);
>
>         print_prompt();
>
> -       mainloop_run();
> +       g_main_loop_run(event_loop);
>
>         printf("\n\nShutting down...\n");
>
> --
> 1.9.1
>

Arman

Re: [PATCH BlueZ 3/5] tools/btgatt-server.c: convert to glib mainloop

[Steven Walter]

On Wed, Dec 3, 2014 at 11:17 AM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Steven,
>
>> On Wed, Dec 3, 2014 at 7:54 AM, Steven Walter <stevenrwalter@gmail.com> wrote:
>> Prep work for running gatt-dbus services
>> ---
>>  Makefile.tools        |   4 +-
>>  tools/btgatt-server.c | 136 ++++++++++++++++++++++++++------------------------
>>  2 files changed, 72 insertions(+), 68 deletions(-)
>>
>> diff --git a/Makefile.tools b/Makefile.tools
>> index ef4162b..7520ab5 100644
>> --- a/Makefile.tools
>> +++ b/Makefile.tools
>> @@ -277,8 +277,8 @@ tools_btgatt_client_LDADD = src/libshared-mainloop.la \
>>                                                 lib/libbluetooth-internal.la
>>
>>  tools_btgatt_server_SOURCES = tools/btgatt-server.c src/uuid-helper.c
>> -tools_btgatt_server_LDADD = src/libshared-mainloop.la \
>> -                                               lib/libbluetooth-internal.la
>> +tools_btgatt_server_LDADD = lib/libbluetooth-internal.la src/libshared-glib.la \
>> +                                               @GLIB_LIBS@
>>
>
> I'm not sure if this is something that we want. We've been using
> monitor/mainloop in the newer tools as its more lightweight than glib.
> I also don't see how gatt-dbus really fits into the scope of this
> tool.

The problem is probably that I don't understand the scope of the tool.
Given its generic name, I was thinking that one would use
btgatt-server to expose any kind of GATT attribute, in-process or not.
Would it make more sense for there to be a stand-alone tool for
publishing the GattManager1 D-Bus service?
-- 
-Steven Walter <stevenrwalter@gmail.com>

Re: [PATCH BlueZ 2/5] gdbus/client.c: don't assume the root object path is valid

[Steven Walter]

Hi Luiz

On Wed, Dec 3, 2014 at 11:08 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Steven,
>
> On Wed, Dec 3, 2014 at 5:54 PM, Steven Walter <stevenrwalter@gmail.com> wrote:
>> We should look for objects at the path the client has registered, not
>> hard-coded at root.  In particular, python D-Bus objects will not
>> respond to requests directed at the root object path.
>> ---
>>  gdbus/client.c  | 2 +-
>>  src/gatt-dbus.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbus/client.c b/gdbus/client.c
>> index eb68a0f..a270fc2 100644
>> --- a/gdbus/client.c
>> +++ b/gdbus/client.c
>> @@ -1115,7 +1115,7 @@ static void get_managed_objects(GDBusClient *client)
>>         if (client->get_objects_call != NULL)
>>                 return;
>>
>> -       msg = dbus_message_new_method_call(client->service_name, "/",
>> +       msg = dbus_message_new_method_call(client->service_name, client->base_path,
>>                                         DBUS_INTERFACE_DBUS ".ObjectManager",
>>                                                         "GetManagedObjects");
>>         if (msg == NULL)
>> diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
>> index a04c38f..e61af15 100644
>> --- a/src/gatt-dbus.c
>> +++ b/src/gatt-dbus.c
>> @@ -524,7 +524,7 @@ static struct external_service *external_service_new(DBusConnection *conn,
>>         GDBusClient *client;
>>         const char *sender = dbus_message_get_sender(msg);
>>
>> -       client = g_dbus_client_new(conn, sender, "/");
>> +       client = g_dbus_client_new(conn, sender, path);
>>         if (!client)
>>                 return NULL;
>>
>> --
>> 1.9.1
>
> I remember the spec had something regarding '/' must be present and
> the ObjectManager was only allowed on that because of that otherwise
> the clients have to know before hand what path are available which is
> the whole point of having ObjectManager to discover that.

I don't read that in the spec.  The specs says:

"The root of each sub-tree implements this interface so other
applications can get all objects, interfaces and properties in a
single method call."

Note "sub-tree", not "the root tree."  The spec is also careful to
talk about the ObjectManager's object path, not assuming that the path
will be '/'.

Regardless, we can always be less strict than the spec.  Since we have
an object path, why wouldn't we use it?
-- 
-Steven Walter <stevenrwalter@gmail.com>

Re: [PATCH BlueZ 3/5] tools/btgatt-server.c: convert to glib mainloop

[Arman Uguray]

Hi Steven,

> On Wed, Dec 3, 2014 at 9:24 AM, Steven Walter <stevenrwalter@gmail.com> wrote:
> On Wed, Dec 3, 2014 at 11:17 AM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Steven,
>>
>>> On Wed, Dec 3, 2014 at 7:54 AM, Steven Walter <stevenrwalter@gmail.com> wrote:
>>> Prep work for running gatt-dbus services
>>> ---
>>>  Makefile.tools        |   4 +-
>>>  tools/btgatt-server.c | 136 ++++++++++++++++++++++++++------------------------
>>>  2 files changed, 72 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/Makefile.tools b/Makefile.tools
>>> index ef4162b..7520ab5 100644
>>> --- a/Makefile.tools
>>> +++ b/Makefile.tools
>>> @@ -277,8 +277,8 @@ tools_btgatt_client_LDADD = src/libshared-mainloop.la \
>>>                                                 lib/libbluetooth-internal.la
>>>
>>>  tools_btgatt_server_SOURCES = tools/btgatt-server.c src/uuid-helper.c
>>> -tools_btgatt_server_LDADD = src/libshared-mainloop.la \
>>> -                                               lib/libbluetooth-internal.la
>>> +tools_btgatt_server_LDADD = lib/libbluetooth-internal.la src/libshared-glib.la \
>>> +                                               @GLIB_LIBS@
>>>
>>
>> I'm not sure if this is something that we want. We've been using
>> monitor/mainloop in the newer tools as its more lightweight than glib.
>> I also don't see how gatt-dbus really fits into the scope of this
>> tool.
>
> The problem is probably that I don't understand the scope of the tool.
> Given its generic name, I was thinking that one would use
> btgatt-server to expose any kind of GATT attribute, in-process or not.
> Would it make more sense for there to be a stand-alone tool for
> publishing the GattManager1 D-Bus service?

At the moment, the purpose of the tool is to basically demonstrate how
shared/gatt-db and shared/gatt-server can be used in a standalone
application. The GattManager1 D-Bus service is really bluetoothd core
concept and that's why I don't think it belongs in that tool.

The D-Bus API would really only apply to the daemon, which still needs
to implement a cleaner version of src/attrib-server and properly
implement the GATT server role (GAP and GATT services) at least for
the plugins before there can be a D-Bus API anyway. We still need a
framework built into the daemon that will properly send Service
Changed indications to all devices during a connection and to bonded
devices at connection establishment when there are changes to services
published via plugins and the D-Bus API.

So to answer this question: the tool probably never needs to export a
D-Bus service.

To address the overall patch set: it's definitely good to start
converting the src/gatt-dbus code to use gatt-db - which is the
ultimate goal anyway - but we still need to bake the new shared/gatt
code into the daemon core before we can really do anything with D-Bus
(in fact, the files src/gatt, src/gatt-dbus themselves will likely get
renamed or moved around). I guess my overall understanding is that
those files aren't really meant to be general purpose GATT D-Bus
functions but rather belong to the bluetooth daemon's core
functionality. Though others can comment more on this.

Thanks,
Arman

Re: [PATCH BlueZ 2/5] gdbus/client.c: don't assume the root object path is valid

[Luiz Augusto von Dentz]

Hi Steven,

On Wed, Dec 3, 2014 at 7:28 PM, Steven Walter <stevenrwalter@gmail.com> wrote:
> Hi Luiz
>
> On Wed, Dec 3, 2014 at 11:08 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Steven,
>>
>> On Wed, Dec 3, 2014 at 5:54 PM, Steven Walter <stevenrwalter@gmail.com> wrote:
>>> We should look for objects at the path the client has registered, not
>>> hard-coded at root.  In particular, python D-Bus objects will not
>>> respond to requests directed at the root object path.
>>> ---
>>>  gdbus/client.c  | 2 +-
>>>  src/gatt-dbus.c | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdbus/client.c b/gdbus/client.c
>>> index eb68a0f..a270fc2 100644
>>> --- a/gdbus/client.c
>>> +++ b/gdbus/client.c
>>> @@ -1115,7 +1115,7 @@ static void get_managed_objects(GDBusClient *client)
>>>         if (client->get_objects_call != NULL)
>>>                 return;
>>>
>>> -       msg = dbus_message_new_method_call(client->service_name, "/",
>>> +       msg = dbus_message_new_method_call(client->service_name, client->base_path,
>>>                                         DBUS_INTERFACE_DBUS ".ObjectManager",
>>>                                                         "GetManagedObjects");
>>>         if (msg == NULL)
>>> diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
>>> index a04c38f..e61af15 100644
>>> --- a/src/gatt-dbus.c
>>> +++ b/src/gatt-dbus.c
>>> @@ -524,7 +524,7 @@ static struct external_service *external_service_new(DBusConnection *conn,
>>>         GDBusClient *client;
>>>         const char *sender = dbus_message_get_sender(msg);
>>>
>>> -       client = g_dbus_client_new(conn, sender, "/");
>>> +       client = g_dbus_client_new(conn, sender, path);
>>>         if (!client)
>>>                 return NULL;
>>>
>>> --
>>> 1.9.1
>>
>> I remember the spec had something regarding '/' must be present and
>> the ObjectManager was only allowed on that because of that otherwise
>> the clients have to know before hand what path are available which is
>> the whole point of having ObjectManager to discover that.
>
> I don't read that in the spec.  The specs says:
>
> "The root of each sub-tree implements this interface so other
> applications can get all objects, interfaces and properties in a
> single method call."
>
> Note "sub-tree", not "the root tree."  The spec is also careful to
> talk about the ObjectManager's object path, not assuming that the path
> will be '/'.

Well that means by having '/' you can have only one ObjectManager
which simplify the implementation quite a bit, but I figure the real
problem with not having '/' is that bindings that can register objects
dynamically, such as in python I suppose, one could register a parent
path to a sub-tree leaving a child path with ObjectManager that would
duplicate signals related to ObjectManager which I believe is quite a
major fault if the binding if it is allowing that to happen. Now that
perhaps does not invalidate the patch if someone really know what they
are doing, but I would first check if python is really doing the right
thing here. Depending on the response we may even have to give some
feedback to D-Bus folks regarding the use of sub-trees.

> Regardless, we can always be less strict than the spec.  Since we have
> an object path, why wouldn't we use it?

Sure but then we need to change quite a few other places that are
still hardcoding '/' in gdbus code, also make sure gdbus changes are
in a separate patch and does not break anything.

-- 
Luiz Augusto von Dentz