[PATCH BlueZ 0/5] core: gatt: Support enabling notifications

[PATCH BlueZ 0/5] core: gatt: Support enabling notifications

[Arman Uguray]

This patch sets brings support for enabling notifications while in GATT
client-role. The changes that are introduced are:

  1. Implemented the StartNotify and StopNotify methods of the
     GattCharacteristic1 interface. This are internally tied to
     bt_gatt_client_register_notify and bt_gatt_client_unregister_notify.
     These also manage notification sessions on a per dbus sender basis.

  2. The exported GATT API objects are not removed in the event of a disconnect,
     if the devices are bonded. All notification sessions are also persisted and
     automatically re-enabled on reconnection. Also, adding new notification
     sessions via StartNotify is allowed during disconnects.

Arman Uguray (5):
  core: gatt: Implement GattCharacteristic1.StartNotify
  core: gatt: Implement GattCharacteristic1.StopNotify
  core: device: Don't check ready in service_removed
  core: device: Add device_is_bonded_for_gatt
  core: gatt: Keep objects over disconnects

 src/device.c      |  31 +++-
 src/device.h      |   1 +
 src/gatt-client.c | 453 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 466 insertions(+), 19 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 1/5] core: gatt: Implement GattCharacteristic1.StartNotify

[Arman Uguray]

This patch implements the StartNotify method of
org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
session to the D-Bus sender which internally registers a unique
notify handler with the bt_gatt_client. Each received notification
or indication causes a PropertiesChanged signal to be emitted for the
"Value" property.

The notify handler gets automatically unregistered when sender
disconnects from D-Bus.

Change-Id: Ia805127613ee538eced4591ba0229789dc54fab8
---
 src/gatt-client.c | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 247 insertions(+), 9 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index cb8ddf6..2f187ad 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -88,6 +88,9 @@ struct characteristic {
 	unsigned int write_id;
 
 	struct queue *descs;
+
+	bool notifying;
+	struct queue *notify_clients;
 };
 
 struct descriptor {
@@ -231,7 +234,9 @@ static void async_dbus_op_free(void *data)
 {
 	struct async_dbus_op *op = data;
 
-	dbus_message_unref(op->msg);
+	if (op->msg)
+		dbus_message_unref(op->msg);
+
 	free(op);
 }
 
@@ -689,12 +694,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
 static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
 {
-	dbus_bool_t notifying = FALSE;
-
-	/*
-	 * TODO: Return the correct value here once StartNotify and StopNotify
-	 * methods are implemented.
-	 */
+	struct characteristic *chrc = data;
+	dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
 
 	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
 
@@ -925,11 +926,240 @@ fail:
 	return btd_error_not_supported(msg);
 }
 
+struct notify_client {
+	struct characteristic *chrc;
+	int ref_count;
+	char *owner;
+	guint watch;
+	unsigned int notify_id;
+};
+
+static void notify_client_free(struct notify_client *client)
+{
+	DBG("owner %s", client->owner);
+
+	g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
+	free(client->owner);
+	free(client);
+}
+
+static void notify_client_unref(void *data)
+{
+	struct notify_client *client = data;
+
+	DBG("owner %s", client->owner);
+
+	if (__sync_sub_and_fetch(&client->ref_count, 1))
+		return;
+
+	notify_client_free(client);
+}
+
+static struct notify_client *notify_client_ref(struct notify_client *client)
+{
+	DBG("owner %s", client->owner);
+
+	__sync_fetch_and_add(&client->ref_count, 1);
+
+	return client;
+}
+
+static bool match_notifying(const void *a, const void *b)
+{
+	const struct notify_client *client = a;
+
+	return !!client->notify_id;
+}
+
+static void update_notifying(struct characteristic *chrc)
+{
+	if (!chrc->notifying)
+		return;
+
+	if (queue_find(chrc->notify_clients, match_notifying, NULL))
+		return;
+
+	chrc->notifying = false;
+
+	g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
+						GATT_CHARACTERISTIC_IFACE,
+						"Notifying");
+}
+
+static void notify_client_disconnect(DBusConnection *conn, void *user_data)
+{
+	struct notify_client *client = user_data;
+	struct characteristic *chrc = client->chrc;
+	struct bt_gatt_client *gatt = chrc->service->client->gatt;
+
+	DBG("owner %s", client->owner);
+
+	queue_remove(chrc->notify_clients, client);
+	bt_gatt_client_unregister_notify(gatt, client->notify_id);
+
+	update_notifying(chrc);
+
+	notify_client_unref(client);
+}
+
+static struct notify_client *notify_client_create(struct characteristic *chrc,
+							const char *owner)
+{
+	struct notify_client *client;
+
+	client = new0(struct notify_client, 1);
+	if (!client)
+		return NULL;
+
+	client->chrc = chrc;
+	client->owner = strdup(owner);
+	if (!client->owner) {
+		free(client);
+		return NULL;
+	}
+
+	client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
+						owner, notify_client_disconnect,
+						client, NULL);
+	if (!client->watch) {
+		free(client->owner);
+		free(client);
+		return NULL;
+	}
+
+	return notify_client_ref(client);
+}
+
+static bool match_notify_sender(const void *a, const void *b)
+{
+	const struct notify_client *client = a;
+	const char *sender = b;
+
+	return strcmp(client->owner, sender) == 0;
+}
+
+static void notify_cb(uint16_t value_handle, const uint8_t *value,
+					uint16_t length, void *user_data)
+{
+	struct async_dbus_op *op = user_data;
+	struct notify_client *client = op->data;
+	struct characteristic *chrc = client->chrc;
+
+	/*
+	 * Even if the value didn't change, we want to send a PropertiesChanged
+	 * signal so that we propagate the notification/indication to
+	 * applications.
+	 */
+	gatt_db_attribute_reset(chrc->attr);
+	gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
+						write_characteristic_cb, chrc);
+}
+
+static void register_notify_cb(unsigned int id, uint16_t att_ecode,
+								void *user_data)
+{
+	struct async_dbus_op *op = user_data;
+	struct notify_client *client = op->data;
+	struct characteristic *chrc = client->chrc;
+	DBusMessage *reply;
+
+	/* Make sure that an interim disconnect did not remove the client */
+	if (!queue_find(chrc->notify_clients, NULL, client)) {
+		bt_gatt_client_unregister_notify(chrc->service->client->gatt,
+									id);
+		notify_client_unref(client);
+
+		reply = btd_error_failed(op->msg,
+						"Characteristic not available");
+		goto done;
+	}
+
+	/*
+	 * Drop the reference count that we added when registering the callback.
+	 */
+	notify_client_unref(client);
+
+	if (!id) {
+		queue_remove(chrc->notify_clients, client);
+		notify_client_free(client);
+
+		reply = create_gatt_dbus_error(op->msg, att_ecode);
+
+		goto done;
+	}
+
+	client->notify_id = id;
+
+	if (!chrc->notifying) {
+		chrc->notifying = true;
+		g_dbus_emit_property_changed(btd_get_dbus_connection(),
+					chrc->path, GATT_CHARACTERISTIC_IFACE,
+					"Notifying");
+	}
+
+	reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
+
+done:
+	if (reply)
+		g_dbus_send_message(btd_get_dbus_connection(), reply);
+	else
+		error("Failed to construct D-Bus message reply");
+
+	dbus_message_unref(op->msg);
+	op->msg = NULL;
+}
+
 static DBusMessage *characteristic_start_notify(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
-	/* TODO: Implement */
-	return btd_error_failed(msg, "Not implemented");
+	struct characteristic *chrc = user_data;
+	struct bt_gatt_client *gatt = chrc->service->client->gatt;
+	const char *sender = dbus_message_get_sender(msg);
+	struct async_dbus_op *op;
+	struct notify_client *client;
+
+	if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
+				chrc->props & BT_GATT_CHRC_PROP_INDICATE))
+		return btd_error_not_supported(msg);
+
+	/* Each client can only have one active notify session. */
+	client = queue_find(chrc->notify_clients, match_notify_sender, sender);
+	if (client)
+		return client->notify_id ?
+				btd_error_failed(msg, "Already notifying") :
+				btd_error_in_progress(msg);
+
+	client = notify_client_create(chrc, sender);
+	if (!client)
+		return btd_error_failed(msg, "Failed allocate notify session");
+
+	op = new0(struct async_dbus_op, 1);
+	if (!op) {
+		notify_client_unref(client);
+		return btd_error_failed(msg, "Failed to initialize request");
+	}
+
+	/*
+	 * Add to the ref count so that a disconnect during register won't free
+	 * the client instance.
+	 */
+	op->data = notify_client_ref(client);
+	op->msg = dbus_message_ref(msg);
+
+	queue_push_tail(chrc->notify_clients, client);
+
+	if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
+						register_notify_cb, notify_cb,
+						op, async_dbus_op_free))
+		return NULL;
+
+	queue_remove(chrc->notify_clients, client);
+	async_dbus_op_free(op);
+
+	/* Directly free the client */
+	notify_client_free(client);
+
+	return btd_error_failed(msg, "Failed to register notify session");
 }
 
 static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
@@ -1022,6 +1252,13 @@ static struct characteristic *characteristic_create(
 		return NULL;
 	}
 
+	chrc->notify_clients = queue_new();
+	if (!chrc->notify_clients) {
+		queue_destroy(chrc->descs, NULL);
+		free(chrc);
+		return NULL;
+	}
+
 	chrc->service = service;
 
 	gatt_db_attribute_get_char_data(attr, &chrc->handle,
@@ -1064,6 +1301,7 @@ static void unregister_characteristic(void *data)
 	if (chrc->write_id)
 		bt_gatt_client_cancel(gatt, chrc->write_id);
 
+	queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
 	queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
 
 	g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 2/5] core: gatt: Implement GattCharacteristic1.StopNotify

[Arman Uguray]

This patch implements the StopNotify method of
org.bluez.GattCharacteristic1.

Change-Id: I2c941c9f1cdbfb1e8a02257b01fc7377b035e28d
---
 src/gatt-client.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 2f187ad..a3cebca 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1165,8 +1165,25 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
 static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
-	/* TODO: Implement */
-	return btd_error_failed(msg, "Not implemented");
+	struct characteristic *chrc = user_data;
+	struct bt_gatt_client *gatt = chrc->service->client->gatt;
+	const char *sender = dbus_message_get_sender(msg);
+	struct notify_client *client;
+
+	if (!chrc->notifying)
+		return btd_error_failed(msg, "Not notifying");
+
+	client = queue_remove_if(chrc->notify_clients, match_notify_sender,
+							(void *) sender);
+	if (!client)
+		return btd_error_failed(msg, "No notify session started");
+
+	bt_gatt_client_unregister_notify(gatt, client->notify_id);
+	update_notifying(chrc);
+
+	notify_client_unref(client);
+
+	return dbus_message_new_method_return(msg);
 }
 
 static void append_desc_path(void *data, void *user_data)
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 3/5] core: device: Don't check ready in service_removed

[Arman Uguray]

shared/gatt-client clears a given gatt-db if there's an error during its
init sequence, even if the given gatt-db was previously populated (e.g.
from a cache). This is to make sure that the database contents are at no
point invalid.

This patch removes a check for bt_gatt_client_is_ready and the
corresponding early-return from btd_device's service_removed handler, so
that other layers can be notified of invalidated gatt_db_attribute
pointers.

Change-Id: I8e188d0d0f81398b89f63183ed0708830b6dd9c5
---
 src/device.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/device.c b/src/device.c
index 8dea70b..9cb5acb 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2762,8 +2762,21 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
 	struct gatt_primary *prim;
 	uint16_t start, end;
 
-	if (!bt_gatt_client_is_ready(device->client))
-		return;
+	/*
+	 * NOTE: shared/gatt-client clears the database in case of failure. This
+	 * triggers the service_removed callback for all affected services.
+	 * Hence, this function will be called in the following cases:
+	 *
+	 *    1. When a GATT service gets removed due to "Service Changed".
+	 *
+	 *    2. When a GATT service gets removed when the database get cleared
+	 *       upon disconnection with a non-bonded device.
+	 *
+	 *    3. When a GATT service gets removed when the database get cleared
+	 *       by shared/gatt-client when its initialization procedure fails,
+	 *       e.g. due to an ATT protocol error or an unexpected disconnect.
+	 *       In this case the gatt-client will not be ready.
+	 */
 
 	gatt_db_attribute_get_service_handles(attr, &start, &end);
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 4/5] core: device: Add device_is_bonded_for_gatt

[Arman Uguray]

Added the device_is_bonded_for_gatt function, which returns true if
the bonding state is true for the bearer over which the GATT db was
populated. For now, this only returns the LE state but added for
better layering, so that future changes to the internal logic can be
simply performed here to avoid regressions.

Change-Id: I6ebd16a1111aa38645a0a072536f4171d246b931
---
 src/device.c | 14 +++++++++++++-
 src/device.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index 9cb5acb..2ef2b6a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -506,8 +506,10 @@ static void gatt_client_cleanup(struct btd_device *device)
 	bt_gatt_client_unref(device->client);
 	device->client = NULL;
 
-	if (!device->le_state.bonded)
+	if (!device_is_bonded_for_gatt(device)) {
+		DBG("Device is not bonded, clearing client attribute cache");
 		gatt_db_clear(device->db);
+	}
 }
 
 static void attio_cleanup(struct btd_device *device)
@@ -638,6 +640,16 @@ bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type)
 	return state->bonded;
 }
 
+bool device_is_bonded_for_gatt(struct btd_device *device)
+{
+	/*
+	 * TODO: Once we properly support GATT over BR/EDR, we should remember
+	 * whether the GATT client  database was populated over LE or BR/EDR and
+	 * check the correct bonding state.
+	 */
+	return device->le_state.bonded;
+}
+
 gboolean device_is_trusted(struct btd_device *device)
 {
 	return device->trusted;
diff --git a/src/device.h b/src/device.h
index a7fefee..b4aead1 100644
--- a/src/device.h
+++ b/src/device.h
@@ -82,6 +82,7 @@ const char *device_get_path(const struct btd_device *device);
 gboolean device_is_temporary(struct btd_device *device);
 bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
 bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
+bool device_is_bonded_for_gatt(struct btd_device *device);
 gboolean device_is_trusted(struct btd_device *device);
 void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
 void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH BlueZ 5/5] core: gatt: Keep objects over disconnects

[Arman Uguray]

This patch changes the way that the life-time for exported GATT API
objects are managed, so that they are kept alive in the case of a
disconnection if the devices are bonded. In this case, all API method
calls return an error except for StartNotify/StopNotify.

All notification sessions that were initiated during and in-between
connections are maintained and a notification callback is registered
for each session immediately on reconnection.

Change-Id: Ib7ac72a91dd8dc4b631c0948b4bd41af0bd4ff7b
---
 src/gatt-client.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 187 insertions(+), 21 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index a3cebca..6c83f11 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -58,6 +58,7 @@ struct btd_gatt_client {
 	struct bt_gatt_client *gatt;
 
 	struct queue *services;
+	struct queue *all_notify_clients;
 };
 
 struct service {
@@ -387,6 +388,9 @@ static DBusMessage *descriptor_read_value(DBusConnection *conn,
 	struct bt_gatt_client *gatt = desc->chrc->service->client->gatt;
 	struct async_dbus_op *op;
 
+	if (!gatt)
+		return btd_error_failed(msg, "Not connected");
+
 	if (desc->read_id)
 		return btd_error_in_progress(msg);
 
@@ -521,6 +525,9 @@ static DBusMessage *descriptor_write_value(DBusConnection *conn,
 	uint8_t *value = NULL;
 	size_t value_len = 0;
 
+	if (!gatt)
+		return btd_error_failed(msg, "Not connected");
+
 	if (desc->write_id)
 		return btd_error_in_progress(msg);
 
@@ -815,6 +822,9 @@ static DBusMessage *characteristic_read_value(DBusConnection *conn,
 	struct bt_gatt_client *gatt = chrc->service->client->gatt;
 	struct async_dbus_op *op;
 
+	if (!gatt)
+		return btd_error_failed(msg, "Not connected");
+
 	if (chrc->read_id)
 		return btd_error_in_progress(msg);
 
@@ -859,6 +869,9 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
 	size_t value_len = 0;
 	bool supported = false;
 
+	if (!gatt)
+		return btd_error_failed(msg, "Not connected");
+
 	if (chrc->write_id)
 		return btd_error_in_progress(msg);
 
@@ -995,6 +1008,7 @@ static void notify_client_disconnect(DBusConnection *conn, void *user_data)
 	DBG("owner %s", client->owner);
 
 	queue_remove(chrc->notify_clients, client);
+	queue_remove(chrc->service->client->all_notify_clients, client);
 	bt_gatt_client_unregister_notify(gatt, client->notify_id);
 
 	update_notifying(chrc);
@@ -1055,6 +1069,31 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
 						write_characteristic_cb, chrc);
 }
 
+static DBusMessage *create_notify_reply(struct async_dbus_op *op,
+						bool success, uint8_t att_ecode)
+{
+	DBusMessage *reply = NULL;
+
+	if (!op->msg)
+		return NULL;
+
+	if (success)
+		reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
+	else if (att_ecode)
+		reply = create_gatt_dbus_error(op->msg, att_ecode);
+	else
+		reply = btd_error_failed(op->msg,
+						"Characteristic not available");
+
+	dbus_message_unref(op->msg);
+	op->msg = NULL;
+
+	if (!reply)
+		error("Failed to construct D-Bus message reply");
+
+	return reply;
+}
+
 static void register_notify_cb(unsigned int id, uint16_t att_ecode,
 								void *user_data)
 {
@@ -1069,8 +1108,8 @@ static void register_notify_cb(unsigned int id, uint16_t att_ecode,
 									id);
 		notify_client_unref(client);
 
-		reply = btd_error_failed(op->msg,
-						"Characteristic not available");
+		reply = create_notify_reply(op, false, 0);
+
 		goto done;
 	}
 
@@ -1081,9 +1120,10 @@ static void register_notify_cb(unsigned int id, uint16_t att_ecode,
 
 	if (!id) {
 		queue_remove(chrc->notify_clients, client);
+		queue_remove(chrc->service->client->all_notify_clients, client);
 		notify_client_free(client);
 
-		reply = create_gatt_dbus_error(op->msg, att_ecode);
+		reply = create_notify_reply(op, false, att_ecode);
 
 		goto done;
 	}
@@ -1097,16 +1137,11 @@ static void register_notify_cb(unsigned int id, uint16_t att_ecode,
 					"Notifying");
 	}
 
-	reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
+	reply = create_notify_reply(op, true, 0);
 
 done:
 	if (reply)
 		g_dbus_send_message(btd_get_dbus_connection(), reply);
-	else
-		error("Failed to construct D-Bus message reply");
-
-	dbus_message_unref(op->msg);
-	op->msg = NULL;
 }
 
 static DBusMessage *characteristic_start_notify(DBusConnection *conn,
@@ -1133,12 +1168,33 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
 	if (!client)
 		return btd_error_failed(msg, "Failed allocate notify session");
 
-	op = new0(struct async_dbus_op, 1);
-	if (!op) {
-		notify_client_unref(client);
-		return btd_error_failed(msg, "Failed to initialize request");
+	queue_push_tail(chrc->notify_clients, client);
+	queue_push_tail(chrc->service->client->all_notify_clients, client);
+
+	/*
+	 * If the device is currently not connected, return success. We will
+	 * automatically try and register all clients when a GATT client becomes
+	 * ready.
+	 */
+	if (!gatt) {
+		DBusMessage *reply;
+
+		reply = g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+		if (reply)
+			return reply;
+
+		/*
+		 * Clean up and respond with an error instead of timing out to
+		 * avoid any ambiguities.
+		 */
+		error("Failed to construct D-Bus message reply");
+		goto fail;
 	}
 
+	op = new0(struct async_dbus_op, 1);
+	if (!op)
+		goto fail;
+
 	/*
 	 * Add to the ref count so that a disconnect during register won't free
 	 * the client instance.
@@ -1146,16 +1202,17 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
 	op->data = notify_client_ref(client);
 	op->msg = dbus_message_ref(msg);
 
-	queue_push_tail(chrc->notify_clients, client);
-
 	if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
 						register_notify_cb, notify_cb,
 						op, async_dbus_op_free))
 		return NULL;
 
-	queue_remove(chrc->notify_clients, client);
 	async_dbus_op_free(op);
 
+fail:
+	queue_remove(chrc->notify_clients, client);
+	queue_remove(chrc->service->client->all_notify_clients, client);
+
 	/* Directly free the client */
 	notify_client_free(client);
 
@@ -1178,6 +1235,7 @@ static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
 	if (!client)
 		return btd_error_failed(msg, "No notify session started");
 
+	queue_remove(chrc->service->client->all_notify_clients, client);
 	bt_gatt_client_unregister_notify(gatt, client->notify_id);
 	update_notifying(chrc);
 
@@ -1682,6 +1740,13 @@ struct btd_gatt_client *btd_gatt_client_new(struct btd_device *device)
 		return NULL;
 	}
 
+	client->all_notify_clients = queue_new();
+	if (!client->all_notify_clients) {
+		queue_destroy(client->services, NULL);
+		free(client);
+		return NULL;
+	}
+
 	client->device = device;
 	ba2str(device_get_address(device), client->devaddr);
 
@@ -1696,11 +1761,43 @@ void btd_gatt_client_destroy(struct btd_gatt_client *client)
 		return;
 
 	queue_destroy(client->services, unregister_service);
+	queue_destroy(client->all_notify_clients, NULL);
 	bt_gatt_client_unref(client->gatt);
 	gatt_db_unref(client->db);
 	free(client);
 }
 
+static void register_notify(void *data, void *user_data)
+{
+	struct notify_client *notify_client = data;
+	struct btd_gatt_client *client = user_data;
+	struct async_dbus_op *op;
+
+	DBG("Re-register subscribed notification client");
+
+	op = new0(struct async_dbus_op, 1);
+	if (!op)
+		goto fail;
+
+	op->data = notify_client_ref(notify_client);
+
+	if (bt_gatt_client_register_notify(client->gatt,
+					notify_client->chrc->value_handle,
+					register_notify_cb, notify_cb,
+					op, async_dbus_op_free))
+		return;
+
+	async_dbus_op_free(op);
+
+fail:
+	DBG("Failed to re-register notification client");
+
+	queue_remove(notify_client->chrc->notify_clients, client);
+	queue_remove(client->all_notify_clients, client);
+
+	notify_client_free(notify_client);
+}
+
 void btd_gatt_client_ready(struct btd_gatt_client *client)
 {
 	struct bt_gatt_client *gatt;
@@ -1719,7 +1816,19 @@ void btd_gatt_client_ready(struct btd_gatt_client *client)
 
 	client->ready = true;
 
-	create_services(client);
+	DBG("GATT client ready");
+
+	if (queue_isempty(client->services)) {
+		DBG("Exporting services");
+		create_services(client);
+		return;
+	}
+
+	/*
+	 * Services have already been created before. Re-enable notifications
+	 * for any pre-registered notification sessions.
+	 */
+	queue_foreach(client->all_notify_clients, register_notify, client);
 }
 
 void btd_gatt_client_service_added(struct btd_gatt_client *client,
@@ -1757,18 +1866,75 @@ void btd_gatt_client_service_removed(struct btd_gatt_client *client,
 						unregister_service);
 }
 
+static void clear_notify_id(void *data, void *user_data)
+{
+	struct notify_client *client = data;
+
+	client->notify_id = 0;
+}
+
+static void cancel_desc_ops(void *data, void *user_data)
+{
+	struct descriptor *desc = data;
+	struct bt_gatt_client *gatt = user_data;
+
+	if (desc->read_id) {
+		bt_gatt_client_cancel(gatt, desc->read_id);
+		desc->read_id = 0;
+	}
+
+	if (desc->write_id) {
+		bt_gatt_client_cancel(gatt, desc->write_id);
+		desc->write_id = 0;
+	}
+}
+
+static void cancel_chrc_ops(void *data, void *user_data)
+{
+	struct characteristic *chrc = data;
+	struct bt_gatt_client *gatt = user_data;
+
+	if (chrc->read_id) {
+		bt_gatt_client_cancel(gatt, chrc->read_id);
+		chrc->read_id = 0;
+	}
+
+	if (chrc->write_id) {
+		bt_gatt_client_cancel(gatt, chrc->write_id);
+		chrc->write_id = 0;
+	}
+
+	queue_foreach(chrc->descs, cancel_desc_ops, user_data);
+}
+
+static void cancel_ops(void *data, void *user_data)
+{
+	struct service *service = data;
+
+	queue_foreach(service->chrcs, cancel_chrc_ops, user_data);
+}
+
 void btd_gatt_client_disconnected(struct btd_gatt_client *client)
 {
-	if (!client)
+	if (!client || !client->gatt)
 		return;
 
-	DBG("Device disconnected. Cleaning up");
+	DBG("Device disconnected. Cleaning up.");
 
 	/*
 	 * Remove all services. We'll recreate them when a new bt_gatt_client
-	 * becomes ready.
+	 * becomes ready. Leave the services around if the device is bonded.
 	 */
-	queue_remove_all(client->services, NULL, NULL, unregister_service);
+	if (!device_is_bonded_for_gatt(client->device)) {
+		DBG("Device not bonded. Removing exported services.");
+		queue_remove_all(client->services, NULL, NULL,
+							unregister_service);
+	} else {
+		DBG("Device is bonded. Keeping exported services up.");
+		queue_foreach(client->all_notify_clients, clear_notify_id,
+									NULL);
+		queue_foreach(client->services, cancel_ops, client->gatt);
+	}
 
 	bt_gatt_client_unref(client->gatt);
 	client->gatt = NULL;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH BlueZ 0/5] core: gatt: Support enabling notifications

[Arman Uguray]

Hi,

> On Wed, Jan 28, 2015 at 6:10 PM, Arman Uguray <armansito@chromium.org> wrote:
> This patch sets brings support for enabling notifications while in GATT
> client-role. The changes that are introduced are:
>
>   1. Implemented the StartNotify and StopNotify methods of the
>      GattCharacteristic1 interface. This are internally tied to
>      bt_gatt_client_register_notify and bt_gatt_client_unregister_notify.
>      These also manage notification sessions on a per dbus sender basis.
>
>   2. The exported GATT API objects are not removed in the event of a disconnect,
>      if the devices are bonded. All notification sessions are also persisted and
>      automatically re-enabled on reconnection. Also, adding new notification
>      sessions via StartNotify is allowed during disconnects.
>
> Arman Uguray (5):
>   core: gatt: Implement GattCharacteristic1.StartNotify
>   core: gatt: Implement GattCharacteristic1.StopNotify
>   core: device: Don't check ready in service_removed
>   core: device: Add device_is_bonded_for_gatt
>   core: gatt: Keep objects over disconnects
>
>  src/device.c      |  31 +++-
>  src/device.h      |   1 +
>  src/gatt-client.c | 453 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 466 insertions(+), 19 deletions(-)
>
> --
> 2.2.0.rc0.207.ga3a616c
>

I forgot to remove the Change-Id lines from the commit messages, so
please ignore/remove them.

Thanks,
Arman

Re: [PATCH BlueZ 1/5] core: gatt: Implement GattCharacteristic1.StartNotify

[Luiz Augusto von Dentz]

Hi Arman,

On Thu, Jan 29, 2015 at 4:10 AM, Arman Uguray <armansito@chromium.org> wrote:
> This patch implements the StartNotify method of
> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
> session to the D-Bus sender which internally registers a unique
> notify handler with the bt_gatt_client. Each received notification
> or indication causes a PropertiesChanged signal to be emitted for the
> "Value" property.
>
> The notify handler gets automatically unregistered when sender
> disconnects from D-Bus.
>
> Change-Id: Ia805127613ee538eced4591ba0229789dc54fab8
> ---
>  src/gatt-client.c | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 247 insertions(+), 9 deletions(-)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index cb8ddf6..2f187ad 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -88,6 +88,9 @@ struct characteristic {
>         unsigned int write_id;
>
>         struct queue *descs;
> +
> +       bool notifying;
> +       struct queue *notify_clients;
>  };
>
>  struct descriptor {
> @@ -231,7 +234,9 @@ static void async_dbus_op_free(void *data)
>  {
>         struct async_dbus_op *op = data;
>
> -       dbus_message_unref(op->msg);
> +       if (op->msg)
> +               dbus_message_unref(op->msg);
> +
>         free(op);
>  }
>
> @@ -689,12 +694,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
>  static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *data)
>  {
> -       dbus_bool_t notifying = FALSE;
> -
> -       /*
> -        * TODO: Return the correct value here once StartNotify and StopNotify
> -        * methods are implemented.
> -        */
> +       struct characteristic *chrc = data;
> +       dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>
>         dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
>
> @@ -925,11 +926,240 @@ fail:
>         return btd_error_not_supported(msg);
>  }
>
> +struct notify_client {
> +       struct characteristic *chrc;
> +       int ref_count;
> +       char *owner;
> +       guint watch;
> +       unsigned int notify_id;
> +};
> +
> +static void notify_client_free(struct notify_client *client)
> +{
> +       DBG("owner %s", client->owner);
> +
> +       g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
> +       free(client->owner);
> +       free(client);
> +}
> +
> +static void notify_client_unref(void *data)
> +{
> +       struct notify_client *client = data;
> +
> +       DBG("owner %s", client->owner);
> +
> +       if (__sync_sub_and_fetch(&client->ref_count, 1))
> +               return;
> +
> +       notify_client_free(client);
> +}
> +
> +static struct notify_client *notify_client_ref(struct notify_client *client)
> +{
> +       DBG("owner %s", client->owner);
> +
> +       __sync_fetch_and_add(&client->ref_count, 1);
> +
> +       return client;
> +}
> +
> +static bool match_notifying(const void *a, const void *b)
> +{
> +       const struct notify_client *client = a;
> +
> +       return !!client->notify_id;
> +}
> +
> +static void update_notifying(struct characteristic *chrc)
> +{
> +       if (!chrc->notifying)
> +               return;
> +
> +       if (queue_find(chrc->notify_clients, match_notifying, NULL))
> +               return;
> +
> +       chrc->notifying = false;
> +
> +       g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
> +                                               GATT_CHARACTERISTIC_IFACE,
> +                                               "Notifying");
> +}
> +
> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
> +{
> +       struct notify_client *client = user_data;
> +       struct characteristic *chrc = client->chrc;
> +       struct bt_gatt_client *gatt = chrc->service->client->gatt;
> +
> +       DBG("owner %s", client->owner);
> +
> +       queue_remove(chrc->notify_clients, client);
> +       bt_gatt_client_unregister_notify(gatt, client->notify_id);
> +
> +       update_notifying(chrc);
> +
> +       notify_client_unref(client);
> +}
> +
> +static struct notify_client *notify_client_create(struct characteristic *chrc,
> +                                                       const char *owner)
> +{
> +       struct notify_client *client;
> +
> +       client = new0(struct notify_client, 1);
> +       if (!client)
> +               return NULL;
> +
> +       client->chrc = chrc;
> +       client->owner = strdup(owner);
> +       if (!client->owner) {
> +               free(client);
> +               return NULL;
> +       }
> +
> +       client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
> +                                               owner, notify_client_disconnect,
> +                                               client, NULL);
> +       if (!client->watch) {
> +               free(client->owner);
> +               free(client);
> +               return NULL;
> +       }
> +
> +       return notify_client_ref(client);
> +}
> +
> +static bool match_notify_sender(const void *a, const void *b)
> +{
> +       const struct notify_client *client = a;
> +       const char *sender = b;
> +
> +       return strcmp(client->owner, sender) == 0;
> +}
> +
> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
> +                                       uint16_t length, void *user_data)
> +{
> +       struct async_dbus_op *op = user_data;
> +       struct notify_client *client = op->data;
> +       struct characteristic *chrc = client->chrc;
> +
> +       /*
> +        * Even if the value didn't change, we want to send a PropertiesChanged
> +        * signal so that we propagate the notification/indication to
> +        * applications.
> +        */
> +       gatt_db_attribute_reset(chrc->attr);
> +       gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
> +                                               write_characteristic_cb, chrc);
> +}
> +
> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
> +                                                               void *user_data)
> +{
> +       struct async_dbus_op *op = user_data;
> +       struct notify_client *client = op->data;
> +       struct characteristic *chrc = client->chrc;
> +       DBusMessage *reply;
> +
> +       /* Make sure that an interim disconnect did not remove the client */
> +       if (!queue_find(chrc->notify_clients, NULL, client)) {
> +               bt_gatt_client_unregister_notify(chrc->service->client->gatt,
> +                                                                       id);
> +               notify_client_unref(client);
> +
> +               reply = btd_error_failed(op->msg,
> +                                               "Characteristic not available");
> +               goto done;
> +       }
> +
> +       /*
> +        * Drop the reference count that we added when registering the callback.
> +        */
> +       notify_client_unref(client);
> +
> +       if (!id) {
> +               queue_remove(chrc->notify_clients, client);
> +               notify_client_free(client);
> +
> +               reply = create_gatt_dbus_error(op->msg, att_ecode);
> +
> +               goto done;
> +       }
> +
> +       client->notify_id = id;
> +
> +       if (!chrc->notifying) {
> +               chrc->notifying = true;
> +               g_dbus_emit_property_changed(btd_get_dbus_connection(),
> +                                       chrc->path, GATT_CHARACTERISTIC_IFACE,
> +                                       "Notifying");
> +       }
> +
> +       reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
> +
> +done:
> +       if (reply)
> +               g_dbus_send_message(btd_get_dbus_connection(), reply);
> +       else
> +               error("Failed to construct D-Bus message reply");
> +
> +       dbus_message_unref(op->msg);
> +       op->msg = NULL;

The call to dbus_message_unref can probably be done in notify_client_free.

> +}
> +
>  static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>                                         DBusMessage *msg, void *user_data)
>  {
> -       /* TODO: Implement */
> -       return btd_error_failed(msg, "Not implemented");
> +       struct characteristic *chrc = user_data;
> +       struct bt_gatt_client *gatt = chrc->service->client->gatt;
> +       const char *sender = dbus_message_get_sender(msg);
> +       struct async_dbus_op *op;
> +       struct notify_client *client;
> +
> +       if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
> +                               chrc->props & BT_GATT_CHRC_PROP_INDICATE))
> +               return btd_error_not_supported(msg);
> +
> +       /* Each client can only have one active notify session. */
> +       client = queue_find(chrc->notify_clients, match_notify_sender, sender);
> +       if (client)
> +               return client->notify_id ?
> +                               btd_error_failed(msg, "Already notifying") :
> +                               btd_error_in_progress(msg);
> +
> +       client = notify_client_create(chrc, sender);
> +       if (!client)
> +               return btd_error_failed(msg, "Failed allocate notify session");
> +
> +       op = new0(struct async_dbus_op, 1);
> +       if (!op) {
> +               notify_client_unref(client);
> +               return btd_error_failed(msg, "Failed to initialize request");
> +       }
> +
> +       /*
> +        * Add to the ref count so that a disconnect during register won't free
> +        * the client instance.
> +        */
> +       op->data = notify_client_ref(client);
> +       op->msg = dbus_message_ref(msg);
> +
> +       queue_push_tail(chrc->notify_clients, client);
> +
> +       if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
> +                                               register_notify_cb, notify_cb,
> +                                               op, async_dbus_op_free))
> +               return NULL;

We better stop this pattern of doing non-cancelable operation, not
only we cannot cancel any of the async_dbus_op it actually doesn't
track the caller and we might have to store the op in the
characteristic because in the event of the later being freed we will
probably crash on the callback.

Usually the pattern we had used for handling pending operation is to
attach them to the resource being passed as user_data, not the other
way around like you doing, take a look at
android/hog.c:set_and_store_gatt_req it stores the id and then attach
to the resource so in case anything happens to it the request can be
cancelled.

> +       queue_remove(chrc->notify_clients, client);
> +       async_dbus_op_free(op);
> +
> +       /* Directly free the client */
> +       notify_client_free(client);
> +
> +       return btd_error_failed(msg, "Failed to register notify session");
>  }
>
>  static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
> @@ -1022,6 +1252,13 @@ static struct characteristic *characteristic_create(
>                 return NULL;
>         }
>
> +       chrc->notify_clients = queue_new();
> +       if (!chrc->notify_clients) {
> +               queue_destroy(chrc->descs, NULL);
> +               free(chrc);
> +               return NULL;
> +       }
> +
>         chrc->service = service;
>
>         gatt_db_attribute_get_char_data(attr, &chrc->handle,
> @@ -1064,6 +1301,7 @@ static void unregister_characteristic(void *data)
>         if (chrc->write_id)
>                 bt_gatt_client_cancel(gatt, chrc->write_id);
>
> +       queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>         queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>
>         g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> --
> 2.2.0.rc0.207.ga3a616c
>



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 4/5] core: device: Add device_is_bonded_for_gatt

[Luiz Augusto von Dentz]

Hi Arman,

On Thu, Jan 29, 2015 at 4:11 AM, Arman Uguray <armansito@chromium.org> wrote:
> Added the device_is_bonded_for_gatt function, which returns true if
> the bonding state is true for the bearer over which the GATT db was
> populated. For now, this only returns the LE state but added for
> better layering, so that future changes to the internal logic can be
> simply performed here to avoid regressions.
>
> Change-Id: I6ebd16a1111aa38645a0a072536f4171d246b931
> ---
>  src/device.c | 14 +++++++++++++-
>  src/device.h |  1 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/device.c b/src/device.c
> index 9cb5acb..2ef2b6a 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -506,8 +506,10 @@ static void gatt_client_cleanup(struct btd_device *device)
>         bt_gatt_client_unref(device->client);
>         device->client = NULL;
>
> -       if (!device->le_state.bonded)
> +       if (!device_is_bonded_for_gatt(device)) {
> +               DBG("Device is not bonded, clearing client attribute cache");
>                 gatt_db_clear(device->db);
> +       }
>  }
>
>  static void attio_cleanup(struct btd_device *device)
> @@ -638,6 +640,16 @@ bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type)
>         return state->bonded;
>  }
>
> +bool device_is_bonded_for_gatt(struct btd_device *device)
> +{
> +       /*
> +        * TODO: Once we properly support GATT over BR/EDR, we should remember
> +        * whether the GATT client  database was populated over LE or BR/EDR and
> +        * check the correct bonding state.
> +        */
> +       return device->le_state.bonded;
> +}
> +
>  gboolean device_is_trusted(struct btd_device *device)
>  {
>         return device->trusted;
> diff --git a/src/device.h b/src/device.h
> index a7fefee..b4aead1 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -82,6 +82,7 @@ const char *device_get_path(const struct btd_device *device);
>  gboolean device_is_temporary(struct btd_device *device);
>  bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
>  bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
> +bool device_is_bonded_for_gatt(struct btd_device *device);

Isn't this equivalent to device_is_bonded or perhaps we don't the
type? I guess it is possible to retrieve in what transport we are
connected and then check if we it is bonded.

>  gboolean device_is_trusted(struct btd_device *device);
>  void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
>  void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
> --
> 2.2.0.rc0.207.ga3a616c
>



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 4/5] core: device: Add device_is_bonded_for_gatt

[Arman Uguray]

Hi Luiz,

> On Thu, Jan 29, 2015 at 5:52 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Thu, Jan 29, 2015 at 4:11 AM, Arman Uguray <armansito@chromium.org> wrote:
>> Added the device_is_bonded_for_gatt function, which returns true if
>> the bonding state is true for the bearer over which the GATT db was
>> populated. For now, this only returns the LE state but added for
>> better layering, so that future changes to the internal logic can be
>> simply performed here to avoid regressions.
>>
>> Change-Id: I6ebd16a1111aa38645a0a072536f4171d246b931
>> ---
>>  src/device.c | 14 +++++++++++++-
>>  src/device.h |  1 +
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/device.c b/src/device.c
>> index 9cb5acb..2ef2b6a 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -506,8 +506,10 @@ static void gatt_client_cleanup(struct btd_device *device)
>>         bt_gatt_client_unref(device->client);
>>         device->client = NULL;
>>
>> -       if (!device->le_state.bonded)
>> +       if (!device_is_bonded_for_gatt(device)) {
>> +               DBG("Device is not bonded, clearing client attribute cache");
>>                 gatt_db_clear(device->db);
>> +       }
>>  }
>>
>>  static void attio_cleanup(struct btd_device *device)
>> @@ -638,6 +640,16 @@ bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type)
>>         return state->bonded;
>>  }
>>
>> +bool device_is_bonded_for_gatt(struct btd_device *device)
>> +{
>> +       /*
>> +        * TODO: Once we properly support GATT over BR/EDR, we should remember
>> +        * whether the GATT client  database was populated over LE or BR/EDR and
>> +        * check the correct bonding state.
>> +        */
>> +       return device->le_state.bonded;
>> +}
>> +
>>  gboolean device_is_trusted(struct btd_device *device)
>>  {
>>         return device->trusted;
>> diff --git a/src/device.h b/src/device.h
>> index a7fefee..b4aead1 100644
>> --- a/src/device.h
>> +++ b/src/device.h
>> @@ -82,6 +82,7 @@ const char *device_get_path(const struct btd_device *device);
>>  gboolean device_is_temporary(struct btd_device *device);
>>  bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
>>  bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
>> +bool device_is_bonded_for_gatt(struct btd_device *device);
>
> Isn't this equivalent to device_is_bonded or perhaps we don't the
> type? I guess it is possible to retrieve in what transport we are
> connected and then check if we it is bonded.
>

It's pretty much the same thing, except it would return the bonding
state based on the transport that did the GATT discovery. I figured it
would be better to have this function in one place so that we only
have one place to change when we properly support GATT over BR/EDR in
the future. Just using device_is_bonded and passing BDADDR_LE_PUBLIC
for the bdaddr works too for now, we just need to remember to update
them later. I'll go ahead and leave this out for now.

>>  gboolean device_is_trusted(struct btd_device *device);
>>  void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
>>  void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>
>
>
> --
> Luiz Augusto von Dentz

Arman

Re: [PATCH BlueZ 1/5] core: gatt: Implement GattCharacteristic1.StartNotify

[Arman Uguray]

Hi Luiz,

> On Thu, Jan 29, 2015 at 5:37 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Thu, Jan 29, 2015 at 4:10 AM, Arman Uguray <armansito@chromium.org> wrote:
>> This patch implements the StartNotify method of
>> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
>> session to the D-Bus sender which internally registers a unique
>> notify handler with the bt_gatt_client. Each received notification
>> or indication causes a PropertiesChanged signal to be emitted for the
>> "Value" property.
>>
>> The notify handler gets automatically unregistered when sender
>> disconnects from D-Bus.
>>
>> Change-Id: Ia805127613ee538eced4591ba0229789dc54fab8
>> ---
>>  src/gatt-client.c | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 247 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index cb8ddf6..2f187ad 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -88,6 +88,9 @@ struct characteristic {
>>         unsigned int write_id;
>>
>>         struct queue *descs;
>> +
>> +       bool notifying;
>> +       struct queue *notify_clients;
>>  };
>>
>>  struct descriptor {
>> @@ -231,7 +234,9 @@ static void async_dbus_op_free(void *data)
>>  {
>>         struct async_dbus_op *op = data;
>>
>> -       dbus_message_unref(op->msg);
>> +       if (op->msg)
>> +               dbus_message_unref(op->msg);
>> +
>>         free(op);
>>  }
>>
>> @@ -689,12 +694,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
>>  static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
>>                                         DBusMessageIter *iter, void *data)
>>  {
>> -       dbus_bool_t notifying = FALSE;
>> -
>> -       /*
>> -        * TODO: Return the correct value here once StartNotify and StopNotify
>> -        * methods are implemented.
>> -        */
>> +       struct characteristic *chrc = data;
>> +       dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>>
>>         dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
>>
>> @@ -925,11 +926,240 @@ fail:
>>         return btd_error_not_supported(msg);
>>  }
>>
>> +struct notify_client {
>> +       struct characteristic *chrc;
>> +       int ref_count;
>> +       char *owner;
>> +       guint watch;
>> +       unsigned int notify_id;
>> +};
>> +
>> +static void notify_client_free(struct notify_client *client)
>> +{
>> +       DBG("owner %s", client->owner);
>> +
>> +       g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
>> +       free(client->owner);
>> +       free(client);
>> +}
>> +
>> +static void notify_client_unref(void *data)
>> +{
>> +       struct notify_client *client = data;
>> +
>> +       DBG("owner %s", client->owner);
>> +
>> +       if (__sync_sub_and_fetch(&client->ref_count, 1))
>> +               return;
>> +
>> +       notify_client_free(client);
>> +}
>> +
>> +static struct notify_client *notify_client_ref(struct notify_client *client)
>> +{
>> +       DBG("owner %s", client->owner);
>> +
>> +       __sync_fetch_and_add(&client->ref_count, 1);
>> +
>> +       return client;
>> +}
>> +
>> +static bool match_notifying(const void *a, const void *b)
>> +{
>> +       const struct notify_client *client = a;
>> +
>> +       return !!client->notify_id;
>> +}
>> +
>> +static void update_notifying(struct characteristic *chrc)
>> +{
>> +       if (!chrc->notifying)
>> +               return;
>> +
>> +       if (queue_find(chrc->notify_clients, match_notifying, NULL))
>> +               return;
>> +
>> +       chrc->notifying = false;
>> +
>> +       g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
>> +                                               GATT_CHARACTERISTIC_IFACE,
>> +                                               "Notifying");
>> +}
>> +
>> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
>> +{
>> +       struct notify_client *client = user_data;
>> +       struct characteristic *chrc = client->chrc;
>> +       struct bt_gatt_client *gatt = chrc->service->client->gatt;
>> +
>> +       DBG("owner %s", client->owner);
>> +
>> +       queue_remove(chrc->notify_clients, client);
>> +       bt_gatt_client_unregister_notify(gatt, client->notify_id);
>> +
>> +       update_notifying(chrc);
>> +
>> +       notify_client_unref(client);
>> +}
>> +
>> +static struct notify_client *notify_client_create(struct characteristic *chrc,
>> +                                                       const char *owner)
>> +{
>> +       struct notify_client *client;
>> +
>> +       client = new0(struct notify_client, 1);
>> +       if (!client)
>> +               return NULL;
>> +
>> +       client->chrc = chrc;
>> +       client->owner = strdup(owner);
>> +       if (!client->owner) {
>> +               free(client);
>> +               return NULL;
>> +       }
>> +
>> +       client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
>> +                                               owner, notify_client_disconnect,
>> +                                               client, NULL);
>> +       if (!client->watch) {
>> +               free(client->owner);
>> +               free(client);
>> +               return NULL;
>> +       }
>> +
>> +       return notify_client_ref(client);
>> +}
>> +
>> +static bool match_notify_sender(const void *a, const void *b)
>> +{
>> +       const struct notify_client *client = a;
>> +       const char *sender = b;
>> +
>> +       return strcmp(client->owner, sender) == 0;
>> +}
>> +
>> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> +                                       uint16_t length, void *user_data)
>> +{
>> +       struct async_dbus_op *op = user_data;
>> +       struct notify_client *client = op->data;
>> +       struct characteristic *chrc = client->chrc;
>> +
>> +       /*
>> +        * Even if the value didn't change, we want to send a PropertiesChanged
>> +        * signal so that we propagate the notification/indication to
>> +        * applications.
>> +        */
>> +       gatt_db_attribute_reset(chrc->attr);
>> +       gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>> +                                               write_characteristic_cb, chrc);
>> +}
>> +
>> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
>> +                                                               void *user_data)
>> +{
>> +       struct async_dbus_op *op = user_data;
>> +       struct notify_client *client = op->data;
>> +       struct characteristic *chrc = client->chrc;
>> +       DBusMessage *reply;
>> +
>> +       /* Make sure that an interim disconnect did not remove the client */
>> +       if (!queue_find(chrc->notify_clients, NULL, client)) {
>> +               bt_gatt_client_unregister_notify(chrc->service->client->gatt,
>> +                                                                       id);
>> +               notify_client_unref(client);
>> +
>> +               reply = btd_error_failed(op->msg,
>> +                                               "Characteristic not available");
>> +               goto done;
>> +       }
>> +
>> +       /*
>> +        * Drop the reference count that we added when registering the callback.
>> +        */
>> +       notify_client_unref(client);
>> +
>> +       if (!id) {
>> +               queue_remove(chrc->notify_clients, client);
>> +               notify_client_free(client);
>> +
>> +               reply = create_gatt_dbus_error(op->msg, att_ecode);
>> +
>> +               goto done;
>> +       }
>> +
>> +       client->notify_id = id;
>> +
>> +       if (!chrc->notifying) {
>> +               chrc->notifying = true;
>> +               g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> +                                       chrc->path, GATT_CHARACTERISTIC_IFACE,
>> +                                       "Notifying");
>> +       }
>> +
>> +       reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
>> +
>> +done:
>> +       if (reply)
>> +               g_dbus_send_message(btd_get_dbus_connection(), reply);
>> +       else
>> +               error("Failed to construct D-Bus message reply");
>> +
>> +       dbus_message_unref(op->msg);
>> +       op->msg = NULL;
>
> The call to dbus_message_unref can probably be done in notify_client_free.
>

Actually it looks like async_dbus_op_free already calls
dbus_message_unref, so no need to do this clean-up here.

>> +}
>> +
>>  static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>>                                         DBusMessage *msg, void *user_data)
>>  {
>> -       /* TODO: Implement */
>> -       return btd_error_failed(msg, "Not implemented");
>> +       struct characteristic *chrc = user_data;
>> +       struct bt_gatt_client *gatt = chrc->service->client->gatt;
>> +       const char *sender = dbus_message_get_sender(msg);
>> +       struct async_dbus_op *op;
>> +       struct notify_client *client;
>> +
>> +       if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
>> +                               chrc->props & BT_GATT_CHRC_PROP_INDICATE))
>> +               return btd_error_not_supported(msg);
>> +
>> +       /* Each client can only have one active notify session. */
>> +       client = queue_find(chrc->notify_clients, match_notify_sender, sender);
>> +       if (client)
>> +               return client->notify_id ?
>> +                               btd_error_failed(msg, "Already notifying") :
>> +                               btd_error_in_progress(msg);
>> +
>> +       client = notify_client_create(chrc, sender);
>> +       if (!client)
>> +               return btd_error_failed(msg, "Failed allocate notify session");
>> +
>> +       op = new0(struct async_dbus_op, 1);
>> +       if (!op) {
>> +               notify_client_unref(client);
>> +               return btd_error_failed(msg, "Failed to initialize request");
>> +       }
>> +
>> +       /*
>> +        * Add to the ref count so that a disconnect during register won't free
>> +        * the client instance.
>> +        */
>> +       op->data = notify_client_ref(client);
>> +       op->msg = dbus_message_ref(msg);
>> +
>> +       queue_push_tail(chrc->notify_clients, client);
>> +
>> +       if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
>> +                                               register_notify_cb, notify_cb,
>> +                                               op, async_dbus_op_free))
>> +               return NULL;
>
> We better stop this pattern of doing non-cancelable operation, not
> only we cannot cancel any of the async_dbus_op it actually doesn't
> track the caller and we might have to store the op in the
> characteristic because in the event of the later being freed we will
> probably crash on the callback.
>
> Usually the pattern we had used for handling pending operation is to
> attach them to the resource being passed as user_data, not the other
> way around like you doing, take a look at
> android/hog.c:set_and_store_gatt_req it stores the id and then attach
> to the resource so in case anything happens to it the request can be
> cancelled.
>

async_dbus_op actually tracks the caller using op->data. The reason
why this has been safe for ReadValue/WriteValue is because I'm storing
the request ID directly in the caller (characteristic/descriptor).
When, for example, a characteristic gets freed, it cancels the request
by calling bt_gatt_client_cancel (which is really a wrapper for
bt_att_cancel). Since the async_dbus_op is passed as user_data at the
beginning, bt_att_cancel destroys the user_data and never executes the
callback for the request.

For bt_gatt_client_register_value we currently don't have a
cancellation mechanism, so you're right that this can cause a crash if
the characteristic is freed. So making the register operation
cancellable should address this, I'll go ahead and fix that.

>> +       queue_remove(chrc->notify_clients, client);
>> +       async_dbus_op_free(op);
>> +
>> +       /* Directly free the client */
>> +       notify_client_free(client);
>> +
>> +       return btd_error_failed(msg, "Failed to register notify session");
>>  }
>>
>>  static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
>> @@ -1022,6 +1252,13 @@ static struct characteristic *characteristic_create(
>>                 return NULL;
>>         }
>>
>> +       chrc->notify_clients = queue_new();
>> +       if (!chrc->notify_clients) {
>> +               queue_destroy(chrc->descs, NULL);
>> +               free(chrc);
>> +               return NULL;
>> +       }
>> +
>>         chrc->service = service;
>>
>>         gatt_db_attribute_get_char_data(attr, &chrc->handle,
>> @@ -1064,6 +1301,7 @@ static void unregister_characteristic(void *data)
>>         if (chrc->write_id)
>>                 bt_gatt_client_cancel(gatt, chrc->write_id);
>>
>> +       queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>>         queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>>
>>         g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>
>
>
> --
> Luiz Augusto von Dentz