[PATCH v2 00/10] gatt: Add server support for AcquireWrite and AcquireNotify

[PATCH v2 00/10] gatt: Add server support for AcquireWrite and AcquireNotify

[Luiz Augusto von Dentz]

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

This implements similar mechanist to use dedicated file descriptors
for IO bypassing D-Bus.

Note: The current implementation opens just one fd per characteristic,
though it could be possible to have one fd per device.

v2: Allow any type of write procedure to work when WriteAcquired exists.

Luiz Augusto von Dentz (10):
  gatt: Remove useless debug
  client: Rework variables for AcquireWrite/AcquireNotify
  doc/gatt-api: Add server support for AcquireWrite and AcquireNotify
  shared/gatt-server: Add bt_gatt_server_get_mtu
  shared/gatt-db: Add gatt_db_attribute_get_user_data
  gatt: Implement AcquireWrite for server
  client: Implement AcquireWrite for server
  gatt: Implement AcquireNotify for server
  client: Implement AcquireNotify for server
  gatt: Update signature of AcquireWrite and AcquireNotify

 client/gatt.c            | 319 ++++++++++++++++++++++++++++++++++++++++-------
 doc/gatt-api.txt         |  36 ++++--
 mesh/gatt.c              |  23 +++-
 src/gatt-client.c        |   8 +-
 src/gatt-database.c      | 258 +++++++++++++++++++++++++++++++++++++-
 src/shared/gatt-db.c     |   8 ++
 src/shared/gatt-db.h     |   2 +
 src/shared/gatt-server.c |   8 ++
 src/shared/gatt-server.h |   1 +
 9 files changed, 598 insertions(+), 65 deletions(-)

-- 
2.13.5

[PATCH v2 01/10] gatt: Remove useless debug

[Luiz Augusto von Dentz]

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

---
 src/gatt-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 419dadb99..d523b883a 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1229,7 +1229,7 @@ static DBusMessage *characteristic_acquire_write(DBusConnection *conn,
 	chrc->write_io = new0(struct pipe_io, 1);
 
 	if (!bt_gatt_client_is_ready(gatt)) {
-		DBG("GATT not ready, wait until it becomes read");
+		/* GATT not ready, wait until it becomes ready */
 		if (!chrc->ready_id)
 			chrc->ready_id = bt_gatt_client_ready_register(gatt,
 							characteristic_ready,
-- 
2.13.5

[PATCH v2 02/10] client: Rework variables for AcquireWrite/AcquireNotify

[Luiz Augusto von Dentz]

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

This creates a struct with necessary fields which is easier to reset.
---
 client/gatt.c | 80 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 39 insertions(+), 41 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 753038303..ab74aa695 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -90,13 +90,14 @@ static GList *descriptors;
 static GList *managers;
 static GList *uuids;
 
-static GDBusProxy *write_proxy;
-static struct io *write_io;
-static uint16_t write_mtu;
+struct pipe_io {
+	GDBusProxy *proxy;
+	struct io *io;
+	uint16_t mtu;
+};
 
-static GDBusProxy *notify_proxy;
-static struct io *notify_io;
-static uint16_t notify_mtu;
+static struct pipe_io write_io;
+static struct pipe_io notify_io;
 
 static void print_service(struct service *service, const char *description)
 {
@@ -236,18 +237,14 @@ void gatt_add_characteristic(GDBusProxy *proxy)
 
 static void notify_io_destroy(void)
 {
-	io_destroy(notify_io);
-	notify_io = NULL;
-	notify_proxy = NULL;
-	notify_mtu = 0;
+	io_destroy(notify_io.io);
+	memset(&notify_io, 0, sizeof(notify_io));
 }
 
 static void write_io_destroy(void)
 {
-	io_destroy(write_io);
-	write_io = NULL;
-	write_proxy = NULL;
-	write_mtu = 0;
+	io_destroy(write_io.io);
+	memset(&write_io, 0, sizeof(write_io));
 }
 
 void gatt_remove_characteristic(GDBusProxy *proxy)
@@ -262,9 +259,9 @@ void gatt_remove_characteristic(GDBusProxy *proxy)
 
 	print_characteristic(proxy, COLORED_DEL);
 
-	if (write_proxy == proxy)
+	if (write_io.proxy == proxy)
 		write_io_destroy();
-	else if (notify_proxy == proxy)
+	else if (notify_io.proxy == proxy)
 		notify_io_destroy();
 }
 
@@ -650,9 +647,10 @@ static void write_attribute(GDBusProxy *proxy, char *arg)
 	iov.iov_len = i;
 
 	/* Write using the fd if it has been acquired and fit the MTU */
-	if (proxy == write_proxy && (write_io && write_mtu >= i)) {
-		rl_printf("Attempting to write fd %d\n", io_get_fd(write_io));
-		if (io_send(write_io, &iov, 1) < 0) {
+	if (proxy == write_io.proxy && (write_io.io && write_io.mtu >= i)) {
+		rl_printf("Attempting to write fd %d\n",
+						io_get_fd(write_io.io));
+		if (io_send(write_io.io, &iov, 1) < 0) {
 			rl_printf("Failed to write: %s", strerror(errno));
 			return;
 		}
@@ -689,7 +687,7 @@ static bool pipe_read(struct io *io, void *user_data)
 	int fd = io_get_fd(io);
 	ssize_t bytes_read;
 
-	if (io != notify_io)
+	if (io != notify_io.io)
 		return true;
 
 	bytes_read = read(fd, buf, sizeof(buf));
@@ -697,7 +695,7 @@ static bool pipe_read(struct io *io, void *user_data)
 		return false;
 
 	rl_printf("[" COLORED_CHG "] %s Notification:\n",
-			g_dbus_proxy_get_path(notify_proxy));
+			g_dbus_proxy_get_path(notify_io.proxy));
 	rl_hexdump(buf, bytes_read);
 
 	return true;
@@ -705,9 +703,9 @@ static bool pipe_read(struct io *io, void *user_data)
 
 static bool pipe_hup(struct io *io, void *user_data)
 {
-	rl_printf("%s closed\n", io == notify_io ? "Notify" : "Write");
+	rl_printf("%s closed\n", io == notify_io.io ? "Notify" : "Write");
 
-	if (io == notify_io)
+	if (io == notify_io.io)
 		notify_io_destroy();
 	else
 		write_io_destroy();
@@ -740,23 +738,23 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 	if (dbus_set_error_from_message(&error, message) == TRUE) {
 		rl_printf("Failed to acquire write: %s\n", error.name);
 		dbus_error_free(&error);
-		write_proxy = NULL;
+		write_io.proxy = NULL;
 		return;
 	}
 
-	if (write_io)
+	if (write_io.io)
 		write_io_destroy();
 
 	if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
-					DBUS_TYPE_UINT16, &write_mtu,
+					DBUS_TYPE_UINT16, &write_io.mtu,
 					DBUS_TYPE_INVALID) == false)) {
 		rl_printf("Invalid AcquireWrite response\n");
 		return;
 	}
 
-	rl_printf("AcquireWrite success: fd %d MTU %u\n", fd, write_mtu);
+	rl_printf("AcquireWrite success: fd %d MTU %u\n", fd, write_io.mtu);
 
-	write_io = pipe_io_new(fd);
+	write_io.io = pipe_io_new(fd);
 }
 
 void gatt_acquire_write(GDBusProxy *proxy, const char *arg)
@@ -776,12 +774,12 @@ void gatt_acquire_write(GDBusProxy *proxy, const char *arg)
 		return;
 	}
 
-	write_proxy = proxy;
+	write_io.proxy = proxy;
 }
 
 void gatt_release_write(GDBusProxy *proxy, const char *arg)
 {
-	if (proxy != write_proxy || !write_io) {
+	if (proxy != write_io.proxy || !write_io.io) {
 		rl_printf("Write not acquired\n");
 		return;
 	}
@@ -799,27 +797,27 @@ static void acquire_notify_reply(DBusMessage *message, void *user_data)
 	if (dbus_set_error_from_message(&error, message) == TRUE) {
 		rl_printf("Failed to acquire notify: %s\n", error.name);
 		dbus_error_free(&error);
-		write_proxy = NULL;
+		write_io.proxy = NULL;
 		return;
 	}
 
-	if (notify_io) {
-		io_destroy(notify_io);
-		notify_io = NULL;
+	if (notify_io.io) {
+		io_destroy(notify_io.io);
+		notify_io.io = NULL;
 	}
 
-	notify_mtu = 0;
+	notify_io.mtu = 0;
 
 	if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
-					DBUS_TYPE_UINT16, &notify_mtu,
+					DBUS_TYPE_UINT16, &notify_io.mtu,
 					DBUS_TYPE_INVALID) == false)) {
 		rl_printf("Invalid AcquireNotify response\n");
 		return;
 	}
 
-	rl_printf("AcquireNotify success: fd %d MTU %u\n", fd, notify_mtu);
+	rl_printf("AcquireNotify success: fd %d MTU %u\n", fd, notify_io.mtu);
 
-	notify_io = pipe_io_new(fd);
+	notify_io.io = pipe_io_new(fd);
 }
 
 void gatt_acquire_notify(GDBusProxy *proxy, const char *arg)
@@ -839,13 +837,13 @@ void gatt_acquire_notify(GDBusProxy *proxy, const char *arg)
 		return;
 	}
 
-	notify_proxy = proxy;
+	notify_io.proxy = proxy;
 }
 
 void gatt_release_notify(GDBusProxy *proxy, const char *arg)
 {
-	if (proxy != notify_proxy || !notify_io) {
-		rl_printf("Write not acquired\n");
+	if (proxy != notify_io.proxy || !notify_io.io) {
+		rl_printf("Notify not acquired\n");
 		return;
 	}
 
-- 
2.13.5

[PATCH v2 03/10] doc/gatt-api: Add server support for AcquireWrite and AcquireNotify

[Luiz Augusto von Dentz]

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

This enables servers to use the same mechanism to use packet based IO
using file descriptors bypassing D-Bus.

Note that the application is free to choose any type of medium that can
use file descriptors, thus this is not limited to pipe2 although that is
probably recommended due its simplicity.
---
 doc/gatt-api.txt | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index cdd15f301..400470631 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -91,13 +91,16 @@ Methods		array{byte} ReadValue(dict options)
 					 org.bluez.Error.NotAuthorized
 					 org.bluez.Error.NotSupported
 
-		fd, uint16 AcquireWrite() [experimental] (Client only)
+		fd, uint16 AcquireWrite(dict options) [experimental, optional]
 
 			Acquire file descriptor and MTU for writing. Usage of
 			WriteValue will be locked causing it to return
 			NotPermitted error.
 
-			Only works with characteristic that has
+			For server the MTU returned shall be equal or smaller
+			than the negotiated MTU.
+
+			For client it only works with characteristic that has
 			WriteAcquired property which relies on
 			write-without-response Flag.
 
@@ -111,15 +114,21 @@ Methods		array{byte} ReadValue(dict options)
 			that the file descriptor is closed during
 			reconnections as the MTU has to be renegotiated.
 
+			Possible options: "device": Object Device (Server only)
+					  "MTU": Exchanged MTU (Server only)
+
 			Possible Errors: org.bluez.Error.Failed
 					 org.bluez.Error.NotSupported
 
-		fd, uint16 AcquireNotify() [experimental] (Client only)
+		fd, uint16 AcquireNotify(dict options) [experimental, optional]
 
 			Acquire file descriptor and MTU for notify. Usage of
 			StartNotify will be locked causing it to return
 			NotPermitted error.
 
+			For server the MTU returned shall be equal or smaller
+			than the negotiated MTU.
+
 			Only works with characteristic that has NotifyAcquired
 			which relies on notify Flag and no other client have
 			called StartNotify.
@@ -140,6 +149,9 @@ Methods		array{byte} ReadValue(dict options)
 			that the file descriptor is closed during
 			reconnections as the MTU has to be renegotiated.
 
+			Possible options: "device": Object Device (Server only)
+					  "MTU": Exchanged MTU (Server only)
+
 			Possible Errors: org.bluez.Error.Failed
 					 org.bluez.Error.NotSupported
 
@@ -188,14 +200,24 @@ Properties	string UUID [read-only]
 		boolean WriteAcquired [read-only, optional]
 
 			True, if this characteristic has been acquired by any
-			client using AcquireWrite. This properties is ommited
-			in case 'write-without-response' flag is not set.
+			client using AcquireWrite.
+
+			For client properties is ommited in case
+			'write-without-response' flag is not set.
+
+			For server the presence of this property indicates
+			that AcquireWrite is supported.
 
 		boolean NotifyAcquired [read-only, optional]
 
 			True, if this characteristic has been acquired by any
-			client using AcquireNotify. This properties is ommited
-			in case 'notify' flag is not set.
+			client using AcquireNotify.
+
+			For client this properties is ommited in case 'notify'
+			flag is not set.
+
+			For server the presence of this property indicates
+			that AcquireNotify is supported.
 
 		boolean Notifying [read-only, optional]
 
-- 
2.13.5

[PATCH v2 04/10] shared/gatt-server: Add bt_gatt_server_get_mtu

[Luiz Augusto von Dentz]

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

This adds bt_gatt_server_get_mtu which can be used to read the current
MTU.
---
 src/shared/gatt-db.h     | 2 ++
 src/shared/gatt-server.c | 8 ++++++++
 src/shared/gatt-server.h | 1 +
 3 files changed, 11 insertions(+)

diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index f4ec51cff..e2ac645f3 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -265,3 +265,5 @@ bool gatt_db_attribute_write_result(struct gatt_db_attribute *attrib,
 						unsigned int id, int err);
 
 bool gatt_db_attribute_reset(struct gatt_db_attribute *attrib);
+
+void *gatt_db_attribute_get_user_data(struct gatt_db_attribute *attrib);
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index dc3bb8ee9..a986c5295 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1503,6 +1503,14 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
 	return bt_gatt_server_ref(server);
 }
 
+uint16_t bt_gatt_server_get_mtu(struct bt_gatt_server *server)
+{
+	if (!server || !server->att)
+		return 0;
+
+	return bt_att_get_mtu(server->att);
+}
+
 struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server)
 {
 	if (!server)
diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
index 0e480e1b3..74a6c721e 100644
--- a/src/shared/gatt-server.h
+++ b/src/shared/gatt-server.h
@@ -27,6 +27,7 @@ struct bt_gatt_server;
 
 struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
 					struct bt_att *att, uint16_t mtu);
+uint16_t bt_gatt_server_get_mtu(struct bt_gatt_server *server);
 
 struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server);
 void bt_gatt_server_unref(struct bt_gatt_server *server);
-- 
2.13.5

[PATCH v2 05/10] shared/gatt-db: Add gatt_db_attribute_get_user_data

[Luiz Augusto von Dentz]

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

This adds gatt_db_attribute_get_user_data which can be used to retrieve
the user_data given at registration.
---
 src/shared/gatt-db.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index f44cd1a07..2487584f3 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1887,3 +1887,11 @@ bool gatt_db_attribute_reset(struct gatt_db_attribute *attrib)
 
 	return true;
 }
+
+void *gatt_db_attribute_get_user_data(struct gatt_db_attribute *attrib)
+{
+	if (!attrib)
+		return NULL;
+
+	return attrib->user_data;
+}
-- 
2.13.5

[PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Luiz Augusto von Dentz]

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

This enables IO via file descriptors using AcquireWrite if server
implements it.
---
 src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 61eed71d6..239fe5384 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -33,6 +33,7 @@
 #include "gdbus/gdbus.h"
 #include "src/shared/util.h"
 #include "src/shared/queue.h"
+#include "src/shared/io.h"
 #include "src/shared/att.h"
 #include "src/shared/gatt-db.h"
 #include "src/shared/gatt-server.h"
@@ -117,6 +118,7 @@ struct external_chrc {
 	uint8_t props;
 	uint8_t ext_props;
 	uint32_t perm;
+	struct io *write_io;
 	struct gatt_db_attribute *attrib;
 	struct gatt_db_attribute *ccc;
 	struct queue *pending_reads;
@@ -325,6 +327,8 @@ static void chrc_free(void *data)
 {
 	struct external_chrc *chrc = data;
 
+	io_destroy(chrc->write_io);
+
 	queue_destroy(chrc->pending_reads, cancel_pending_read);
 	queue_destroy(chrc->pending_writes, cancel_pending_write);
 
@@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct btd_device *device,
 	return NULL;
 }
 
+static bool pipe_hup(struct io *io, void *user_data)
+{
+	struct external_chrc *chrc = user_data;
+
+	DBG("%p closed\n", io);
+
+	if (io == chrc->write_io)
+		chrc->write_io = NULL;
+
+	io_destroy(io);
+
+	return false;
+}
+
+static struct io *pipe_io_new(int fd, void *user_data)
+{
+	struct io *io;
+
+	io = io_new(fd);
+
+	io_set_close_on_destroy(io, true);
+
+	io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
+
+	return io;
+}
+
+static int pipe_io_send(struct io *io, const void *data, size_t len)
+{
+	struct iovec iov;
+
+	iov.iov_base = (void *) data;
+	iov.iov_len = len;
+
+	return io_send(io, &iov, 1);
+}
+
+static void acquire_write_reply(DBusMessage *message, void *user_data)
+{
+	struct pending_op *op = user_data;
+	struct external_chrc *chrc;
+	DBusError err;
+	int fd;
+	uint16_t mtu;
+
+	chrc = gatt_db_attribute_get_user_data(op->attrib);
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, message) == TRUE) {
+		error("Failed to acquire write: %s\n", err.name);
+		dbus_error_free(&err);
+		goto retry;
+	}
+
+	if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
+					DBUS_TYPE_UINT16, &mtu,
+					DBUS_TYPE_INVALID) == false)) {
+		error("Invalid AcquireWrite response\n");
+		goto retry;
+	}
+
+	DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
+
+	chrc->write_io = pipe_io_new(fd, chrc);
+
+	if (pipe_io_send(chrc->write_io, op->data.iov_base,
+				op->data.iov_len) < 0)
+		goto retry;
+
+	return;
+
+retry:
+	send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
+				op->data.iov_base, op->data.iov_len, 0);
+}
+
+static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
+{
+	struct pending_op *op = user_data;
+	DBusMessageIter dict;
+	struct bt_gatt_server *server;
+	uint16_t mtu;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+					DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+					DBUS_TYPE_STRING_AS_STRING
+					DBUS_TYPE_VARIANT_AS_STRING
+					DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
+					&dict);
+
+	append_options(&dict, op);
+
+	server = btd_device_get_gatt_server(op->device);
+
+	mtu = bt_gatt_server_get_mtu(server);
+
+	dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
+
+	dbus_message_iter_close_container(iter, &dict);
+}
+
+static struct pending_op *acquire_write(struct external_chrc *chrc,
+					struct btd_device *device,
+					struct gatt_db_attribute *attrib,
+					unsigned int id,
+					const uint8_t *value, size_t len)
+{
+	struct pending_op *op;
+
+	op = pending_write_new(device, NULL, attrib, id, value, len, 0);
+
+	if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
+					acquire_write_setup,
+					acquire_write_reply,
+					op, pending_op_free))
+		return op;
+
+	pending_op_free(op);
+
+	return NULL;
+}
+
 static uint8_t ccc_write_cb(uint16_t value, void *user_data)
 {
 	struct external_chrc *chrc = user_data;
@@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
 	struct external_chrc *chrc = user_data;
 	struct btd_device *device;
 	struct queue *queue;
+	DBusMessageIter iter;
 
 	if (chrc->attrib != attrib) {
 		error("Write callback called with incorrect attribute");
@@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
 		goto fail;
 	}
 
+	if (chrc->write_io) {
+		if (pipe_io_send(chrc->write_io, value, len) < 0) {
+			error("Unable to write: %s", strerror(errno));
+			goto fail;
+		}
+
+		gatt_db_attribute_write_result(attrib, id, 0);
+		return;
+	}
+
+	if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
+		if (acquire_write(chrc, device, attrib, id, value, len))
+			return;
+	}
+
 	if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
 		queue = chrc->pending_writes;
 	else
-- 
2.13.5

[PATCH v2 07/10] client: Implement AcquireWrite for server

[Luiz Augusto von Dentz]

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

This enables IO via file descriptors using AcquireWrite if server
implements it.
---
 client/gatt.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 158 insertions(+), 8 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index ab74aa695..6401fbd1a 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -32,6 +32,7 @@
 #include <stdbool.h>
 #include <sys/uio.h>
 #include <wordexp.h>
+#include <fcntl.h>
 
 #include <readline/readline.h>
 #include <readline/history.h>
@@ -73,6 +74,8 @@ struct chrc {
 	GList *descs;
 	int value_len;
 	uint8_t *value;
+	uint16_t mtu;
+	struct io *write_io;
 };
 
 struct service {
@@ -683,19 +686,25 @@ void gatt_write_attribute(GDBusProxy *proxy, const char *arg)
 
 static bool pipe_read(struct io *io, void *user_data)
 {
+	struct chrc *chrc = user_data;
 	uint8_t buf[512];
 	int fd = io_get_fd(io);
 	ssize_t bytes_read;
 
-	if (io != notify_io.io)
+	if (io != notify_io.io && !chrc)
 		return true;
 
 	bytes_read = read(fd, buf, sizeof(buf));
 	if (bytes_read < 0)
 		return false;
 
-	rl_printf("[" COLORED_CHG "] %s Notification:\n",
-			g_dbus_proxy_get_path(notify_io.proxy));
+	if (chrc)
+		rl_printf("[" COLORED_CHG "] Attribute %s written:\n",
+							chrc->path);
+	else
+		rl_printf("[" COLORED_CHG "] %s Notification:\n",
+				g_dbus_proxy_get_path(notify_io.proxy));
+
 	rl_hexdump(buf, bytes_read);
 
 	return true;
@@ -703,6 +712,17 @@ static bool pipe_read(struct io *io, void *user_data)
 
 static bool pipe_hup(struct io *io, void *user_data)
 {
+	struct chrc *chrc = user_data;
+
+	if (chrc) {
+		rl_printf("Attribute %s Write pipe closed\n", chrc->path);
+		if (chrc->write_io) {
+			io_destroy(chrc->write_io);
+			chrc->write_io = NULL;
+		}
+		return false;
+	}
+
 	rl_printf("%s closed\n", io == notify_io.io ? "Notify" : "Write");
 
 	if (io == notify_io.io)
@@ -713,7 +733,7 @@ static bool pipe_hup(struct io *io, void *user_data)
 	return false;
 }
 
-static struct io *pipe_io_new(int fd)
+static struct io *pipe_io_new(int fd, void *user_data)
 {
 	struct io *io;
 
@@ -721,9 +741,9 @@ static struct io *pipe_io_new(int fd)
 
 	io_set_close_on_destroy(io, true);
 
-	io_set_read_handler(io, pipe_read, NULL, NULL);
+	io_set_read_handler(io, pipe_read, user_data, NULL);
 
-	io_set_disconnect_handler(io, pipe_hup, NULL, NULL);
+	io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
 
 	return io;
 }
@@ -754,7 +774,7 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 
 	rl_printf("AcquireWrite success: fd %d MTU %u\n", fd, write_io.mtu);
 
-	write_io.io = pipe_io_new(fd);
+	write_io.io = pipe_io_new(fd, NULL);
 }
 
 void gatt_acquire_write(GDBusProxy *proxy, const char *arg)
@@ -817,7 +837,7 @@ static void acquire_notify_reply(DBusMessage *message, void *user_data)
 
 	rl_printf("AcquireNotify success: fd %d MTU %u\n", fd, notify_io.mtu);
 
-	notify_io.io = pipe_io_new(fd);
+	notify_io.io = pipe_io_new(fd, NULL);
 }
 
 void gatt_acquire_notify(GDBusProxy *proxy, const char *arg)
@@ -1302,12 +1322,41 @@ static gboolean chrc_get_flags(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
+static gboolean chrc_get_write_acquired(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct chrc *chrc = data;
+	dbus_bool_t value;
+
+	value = chrc->write_io ? TRUE : FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value);
+
+	return TRUE;
+}
+
+static gboolean chrc_write_acquired_exists(const GDBusPropertyTable *property,
+								void *data)
+{
+	struct chrc *chrc = data;
+	int i;
+
+	for (i = 0; chrc->flags[i]; i++) {
+		if (!strcmp("write-without-response", chrc->flags[i]))
+			return TRUE;
+	}
+
+	return FALSE;
+}
+
 static const GDBusPropertyTable chrc_properties[] = {
 	{ "UUID", "s", chrc_get_uuid, NULL, NULL },
 	{ "Service", "o", chrc_get_service, NULL, NULL },
 	{ "Value", "ay", chrc_get_value, NULL, NULL },
 	{ "Notifying", "b", chrc_get_notifying, NULL, NULL },
 	{ "Flags", "as", chrc_get_flags, NULL, NULL },
+	{ "WriteAcquired", "b", chrc_get_write_acquired, NULL,
+					chrc_write_acquired_exists },
 	{ }
 };
 
@@ -1369,6 +1418,105 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static int parse_options(DBusMessageIter *iter, struct chrc *chrc)
+{
+	DBusMessageIter dict;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
+		return -EINVAL;
+
+	dbus_message_iter_recurse(iter, &dict);
+
+	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
+		const char *key;
+		DBusMessageIter value, entry;
+		int var;
+
+		dbus_message_iter_recurse(&dict, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		var = dbus_message_iter_get_arg_type(&value);
+		if (strcasecmp(key, "Device") == 0) {
+			if (var != DBUS_TYPE_OBJECT_PATH)
+				return -EINVAL;
+		} else if (strcasecmp(key, "MTU") == 0) {
+			if (var != DBUS_TYPE_UINT16)
+				return -EINVAL;
+			dbus_message_iter_get_basic(&value, &chrc->mtu);
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	return 0;
+}
+
+static DBusMessage *chrc_create_pipe(struct chrc *chrc, DBusMessage *msg)
+{
+	int pipefd[2];
+	struct io *io;
+	bool dir;
+	DBusMessage *reply;
+
+	if (pipe2(pipefd, O_DIRECT | O_NONBLOCK | O_CLOEXEC) < 0)
+		return g_dbus_create_error(msg, "org.bluez.Error.Failed", "%s",
+							strerror(errno));
+
+	dir = dbus_message_has_member(msg, "AcquireWrite");
+
+	io = pipe_io_new(pipefd[!dir], chrc);
+	if (!io) {
+		close(pipefd[0]);
+		close(pipefd[1]);
+		return g_dbus_create_error(msg, "org.bluez.Error.Failed", "%s",
+							strerror(errno));
+	}
+
+	reply = g_dbus_create_reply(msg, DBUS_TYPE_UNIX_FD, &pipefd[dir],
+					DBUS_TYPE_UINT16, &chrc->mtu,
+					DBUS_TYPE_INVALID);
+
+	close(pipefd[dir]);
+
+	chrc->write_io = io;
+
+	rl_printf("[" COLORED_CHG "] Attribute %s Write pipe acquired\n",
+							chrc->path);
+
+	return reply;
+}
+
+static DBusMessage *chrc_acquire_write(DBusConnection *conn, DBusMessage *msg,
+							void *user_data)
+{
+	struct chrc *chrc = user_data;
+	DBusMessageIter iter;
+	DBusMessage *reply;
+
+	dbus_message_iter_init(msg, &iter);
+
+	if (chrc->write_io)
+		return g_dbus_create_error(msg,
+					"org.bluez.Error.NotPermitted",
+					NULL);
+
+	if (parse_options(&iter, chrc))
+		return g_dbus_create_error(msg,
+					"org.bluez.Error.InvalidArguments",
+					NULL);
+
+	reply = chrc_create_pipe(chrc, msg);
+
+	if (chrc->write_io)
+		g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
+							"WriteAcquired");
+
+	return reply;
+}
+
 static DBusMessage *chrc_start_notify(DBusConnection *conn, DBusMessage *msg,
 							void *user_data)
 {
@@ -1420,6 +1568,8 @@ static const GDBusMethodTable chrc_methods[] = {
 	{ GDBUS_ASYNC_METHOD("WriteValue", GDBUS_ARGS({ "value", "ay" },
 						{ "options", "a{sv}" }),
 					NULL, chrc_write_value) },
+	{ GDBUS_METHOD("AcquireWrite", GDBUS_ARGS({ "options", "a{sv}" }),
+					NULL, chrc_acquire_write) },
 	{ GDBUS_ASYNC_METHOD("StartNotify", NULL, NULL, chrc_start_notify) },
 	{ GDBUS_METHOD("StopNotify", NULL, NULL, chrc_stop_notify) },
 	{ GDBUS_METHOD("Confirm", NULL, NULL, chrc_confirm) },
-- 
2.13.5

[PATCH v2 08/10] gatt: Implement AcquireNotify for server

[Luiz Augusto von Dentz]

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

This enables IO via file descriptors using AcquireWrite if server
implements it.
---
 src/gatt-database.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 111 insertions(+), 5 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 239fe5384..4932568dd 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -24,6 +24,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <unistd.h>
 
 #include "lib/bluetooth.h"
 #include "lib/sdp.h"
@@ -118,7 +119,9 @@ struct external_chrc {
 	uint8_t props;
 	uint8_t ext_props;
 	uint32_t perm;
+	uint16_t mtu;
 	struct io *write_io;
+	struct io *notify_io;
 	struct gatt_db_attribute *attrib;
 	struct gatt_db_attribute *ccc;
 	struct queue *pending_reads;
@@ -152,7 +155,8 @@ struct device_state {
 	struct queue *ccc_states;
 };
 
-typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value,
+typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att,
+							uint16_t value,
 							void *user_data);
 typedef void (*btd_gatt_database_destroy_t) (void *data);
 
@@ -328,6 +332,7 @@ static void chrc_free(void *data)
 	struct external_chrc *chrc = data;
 
 	io_destroy(chrc->write_io);
+	io_destroy(chrc->notify_io);
 
 	queue_destroy(chrc->pending_reads, cancel_pending_read);
 	queue_destroy(chrc->pending_writes, cancel_pending_write);
@@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
 		goto done;
 
 	if (ccc_cb->callback)
-		ecode = ccc_cb->callback(get_le16(value), ccc_cb->user_data);
+		ecode = ccc_cb->callback(att, get_le16(value),
+						ccc_cb->user_data);
 
 	if (!ecode) {
 		ccc->value[0] = value[0];
@@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *user_data)
 
 	if (io == chrc->write_io)
 		chrc->write_io = NULL;
+	else
+		chrc->notify_io = NULL;
 
 	io_destroy(io);
 
 	return false;
 }
 
+static bool pipe_io_read(struct io *io, void *user_data)
+{
+	struct external_chrc *chrc = user_data;
+	uint8_t buf[512];
+	int fd = io_get_fd(io);
+	ssize_t bytes_read;
+
+	bytes_read = read(fd, buf, sizeof(buf));
+	if (bytes_read < 0)
+		return false;
+
+	send_notification_to_devices(chrc->service->app->database,
+				gatt_db_attribute_get_handle(chrc->attrib),
+				buf, bytes_read,
+				gatt_db_attribute_get_handle(chrc->ccc),
+				false, NULL);
+
+	return true;
+}
+
 static struct io *pipe_io_new(int fd, void *user_data)
 {
 	struct io *io;
@@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *user_data)
 
 	io_set_close_on_destroy(io, true);
 
+	io_set_read_handler(io, pipe_io_read, user_data, NULL);
+
 	io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
 
 	return io;
@@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct external_chrc *chrc,
 	return NULL;
 }
 
-static uint8_t ccc_write_cb(uint16_t value, void *user_data)
+static void acquire_notify_reply(DBusMessage *message, void *user_data)
 {
 	struct external_chrc *chrc = user_data;
+	DBusError err;
+	int fd;
+	uint16_t mtu;
+
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, message) == TRUE) {
+		error("Failed to acquire notify: %s\n", err.name);
+		dbus_error_free(&err);
+		goto retry;
+	}
+
+	if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
+					DBUS_TYPE_UINT16, &mtu,
+					DBUS_TYPE_INVALID) == false)) {
+		error("Invalid AcquirNotify response\n");
+		goto retry;
+	}
+
+	DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);
+
+	chrc->notify_io = pipe_io_new(fd, chrc);
+
+	__sync_fetch_and_add(&chrc->ntfy_cnt, 1);
+
+	return;
+
+retry:
+	g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NULL,
+							NULL, NULL);
+
+	__sync_fetch_and_add(&chrc->ntfy_cnt, 1);
+}
+
+static void acquire_notify_setup(DBusMessageIter *iter, void *user_data)
+{
+	DBusMessageIter dict;
+	struct external_chrc *chrc = user_data;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+					DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+					DBUS_TYPE_STRING_AS_STRING
+					DBUS_TYPE_VARIANT_AS_STRING
+					DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
+					&dict);
+
+	dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu);
+
+	dbus_message_iter_close_container(iter, &dict);
+}
+
+static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void *user_data)
+{
+	struct external_chrc *chrc = user_data;
+	DBusMessageIter iter;
 
 	DBG("External CCC write received with value: 0x%04x", value);
 
@@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, void *user_data)
 		if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1))
 			return 0;
 
+		if (chrc->notify_io) {
+			io_destroy(chrc->notify_io);
+			chrc->notify_io = NULL;
+			return 0;
+		}
+
 		/*
 		 * Send request to stop notifying. This is best-effort
 		 * operation, so simply ignore the return the value.
@@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, void *user_data)
 		(value == 2 && !(chrc->props & BT_GATT_CHRC_PROP_INDICATE)))
 		return BT_ERROR_CCC_IMPROPERLY_CONFIGURED;
 
+	if (chrc->notify_io) {
+		__sync_fetch_and_add(&chrc->ntfy_cnt, 1);
+		return 0;
+	}
+
+	chrc->mtu = bt_att_get_mtu(att);
+
+	/* Make use of AcquireNotify if supported */
+	if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", &iter)) {
+		if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNotify",
+						acquire_notify_setup,
+						acquire_notify_reply,
+						chrc, NULL))
+			return 0;
+	}
+
 	/*
 	 * Always call StartNotify for an incoming enable and ignore the return
 	 * value for now.
 	 */
-	if (g_dbus_proxy_method_call(chrc->proxy,
-						"StartNotify", NULL, NULL,
+	if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NULL,
 						NULL, NULL) == FALSE)
 		return BT_ATT_ERROR_UNLIKELY;
 
-- 
2.13.5

[PATCH v2 09/10] client: Implement AcquireNotify for server

[Luiz Augusto von Dentz]

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

This enables IO via file descriptors using AcquireNotify if server
implements it.
---
 client/gatt.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 6401fbd1a..1ca1159ad 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -76,6 +76,7 @@ struct chrc {
 	uint8_t *value;
 	uint16_t mtu;
 	struct io *write_io;
+	struct io *notify_io;
 };
 
 struct service {
@@ -1349,6 +1350,33 @@ static gboolean chrc_write_acquired_exists(const GDBusPropertyTable *property,
 	return FALSE;
 }
 
+static gboolean chrc_get_notify_acquired(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct chrc *chrc = data;
+	dbus_bool_t value;
+
+	value = chrc->notify_io ? TRUE : FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &value);
+
+	return TRUE;
+}
+
+static gboolean chrc_notify_acquired_exists(const GDBusPropertyTable *property,
+								void *data)
+{
+	struct chrc *chrc = data;
+	int i;
+
+	for (i = 0; chrc->flags[i]; i++) {
+		if (!strcmp("notify", chrc->flags[i]))
+			return TRUE;
+	}
+
+	return FALSE;
+}
+
 static const GDBusPropertyTable chrc_properties[] = {
 	{ "UUID", "s", chrc_get_uuid, NULL, NULL },
 	{ "Service", "o", chrc_get_service, NULL, NULL },
@@ -1357,6 +1385,8 @@ static const GDBusPropertyTable chrc_properties[] = {
 	{ "Flags", "as", chrc_get_flags, NULL, NULL },
 	{ "WriteAcquired", "b", chrc_get_write_acquired, NULL,
 					chrc_write_acquired_exists },
+	{ "NotifyAcquired", "b", chrc_get_notify_acquired, NULL,
+					chrc_notify_acquired_exists },
 	{ }
 };
 
@@ -1481,10 +1511,13 @@ static DBusMessage *chrc_create_pipe(struct chrc *chrc, DBusMessage *msg)
 
 	close(pipefd[dir]);
 
-	chrc->write_io = io;
+	if (dir)
+		chrc->write_io = io;
+	else
+		chrc->notify_io = io;
 
-	rl_printf("[" COLORED_CHG "] Attribute %s Write pipe acquired\n",
-							chrc->path);
+	rl_printf("[" COLORED_CHG "] Attribute %s %s pipe acquired\n",
+					chrc->path, dir ? "Write" : "Notify");
 
 	return reply;
 }
@@ -1517,6 +1550,34 @@ static DBusMessage *chrc_acquire_write(DBusConnection *conn, DBusMessage *msg,
 	return reply;
 }
 
+static DBusMessage *chrc_acquire_notify(DBusConnection *conn, DBusMessage *msg,
+							void *user_data)
+{
+	struct chrc *chrc = user_data;
+	DBusMessageIter iter;
+	DBusMessage *reply;
+
+	dbus_message_iter_init(msg, &iter);
+
+	if (chrc->notify_io)
+		return g_dbus_create_error(msg,
+					"org.bluez.Error.NotPermitted",
+					NULL);
+
+	if (parse_options(&iter, chrc))
+		return g_dbus_create_error(msg,
+					"org.bluez.Error.InvalidArguments",
+					NULL);
+
+	reply = chrc_create_pipe(chrc, msg);
+
+	if (chrc->notify_io)
+		g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
+							"NotifyAcquired");
+
+	return reply;
+}
+
 static DBusMessage *chrc_start_notify(DBusConnection *conn, DBusMessage *msg,
 							void *user_data)
 {
@@ -1570,6 +1631,8 @@ static const GDBusMethodTable chrc_methods[] = {
 					NULL, chrc_write_value) },
 	{ GDBUS_METHOD("AcquireWrite", GDBUS_ARGS({ "options", "a{sv}" }),
 					NULL, chrc_acquire_write) },
+	{ GDBUS_METHOD("AcquireNotify", GDBUS_ARGS({ "options", "a{sv}" }),
+					NULL, chrc_acquire_notify) },
 	{ GDBUS_ASYNC_METHOD("StartNotify", NULL, NULL, chrc_start_notify) },
 	{ GDBUS_METHOD("StopNotify", NULL, NULL, chrc_stop_notify) },
 	{ GDBUS_METHOD("Confirm", NULL, NULL, chrc_confirm) },
-- 
2.13.5

[PATCH v2 10/10] gatt: Update signature of AcquireWrite and AcquireNotify

[Luiz Augusto von Dentz]

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

It should now contain an argument for the options even though there
are not options defined for clients.
---
 client/gatt.c     | 18 ++++++++++++++++--
 mesh/gatt.c       | 23 ++++++++++++++++++++---
 src/gatt-client.c |  6 ++++--
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 1ca1159ad..93aec92e7 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -778,6 +778,20 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 	write_io.io = pipe_io_new(fd, NULL);
 }
 
+static void acquire_setup(DBusMessageIter *iter, void *user_data)
+{
+	DBusMessageIter dict;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+					DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+					DBUS_TYPE_STRING_AS_STRING
+					DBUS_TYPE_VARIANT_AS_STRING
+					DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
+					&dict);
+
+	dbus_message_iter_close_container(iter, &dict);
+}
+
 void gatt_acquire_write(GDBusProxy *proxy, const char *arg)
 {
 	const char *iface;
@@ -789,7 +803,7 @@ void gatt_acquire_write(GDBusProxy *proxy, const char *arg)
 		return;
 	}
 
-	if (g_dbus_proxy_method_call(proxy, "AcquireWrite", NULL,
+	if (g_dbus_proxy_method_call(proxy, "AcquireWrite", acquire_setup,
 				acquire_write_reply, NULL, NULL) == FALSE) {
 		rl_printf("Failed to AcquireWrite\n");
 		return;
@@ -852,7 +866,7 @@ void gatt_acquire_notify(GDBusProxy *proxy, const char *arg)
 		return;
 	}
 
-	if (g_dbus_proxy_method_call(proxy, "AcquireNotify", NULL,
+	if (g_dbus_proxy_method_call(proxy, "AcquireNotify", acquire_setup,
 				acquire_notify_reply, NULL, NULL) == FALSE) {
 		rl_printf("Failed to AcquireNotify\n");
 		return;
diff --git a/mesh/gatt.c b/mesh/gatt.c
index f9615b3e2..001eb17a8 100644
--- a/mesh/gatt.c
+++ b/mesh/gatt.c
@@ -386,6 +386,20 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 	pipe_write(write_io, data);
 }
 
+static void acquire_setup(DBusMessageIter *iter, void *user_data)
+{
+	DBusMessageIter dict;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+					DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+					DBUS_TYPE_STRING_AS_STRING
+					DBUS_TYPE_VARIANT_AS_STRING
+					DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
+					&dict);
+
+	dbus_message_iter_close_container(iter, &dict);
+}
+
 bool mesh_gatt_write(GDBusProxy *proxy, uint8_t *buf, uint16_t len,
 			GDBusReturnFunction cb, void *user_data)
 {
@@ -424,8 +438,9 @@ bool mesh_gatt_write(GDBusProxy *proxy, uint8_t *buf, uint16_t len,
 		return pipe_write(write_io, data);
 
 	if (g_dbus_proxy_get_property(proxy, "WriteAcquired", &iter)) {
-		if (g_dbus_proxy_method_call(proxy, "AcquireWrite", NULL,
-				acquire_write_reply, data, NULL) == FALSE) {
+		if (g_dbus_proxy_method_call(proxy, "AcquireWrite",
+					acquire_setup, acquire_write_reply,
+					data, NULL) == FALSE) {
 			rl_printf("Failed to AcquireWrite\n");
 			write_data_free(data);
 			return false;
@@ -573,6 +588,7 @@ bool mesh_gatt_notify(GDBusProxy *proxy, bool enable, GDBusReturnFunction cb,
 	struct notify_data *data;
 	DBusMessageIter iter;
 	const char *method;
+	GDBusSetupFunction setup = NULL;
 
 	data = g_new0(struct notify_data, 1);
 	data->proxy = proxy;
@@ -584,6 +600,7 @@ bool mesh_gatt_notify(GDBusProxy *proxy, bool enable, GDBusReturnFunction cb,
 		if (g_dbus_proxy_get_property(proxy, "NotifyAcquired", &iter)) {
 			method = "AcquireNotify";
 			cb = acquire_notify_reply;
+			setup = acquire_setup;
 		} else {
 			method = "StartNotify";
 			cb = notify_reply;
@@ -600,7 +617,7 @@ bool mesh_gatt_notify(GDBusProxy *proxy, bool enable, GDBusReturnFunction cb,
 		}
 	}
 
-	if (g_dbus_proxy_method_call(proxy, method, NULL, cb,
+	if (g_dbus_proxy_method_call(proxy, method, setup, cb,
 					data, NULL) == FALSE) {
 		rl_printf("Failed to %s\n", method);
 		return false;
diff --git a/src/gatt-client.c b/src/gatt-client.c
index d523b883a..32b3a8783 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1624,11 +1624,13 @@ static const GDBusMethodTable characteristic_methods[] = {
 						{ "options", "a{sv}" }),
 					NULL,
 					characteristic_write_value) },
-	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("AcquireWrite", NULL,
+	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("AcquireWrite",
+					GDBUS_ARGS({ "options", "a{sv}" }),
 					GDBUS_ARGS({ "fd", "h" },
 						{ "mtu", "q" }),
 					characteristic_acquire_write) },
-	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("AcquireNotify", NULL,
+	{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("AcquireNotify",
+					GDBUS_ARGS({ "options", "a{sv}" }),
 					GDBUS_ARGS({ "fd", "h" },
 						{ "mtu", "q" }),
 					characteristic_acquire_notify) },
-- 
2.13.5

Re: [PATCH v2 00/10] gatt: Add server support for AcquireWrite and AcquireNotify

[Luiz Augusto von Dentz]

Hi,

On Wed, Sep 20, 2017 at 10:44 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This implements similar mechanist to use dedicated file descriptors
> for IO bypassing D-Bus.
>
> Note: The current implementation opens just one fd per characteristic,
> though it could be possible to have one fd per device.
>
> v2: Allow any type of write procedure to work when WriteAcquired exists.
>
> Luiz Augusto von Dentz (10):
>   gatt: Remove useless debug
>   client: Rework variables for AcquireWrite/AcquireNotify
>   doc/gatt-api: Add server support for AcquireWrite and AcquireNotify
>   shared/gatt-server: Add bt_gatt_server_get_mtu
>   shared/gatt-db: Add gatt_db_attribute_get_user_data
>   gatt: Implement AcquireWrite for server
>   client: Implement AcquireWrite for server
>   gatt: Implement AcquireNotify for server
>   client: Implement AcquireNotify for server
>   gatt: Update signature of AcquireWrite and AcquireNotify
>
>  client/gatt.c            | 319 ++++++++++++++++++++++++++++++++++++++++-------
>  doc/gatt-api.txt         |  36 ++++--
>  mesh/gatt.c              |  23 +++-
>  src/gatt-client.c        |   8 +-
>  src/gatt-database.c      | 258 +++++++++++++++++++++++++++++++++++++-
>  src/shared/gatt-db.c     |   8 ++
>  src/shared/gatt-db.h     |   2 +
>  src/shared/gatt-server.c |   8 ++
>  src/shared/gatt-server.h |   1 +
>  9 files changed, 598 insertions(+), 65 deletions(-)
>
> --
> 2.13.5

Any other feedback on this? Does it address all the use cases you guys
have been struggling?


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 00/10] gatt: Add server support for AcquireWrite and AcquireNotify

[Yunhan Wang]

Hi, Luiz

It is pretty good.

Thanks
Best wishes
Yunhan

On Thu, Sep 21, 2017 at 12:49 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Wed, Sep 20, 2017 at 10:44 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This implements similar mechanist to use dedicated file descriptors
>> for IO bypassing D-Bus.
>>
>> Note: The current implementation opens just one fd per characteristic,
>> though it could be possible to have one fd per device.
>>
>> v2: Allow any type of write procedure to work when WriteAcquired exists.
>>
>> Luiz Augusto von Dentz (10):
>>   gatt: Remove useless debug
>>   client: Rework variables for AcquireWrite/AcquireNotify
>>   doc/gatt-api: Add server support for AcquireWrite and AcquireNotify
>>   shared/gatt-server: Add bt_gatt_server_get_mtu
>>   shared/gatt-db: Add gatt_db_attribute_get_user_data
>>   gatt: Implement AcquireWrite for server
>>   client: Implement AcquireWrite for server
>>   gatt: Implement AcquireNotify for server
>>   client: Implement AcquireNotify for server
>>   gatt: Update signature of AcquireWrite and AcquireNotify
>>
>>  client/gatt.c            | 319 ++++++++++++++++++++++++++++++++++++++++-------
>>  doc/gatt-api.txt         |  36 ++++--
>>  mesh/gatt.c              |  23 +++-
>>  src/gatt-client.c        |   8 +-
>>  src/gatt-database.c      | 258 +++++++++++++++++++++++++++++++++++++-
>>  src/shared/gatt-db.c     |   8 ++
>>  src/shared/gatt-db.h     |   2 +
>>  src/shared/gatt-server.c |   8 ++
>>  src/shared/gatt-server.h |   1 +
>>  9 files changed, 598 insertions(+), 65 deletions(-)
>>
>> --
>> 2.13.5
>
> Any other feedback on this? Does it address all the use cases you guys
> have been struggling?
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 00/10] gatt: Add server support for AcquireWrite and AcquireNotify

[Luiz Augusto von Dentz]

Hi,

On Thu, Sep 21, 2017 at 9:15 PM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> It is pretty good.
>
> Thanks
> Best wishes
> Yunhan
>
> On Thu, Sep 21, 2017 at 12:49 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi,
>>
>> On Wed, Sep 20, 2017 at 10:44 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This implements similar mechanist to use dedicated file descriptors
>>> for IO bypassing D-Bus.
>>>
>>> Note: The current implementation opens just one fd per characteristic,
>>> though it could be possible to have one fd per device.
>>>
>>> v2: Allow any type of write procedure to work when WriteAcquired exists.
>>>
>>> Luiz Augusto von Dentz (10):
>>>   gatt: Remove useless debug
>>>   client: Rework variables for AcquireWrite/AcquireNotify
>>>   doc/gatt-api: Add server support for AcquireWrite and AcquireNotify
>>>   shared/gatt-server: Add bt_gatt_server_get_mtu
>>>   shared/gatt-db: Add gatt_db_attribute_get_user_data
>>>   gatt: Implement AcquireWrite for server
>>>   client: Implement AcquireWrite for server
>>>   gatt: Implement AcquireNotify for server
>>>   client: Implement AcquireNotify for server
>>>   gatt: Update signature of AcquireWrite and AcquireNotify
>>>
>>>  client/gatt.c            | 319 ++++++++++++++++++++++++++++++++++++++++-------
>>>  doc/gatt-api.txt         |  36 ++++--
>>>  mesh/gatt.c              |  23 +++-
>>>  src/gatt-client.c        |   8 +-
>>>  src/gatt-database.c      | 258 +++++++++++++++++++++++++++++++++++++-
>>>  src/shared/gatt-db.c     |   8 ++
>>>  src/shared/gatt-db.h     |   2 +
>>>  src/shared/gatt-server.c |   8 ++
>>>  src/shared/gatt-server.h |   1 +
>>>  9 files changed, 598 insertions(+), 65 deletions(-)
>>>
>>> --
>>> 2.13.5
>>
>> Any other feedback on this? Does it address all the use cases you guys
>> have been struggling?

Applied.

-- 
Luiz Augusto von Dentz

Re: [PATCH v2 08/10] gatt: Implement AcquireNotify for server

[Yunhan Wang]

Hi, Luiz

May I have several questions about this feature?

On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This enables IO via file descriptors using AcquireWrite if server
> implements it.
> ---
>  src/gatt-database.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++=
+++---
>  1 file changed, 111 insertions(+), 5 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 239fe5384..4932568dd 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -24,6 +24,7 @@
>  #include <stdint.h>
>  #include <stdlib.h>
>  #include <errno.h>
> +#include <unistd.h>
>
>  #include "lib/bluetooth.h"
>  #include "lib/sdp.h"
> @@ -118,7 +119,9 @@ struct external_chrc {
>         uint8_t props;
>         uint8_t ext_props;
>         uint32_t perm;
> +       uint16_t mtu;
>         struct io *write_io;
> +       struct io *notify_io;
>         struct gatt_db_attribute *attrib;
>         struct gatt_db_attribute *ccc;
>         struct queue *pending_reads;
> @@ -152,7 +155,8 @@ struct device_state {
>         struct queue *ccc_states;
>  };
>
> -typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value,
> +typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att,
> +                                                       uint16_t value,
>                                                         void *user_data);
>  typedef void (*btd_gatt_database_destroy_t) (void *data);
>
> @@ -328,6 +332,7 @@ static void chrc_free(void *data)
>         struct external_chrc *chrc =3D data;
>
>         io_destroy(chrc->write_io);
> +       io_destroy(chrc->notify_io);
>
>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>         queue_destroy(chrc->pending_writes, cancel_pending_write);
> @@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attribut=
e *attrib,
>                 goto done;
>
>         if (ccc_cb->callback)
> -               ecode =3D ccc_cb->callback(get_le16(value), ccc_cb->user_=
data);
> +               ecode =3D ccc_cb->callback(att, get_le16(value),
> +                                               ccc_cb->user_data);
>
>         if (!ecode) {
>                 ccc->value[0] =3D value[0];
> @@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *user_da=
ta)
>
>         if (io =3D=3D chrc->write_io)
>                 chrc->write_io =3D NULL;
> +       else
> +               chrc->notify_io =3D NULL;
>
>         io_destroy(io);
>
>         return false;
>  }
>
> +static bool pipe_io_read(struct io *io, void *user_data)
> +{
> +       struct external_chrc *chrc =3D user_data;
> +       uint8_t buf[512];
> +       int fd =3D io_get_fd(io);
> +       ssize_t bytes_read;
> +
> +       bytes_read =3D read(fd, buf, sizeof(buf));
> +       if (bytes_read < 0)
> +               return false;
> +
> +       send_notification_to_devices(chrc->service->app->database,
> +                               gatt_db_attribute_get_handle(chrc->attrib=
),
> +                               buf, bytes_read,
> +                               gatt_db_attribute_get_handle(chrc->ccc),
> +                               false, NULL);
> +
> +       return true;
> +}
> +
>  static struct io *pipe_io_new(int fd, void *user_data)
>  {
>         struct io *io;
> @@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *user_da=
ta)
>
>         io_set_close_on_destroy(io, true);
>
> +       io_set_read_handler(io, pipe_io_read, user_data, NULL);
> +
>         io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>
>         return io;
> @@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct ext=
ernal_chrc *chrc,
>         return NULL;
>  }
>
> -static uint8_t ccc_write_cb(uint16_t value, void *user_data)
> +static void acquire_notify_reply(DBusMessage *message, void *user_data)
>  {
>         struct external_chrc *chrc =3D user_data;
> +       DBusError err;
> +       int fd;
> +       uint16_t mtu;
> +
> +       dbus_error_init(&err);
> +
> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
> +               error("Failed to acquire notify: %s\n", err.name);
> +               dbus_error_free(&err);
> +               goto retry;
> +       }
> +
> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
> +                                       DBUS_TYPE_UINT16, &mtu,
> +                                       DBUS_TYPE_INVALID) =3D=3D false))=
 {
> +               error("Invalid AcquirNotify response\n");
> +               goto retry;
> +       }
> +
> +       DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);
> +
> +       chrc->notify_io =3D pipe_io_new(fd, chrc);

Why do we need to use pipe and notify_io when implementing this
feature? I think fd and mtu has been pass using dbus message. Could
you clarify more details?

> +
> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
> +
> +       return;
> +
> +retry:
> +       g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NULL,
> +                                                       NULL, NULL);
> +
> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
> +}
> +
> +static void acquire_notify_setup(DBusMessageIter *iter, void *user_data)
> +{
> +       DBusMessageIter dict;
> +       struct external_chrc *chrc =3D user_data;
> +
> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STR=
ING
> +                                       DBUS_TYPE_STRING_AS_STRING
> +                                       DBUS_TYPE_VARIANT_AS_STRING
> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_STRIN=
G,
> +                                       &dict);
> +
> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu);
> +
> +       dbus_message_iter_close_container(iter, &dict);
> +}
> +
> +static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void *us=
er_data)
> +{
> +       struct external_chrc *chrc =3D user_data;
> +       DBusMessageIter iter;
>
>         DBG("External CCC write received with value: 0x%04x", value);
>
> @@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, void *=
user_data)
>                 if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1))
>                         return 0;
>
> +               if (chrc->notify_io) {
> +                       io_destroy(chrc->notify_io);
> +                       chrc->notify_io =3D NULL;
> +                       return 0;
> +               }
> +
>                 /*
>                  * Send request to stop notifying. This is best-effort
>                  * operation, so simply ignore the return the value.
> @@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, void =
*user_data)
>                 (value =3D=3D 2 && !(chrc->props & BT_GATT_CHRC_PROP_INDI=
CATE)))
>                 return BT_ERROR_CCC_IMPROPERLY_CONFIGURED;
>
> +       if (chrc->notify_io) {
> +               __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
> +               return 0;
> +       }
What is the purpose to call __sync_fetch_and_add when there is
charc->notify_io? What is the scenario here=EF=BC=9F

> +
> +       chrc->mtu =3D bt_att_get_mtu(att);
> +
> +       /* Make use of AcquireNotify if supported */
> +       if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", &ite=
r)) {
> +               if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNotify"=
,
> +                                               acquire_notify_setup,
> +                                               acquire_notify_reply,
> +                                               chrc, NULL))
> +                       return 0;
> +       }
When ble central subscribe this characteristic,  if NotifyAcquired is
set in ble peripheral application, then it return, but how can we
subscribe this characteristic? May we call "AcquireNotify" when ble
central subscribe this characteristic or enable notification?

> +
>         /*
>          * Always call StartNotify for an incoming enable and ignore the =
return
>          * value for now.
>          */
> -       if (g_dbus_proxy_method_call(chrc->proxy,
> -                                               "StartNotify", NULL, NULL=
,
> +       if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NU=
LL,
>                                                 NULL, NULL) =3D=3D FALSE)
>                 return BT_ATT_ERROR_UNLIKELY;
>
> --
> 2.13.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Yunhan Wang]

Hi, Luiz

May I ask several questions about acquire write implementation?

On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This enables IO via file descriptors using AcquireWrite if server
> implements it.
> ---
>  src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 61eed71d6..239fe5384 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -33,6 +33,7 @@
>  #include "gdbus/gdbus.h"
>  #include "src/shared/util.h"
>  #include "src/shared/queue.h"
> +#include "src/shared/io.h"
>  #include "src/shared/att.h"
>  #include "src/shared/gatt-db.h"
>  #include "src/shared/gatt-server.h"
> @@ -117,6 +118,7 @@ struct external_chrc {
>         uint8_t props;
>         uint8_t ext_props;
>         uint32_t perm;
> +       struct io *write_io;
>         struct gatt_db_attribute *attrib;
>         struct gatt_db_attribute *ccc;
>         struct queue *pending_reads;
> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>  {
>         struct external_chrc *chrc = data;
>
> +       io_destroy(chrc->write_io);
> +
>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>
> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct btd_device *device,
>         return NULL;
>  }
>
> +static bool pipe_hup(struct io *io, void *user_data)
> +{
> +       struct external_chrc *chrc = user_data;
> +
> +       DBG("%p closed\n", io);
> +
> +       if (io == chrc->write_io)
> +               chrc->write_io = NULL;
> +
> +       io_destroy(io);
> +
> +       return false;
> +}
> +
> +static struct io *pipe_io_new(int fd, void *user_data)
> +{
> +       struct io *io;
> +
> +       io = io_new(fd);
> +
> +       io_set_close_on_destroy(io, true);
> +
> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
> +
> +       return io;
> +}
> +
> +static int pipe_io_send(struct io *io, const void *data, size_t len)
> +{
> +       struct iovec iov;
> +
> +       iov.iov_base = (void *) data;
> +       iov.iov_len = len;
> +
> +       return io_send(io, &iov, 1);
> +}
> +
> +static void acquire_write_reply(DBusMessage *message, void *user_data)
> +{
> +       struct pending_op *op = user_data;
> +       struct external_chrc *chrc;
> +       DBusError err;
> +       int fd;
> +       uint16_t mtu;
> +
> +       chrc = gatt_db_attribute_get_user_data(op->attrib);
> +       dbus_error_init(&err);
> +
> +       if (dbus_set_error_from_message(&err, message) == TRUE) {
> +               error("Failed to acquire write: %s\n", err.name);
> +               dbus_error_free(&err);
> +               goto retry;
> +       }
> +
> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
> +                                       DBUS_TYPE_UINT16, &mtu,
> +                                       DBUS_TYPE_INVALID) == false)) {
> +               error("Invalid AcquireWrite response\n");
> +               goto retry;
> +       }
> +
> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
> +
> +       chrc->write_io = pipe_io_new(fd, chrc);
> +
> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
> +                               op->data.iov_len) < 0)
> +               goto retry;
> +
> +       return;
> +
> +retry:
> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
> +                               op->data.iov_base, op->data.iov_len, 0);
> +}
> +
> +static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
> +{
> +       struct pending_op *op = user_data;
> +       DBusMessageIter dict;
> +       struct bt_gatt_server *server;
> +       uint16_t mtu;
> +
> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
> +                                       DBUS_TYPE_STRING_AS_STRING
> +                                       DBUS_TYPE_VARIANT_AS_STRING
> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
> +                                       &dict);
> +
> +       append_options(&dict, op);
> +
> +       server = btd_device_get_gatt_server(op->device);
> +
> +       mtu = bt_gatt_server_get_mtu(server);
> +
> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
> +
> +       dbus_message_iter_close_container(iter, &dict);
> +}
> +
> +static struct pending_op *acquire_write(struct external_chrc *chrc,
> +                                       struct btd_device *device,
> +                                       struct gatt_db_attribute *attrib,
> +                                       unsigned int id,
> +                                       const uint8_t *value, size_t len)
> +{
> +       struct pending_op *op;
> +
> +       op = pending_write_new(device, NULL, attrib, id, value, len, 0);
> +
> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
> +                                       acquire_write_setup,
> +                                       acquire_write_reply,
> +                                       op, pending_op_free))
> +               return op;
> +
> +       pending_op_free(op);
> +
> +       return NULL;
> +}
> +
>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>  {
>         struct external_chrc *chrc = user_data;
> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
>         struct external_chrc *chrc = user_data;
>         struct btd_device *device;
>         struct queue *queue;
> +       DBusMessageIter iter;
>
>         if (chrc->attrib != attrib) {
>                 error("Write callback called with incorrect attribute");
> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
>                 goto fail;
>         }
>
> +       if (chrc->write_io) {
> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
> +                       error("Unable to write: %s", strerror(errno));
> +                       goto fail;
> +               }
> +
> +               gatt_db_attribute_write_result(attrib, id, 0);
> +               return;
> +       }
> +
Could you elaborate more about this part of codes, why are you using
pipe_io_send to update value instead of dbus?

> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
> +               if (acquire_write(chrc, device, attrib, id, value, len))
> +                       return;
> +       }
> +

Currently my expected scenario is to get MTU when first Gatt Write(
write with response) comes in specific characteristic, for example, I
use iphone as ble central and write value in Characteristic A on Bluez
peripheral, Bluez peripheral get negotiated MTU when first write comes
in. From this part of codes, it seems it return immediately after
acquire_write is called, how can we move forward and write value to
peripheral application? What is the design idea here?

The below is earlier draft idea for this MTU acquisition in server side.
https://www.spinics.net/lists/linux-bluetooth/msg71961.html

>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>                 queue = chrc->pending_writes;
>         else
> --
> 2.13.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Luiz Augusto von Dentz]

Hi Yunhan,

On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> May I ask several questions about acquire write implementation?
>
> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This enables IO via file descriptors using AcquireWrite if server
>> implements it.
>> ---
>>  src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 142 insertions(+)
>>
>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>> index 61eed71d6..239fe5384 100644
>> --- a/src/gatt-database.c
>> +++ b/src/gatt-database.c
>> @@ -33,6 +33,7 @@
>>  #include "gdbus/gdbus.h"
>>  #include "src/shared/util.h"
>>  #include "src/shared/queue.h"
>> +#include "src/shared/io.h"
>>  #include "src/shared/att.h"
>>  #include "src/shared/gatt-db.h"
>>  #include "src/shared/gatt-server.h"
>> @@ -117,6 +118,7 @@ struct external_chrc {
>>         uint8_t props;
>>         uint8_t ext_props;
>>         uint32_t perm;
>> +       struct io *write_io;
>>         struct gatt_db_attribute *attrib;
>>         struct gatt_db_attribute *ccc;
>>         struct queue *pending_reads;
>> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>>  {
>>         struct external_chrc *chrc = data;
>>
>> +       io_destroy(chrc->write_io);
>> +
>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>
>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct btd_device *device,
>>         return NULL;
>>  }
>>
>> +static bool pipe_hup(struct io *io, void *user_data)
>> +{
>> +       struct external_chrc *chrc = user_data;
>> +
>> +       DBG("%p closed\n", io);
>> +
>> +       if (io == chrc->write_io)
>> +               chrc->write_io = NULL;
>> +
>> +       io_destroy(io);
>> +
>> +       return false;
>> +}
>> +
>> +static struct io *pipe_io_new(int fd, void *user_data)
>> +{
>> +       struct io *io;
>> +
>> +       io = io_new(fd);
>> +
>> +       io_set_close_on_destroy(io, true);
>> +
>> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>> +
>> +       return io;
>> +}
>> +
>> +static int pipe_io_send(struct io *io, const void *data, size_t len)
>> +{
>> +       struct iovec iov;
>> +
>> +       iov.iov_base = (void *) data;
>> +       iov.iov_len = len;
>> +
>> +       return io_send(io, &iov, 1);
>> +}
>> +
>> +static void acquire_write_reply(DBusMessage *message, void *user_data)
>> +{
>> +       struct pending_op *op = user_data;
>> +       struct external_chrc *chrc;
>> +       DBusError err;
>> +       int fd;
>> +       uint16_t mtu;
>> +
>> +       chrc = gatt_db_attribute_get_user_data(op->attrib);
>> +       dbus_error_init(&err);
>> +
>> +       if (dbus_set_error_from_message(&err, message) == TRUE) {
>> +               error("Failed to acquire write: %s\n", err.name);
>> +               dbus_error_free(&err);
>> +               goto retry;
>> +       }
>> +
>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd,
>> +                                       DBUS_TYPE_UINT16, &mtu,
>> +                                       DBUS_TYPE_INVALID) == false)) {
>> +               error("Invalid AcquireWrite response\n");
>> +               goto retry;
>> +       }
>> +
>> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
>> +
>> +       chrc->write_io = pipe_io_new(fd, chrc);
>> +
>> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
>> +                               op->data.iov_len) < 0)
>> +               goto retry;
>> +
>> +       return;
>> +
>> +retry:
>> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
>> +                               op->data.iov_base, op->data.iov_len, 0);
>> +}
>> +
>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
>> +{
>> +       struct pending_op *op = user_data;
>> +       DBusMessageIter dict;
>> +       struct bt_gatt_server *server;
>> +       uint16_t mtu;
>> +
>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
>> +                                       DBUS_TYPE_STRING_AS_STRING
>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
>> +                                       &dict);
>> +
>> +       append_options(&dict, op);
>> +
>> +       server = btd_device_get_gatt_server(op->device);
>> +
>> +       mtu = bt_gatt_server_get_mtu(server);
>> +
>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
>> +
>> +       dbus_message_iter_close_container(iter, &dict);
>> +}
>> +
>> +static struct pending_op *acquire_write(struct external_chrc *chrc,
>> +                                       struct btd_device *device,
>> +                                       struct gatt_db_attribute *attrib,
>> +                                       unsigned int id,
>> +                                       const uint8_t *value, size_t len)
>> +{
>> +       struct pending_op *op;
>> +
>> +       op = pending_write_new(device, NULL, attrib, id, value, len, 0);
>> +
>> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>> +                                       acquire_write_setup,
>> +                                       acquire_write_reply,
>> +                                       op, pending_op_free))
>> +               return op;
>> +
>> +       pending_op_free(op);
>> +
>> +       return NULL;
>> +}
>> +
>>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>  {
>>         struct external_chrc *chrc = user_data;
>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
>>         struct external_chrc *chrc = user_data;
>>         struct btd_device *device;
>>         struct queue *queue;
>> +       DBusMessageIter iter;
>>
>>         if (chrc->attrib != attrib) {
>>                 error("Write callback called with incorrect attribute");
>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
>>                 goto fail;
>>         }
>>
>> +       if (chrc->write_io) {
>> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
>> +                       error("Unable to write: %s", strerror(errno));
>> +                       goto fail;
>> +               }
>> +
>> +               gatt_db_attribute_write_result(attrib, id, 0);
>> +               return;
>> +       }
>> +
> Could you elaborate more about this part of codes, why are you using
> pipe_io_send to update value instead of dbus?

This is doing the write via fd if write_io has been acquired, the
whole point is avoiding D-Bus.

>> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
>> +               if (acquire_write(chrc, device, attrib, id, value, len))
>> +                       return;
>> +       }
>> +
>
> Currently my expected scenario is to get MTU when first Gatt Write(
> write with response) comes in specific characteristic, for example, I
> use iphone as ble central and write value in Characteristic A on Bluez
> peripheral, Bluez peripheral get negotiated MTU when first write comes
> in. From this part of codes, it seems it return immediately after
> acquire_write is called, how can we move forward and write value to
> peripheral application? What is the design idea here?

After AcquireWrite the data is sent via fd as well:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database.c#n1891

> The below is earlier draft idea for this MTU acquisition in server side.
> https://www.spinics.net/lists/linux-bluetooth/msg71961.html
>
>>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>>                 queue = chrc->pending_writes;
>>         else
>> --
>> 2.13.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 08/10] gatt: Implement AcquireNotify for server

[Luiz Augusto von Dentz]

Hi Yunhan,

On Fri, Sep 29, 2017 at 7:47 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> May I have several questions about this feature?
>
> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This enables IO via file descriptors using AcquireWrite if server
>> implements it.
>> ---
>>  src/gatt-database.c | 116 +++++++++++++++++++++++++++++++++++++++++++++=
++++---
>>  1 file changed, 111 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>> index 239fe5384..4932568dd 100644
>> --- a/src/gatt-database.c
>> +++ b/src/gatt-database.c
>> @@ -24,6 +24,7 @@
>>  #include <stdint.h>
>>  #include <stdlib.h>
>>  #include <errno.h>
>> +#include <unistd.h>
>>
>>  #include "lib/bluetooth.h"
>>  #include "lib/sdp.h"
>> @@ -118,7 +119,9 @@ struct external_chrc {
>>         uint8_t props;
>>         uint8_t ext_props;
>>         uint32_t perm;
>> +       uint16_t mtu;
>>         struct io *write_io;
>> +       struct io *notify_io;
>>         struct gatt_db_attribute *attrib;
>>         struct gatt_db_attribute *ccc;
>>         struct queue *pending_reads;
>> @@ -152,7 +155,8 @@ struct device_state {
>>         struct queue *ccc_states;
>>  };
>>
>> -typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value,
>> +typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att,
>> +                                                       uint16_t value,
>>                                                         void *user_data)=
;
>>  typedef void (*btd_gatt_database_destroy_t) (void *data);
>>
>> @@ -328,6 +332,7 @@ static void chrc_free(void *data)
>>         struct external_chrc *chrc =3D data;
>>
>>         io_destroy(chrc->write_io);
>> +       io_destroy(chrc->notify_io);
>>
>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>> @@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attribu=
te *attrib,
>>                 goto done;
>>
>>         if (ccc_cb->callback)
>> -               ecode =3D ccc_cb->callback(get_le16(value), ccc_cb->user=
_data);
>> +               ecode =3D ccc_cb->callback(att, get_le16(value),
>> +                                               ccc_cb->user_data);
>>
>>         if (!ecode) {
>>                 ccc->value[0] =3D value[0];
>> @@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *user_d=
ata)
>>
>>         if (io =3D=3D chrc->write_io)
>>                 chrc->write_io =3D NULL;
>> +       else
>> +               chrc->notify_io =3D NULL;
>>
>>         io_destroy(io);
>>
>>         return false;
>>  }
>>
>> +static bool pipe_io_read(struct io *io, void *user_data)
>> +{
>> +       struct external_chrc *chrc =3D user_data;
>> +       uint8_t buf[512];
>> +       int fd =3D io_get_fd(io);
>> +       ssize_t bytes_read;
>> +
>> +       bytes_read =3D read(fd, buf, sizeof(buf));
>> +       if (bytes_read < 0)
>> +               return false;
>> +
>> +       send_notification_to_devices(chrc->service->app->database,
>> +                               gatt_db_attribute_get_handle(chrc->attri=
b),
>> +                               buf, bytes_read,
>> +                               gatt_db_attribute_get_handle(chrc->ccc),
>> +                               false, NULL);
>> +
>> +       return true;
>> +}
>> +
>>  static struct io *pipe_io_new(int fd, void *user_data)
>>  {
>>         struct io *io;
>> @@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *user_d=
ata)
>>
>>         io_set_close_on_destroy(io, true);
>>
>> +       io_set_read_handler(io, pipe_io_read, user_data, NULL);
>> +
>>         io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>
>>         return io;
>> @@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct ex=
ternal_chrc *chrc,
>>         return NULL;
>>  }
>>
>> -static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>> +static void acquire_notify_reply(DBusMessage *message, void *user_data)
>>  {
>>         struct external_chrc *chrc =3D user_data;
>> +       DBusError err;
>> +       int fd;
>> +       uint16_t mtu;
>> +
>> +       dbus_error_init(&err);
>> +
>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>> +               error("Failed to acquire notify: %s\n", err.name);
>> +               dbus_error_free(&err);
>> +               goto retry;
>> +       }
>> +
>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &fd=
,
>> +                                       DBUS_TYPE_UINT16, &mtu,
>> +                                       DBUS_TYPE_INVALID) =3D=3D false)=
) {
>> +               error("Invalid AcquirNotify response\n");
>> +               goto retry;
>> +       }
>> +
>> +       DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);
>> +
>> +       chrc->notify_io =3D pipe_io_new(fd, chrc);
>
> Why do we need to use pipe and notify_io when implementing this
> feature? I think fd and mtu has been pass using dbus message. Could
> you clarify more details?

notify_io is the receiving end of the notification, the application
will write its notifications using the fd which is captured by
checking if the fd is readable.

>
>> +
>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>> +
>> +       return;
>> +
>> +retry:
>> +       g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NULL,
>> +                                                       NULL, NULL);
>> +
>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>> +}
>> +
>> +static void acquire_notify_setup(DBusMessageIter *iter, void *user_data=
)
>> +{
>> +       DBusMessageIter dict;
>> +       struct external_chrc *chrc =3D user_data;
>> +
>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_ST=
RING
>> +                                       DBUS_TYPE_STRING_AS_STRING
>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_STRI=
NG,
>> +                                       &dict);
>> +
>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu);
>> +
>> +       dbus_message_iter_close_container(iter, &dict);
>> +}
>> +
>> +static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void *u=
ser_data)
>> +{
>> +       struct external_chrc *chrc =3D user_data;
>> +       DBusMessageIter iter;
>>
>>         DBG("External CCC write received with value: 0x%04x", value);
>>
>> @@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, void =
*user_data)
>>                 if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1))
>>                         return 0;
>>
>> +               if (chrc->notify_io) {
>> +                       io_destroy(chrc->notify_io);
>> +                       chrc->notify_io =3D NULL;
>> +                       return 0;
>> +               }
>> +
>>                 /*
>>                  * Send request to stop notifying. This is best-effort
>>                  * operation, so simply ignore the return the value.
>> @@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, void=
 *user_data)
>>                 (value =3D=3D 2 && !(chrc->props & BT_GATT_CHRC_PROP_IND=
ICATE)))
>>                 return BT_ERROR_CCC_IMPROPERLY_CONFIGURED;
>>
>> +       if (chrc->notify_io) {
>> +               __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>> +               return 0;
>> +       }
> What is the purpose to call __sync_fetch_and_add when there is
> charc->notify_io? What is the scenario here=EF=BC=9F

This is using the number of subscribed client as a refcount so when
there is no client left we can drop the notify_io as well.

>
>> +
>> +       chrc->mtu =3D bt_att_get_mtu(att);
>> +
>> +       /* Make use of AcquireNotify if supported */
>> +       if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", &it=
er)) {
>> +               if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNotify=
",
>> +                                               acquire_notify_setup,
>> +                                               acquire_notify_reply,
>> +                                               chrc, NULL))
>> +                       return 0;
>> +       }
> When ble central subscribe this characteristic,  if NotifyAcquired is
> set in ble peripheral application, then it return, but how can we
> subscribe this characteristic? May we call "AcquireNotify" when ble
> central subscribe this characteristic or enable notification?

If NotifyAcquired is supported it will call AcquireNotify, or are you
saying you tried that? The bluetoothctl has code to utilize this
feature, you just have to register a characteristic with notify flag.

>> +
>>         /*
>>          * Always call StartNotify for an incoming enable and ignore the=
 return
>>          * value for now.
>>          */
>> -       if (g_dbus_proxy_method_call(chrc->proxy,
>> -                                               "StartNotify", NULL, NUL=
L,
>> +       if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, N=
ULL,
>>                                                 NULL, NULL) =3D=3D FALSE=
)
>>                 return BT_ATT_ERROR_UNLIKELY;
>>
>> --
>> 2.13.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoot=
h" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--=20
Luiz Augusto von Dentz

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Yunhan Wang]

Hi, Luiz

For server, what is the benefit to use fd instead of DBUS to pass
write value? Isn't it better that we put all bluez client and server
only chat via DBUS upon bluetoothd? For mtu, definitely we can do it
using DBUS message, if we use current acquire_write, in server
application side, we will have to implement callback for
"AcquireWrite" instead of "WriteValue=E2=80=9C=E3=80=81

Thanks
Best wishes
Yunhan

On Thu, Sep 28, 2017 at 11:54 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunhan,
>
> On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote=
:
>> Hi, Luiz
>>
>> May I ask several questions about acquire write implementation?
>>
>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This enables IO via file descriptors using AcquireWrite if server
>>> implements it.
>>> ---
>>>  src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++++++=
++++++++
>>>  1 file changed, 142 insertions(+)
>>>
>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>> index 61eed71d6..239fe5384 100644
>>> --- a/src/gatt-database.c
>>> +++ b/src/gatt-database.c
>>> @@ -33,6 +33,7 @@
>>>  #include "gdbus/gdbus.h"
>>>  #include "src/shared/util.h"
>>>  #include "src/shared/queue.h"
>>> +#include "src/shared/io.h"
>>>  #include "src/shared/att.h"
>>>  #include "src/shared/gatt-db.h"
>>>  #include "src/shared/gatt-server.h"
>>> @@ -117,6 +118,7 @@ struct external_chrc {
>>>         uint8_t props;
>>>         uint8_t ext_props;
>>>         uint32_t perm;
>>> +       struct io *write_io;
>>>         struct gatt_db_attribute *attrib;
>>>         struct gatt_db_attribute *ccc;
>>>         struct queue *pending_reads;
>>> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>>>  {
>>>         struct external_chrc *chrc =3D data;
>>>
>>> +       io_destroy(chrc->write_io);
>>> +
>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>
>>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct btd=
_device *device,
>>>         return NULL;
>>>  }
>>>
>>> +static bool pipe_hup(struct io *io, void *user_data)
>>> +{
>>> +       struct external_chrc *chrc =3D user_data;
>>> +
>>> +       DBG("%p closed\n", io);
>>> +
>>> +       if (io =3D=3D chrc->write_io)
>>> +               chrc->write_io =3D NULL;
>>> +
>>> +       io_destroy(io);
>>> +
>>> +       return false;
>>> +}
>>> +
>>> +static struct io *pipe_io_new(int fd, void *user_data)
>>> +{
>>> +       struct io *io;
>>> +
>>> +       io =3D io_new(fd);
>>> +
>>> +       io_set_close_on_destroy(io, true);
>>> +
>>> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>> +
>>> +       return io;
>>> +}
>>> +
>>> +static int pipe_io_send(struct io *io, const void *data, size_t len)
>>> +{
>>> +       struct iovec iov;
>>> +
>>> +       iov.iov_base =3D (void *) data;
>>> +       iov.iov_len =3D len;
>>> +
>>> +       return io_send(io, &iov, 1);
>>> +}
>>> +
>>> +static void acquire_write_reply(DBusMessage *message, void *user_data)
>>> +{
>>> +       struct pending_op *op =3D user_data;
>>> +       struct external_chrc *chrc;
>>> +       DBusError err;
>>> +       int fd;
>>> +       uint16_t mtu;
>>> +
>>> +       chrc =3D gatt_db_attribute_get_user_data(op->attrib);
>>> +       dbus_error_init(&err);
>>> +
>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>>> +               error("Failed to acquire write: %s\n", err.name);
>>> +               dbus_error_free(&err);
>>> +               goto retry;
>>> +       }
>>> +
>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &f=
d,
>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>> +                                       DBUS_TYPE_INVALID) =3D=3D false=
)) {
>>> +               error("Invalid AcquireWrite response\n");
>>> +               goto retry;
>>> +       }
>>> +
>>> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
>>> +
>>> +       chrc->write_io =3D pipe_io_new(fd, chrc);
>>> +
>>> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
>>> +                               op->data.iov_len) < 0)
>>> +               goto retry;
>>> +
>>> +       return;
>>> +
>>> +retry:
>>> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
>>> +                               op->data.iov_base, op->data.iov_len, 0)=
;
>>> +}
>>> +
>>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_data=
)
>>> +{
>>> +       struct pending_op *op =3D user_data;
>>> +       DBusMessageIter dict;
>>> +       struct bt_gatt_server *server;
>>> +       uint16_t mtu;
>>> +
>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_S=
TRING
>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_STR=
ING,
>>> +                                       &dict);
>>> +
>>> +       append_options(&dict, op);
>>> +
>>> +       server =3D btd_device_get_gatt_server(op->device);
>>> +
>>> +       mtu =3D bt_gatt_server_get_mtu(server);
>>> +
>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
>>> +
>>> +       dbus_message_iter_close_container(iter, &dict);
>>> +}
>>> +
>>> +static struct pending_op *acquire_write(struct external_chrc *chrc,
>>> +                                       struct btd_device *device,
>>> +                                       struct gatt_db_attribute *attri=
b,
>>> +                                       unsigned int id,
>>> +                                       const uint8_t *value, size_t le=
n)
>>> +{
>>> +       struct pending_op *op;
>>> +
>>> +       op =3D pending_write_new(device, NULL, attrib, id, value, len, =
0);
>>> +
>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>>> +                                       acquire_write_setup,
>>> +                                       acquire_write_reply,
>>> +                                       op, pending_op_free))
>>> +               return op;
>>> +
>>> +       pending_op_free(op);
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>  {
>>>         struct external_chrc *chrc =3D user_data;
>>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attribut=
e *attrib,
>>>         struct external_chrc *chrc =3D user_data;
>>>         struct btd_device *device;
>>>         struct queue *queue;
>>> +       DBusMessageIter iter;
>>>
>>>         if (chrc->attrib !=3D attrib) {
>>>                 error("Write callback called with incorrect attribute")=
;
>>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attribu=
te *attrib,
>>>                 goto fail;
>>>         }
>>>
>>> +       if (chrc->write_io) {
>>> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
>>> +                       error("Unable to write: %s", strerror(errno));
>>> +                       goto fail;
>>> +               }
>>> +
>>> +               gatt_db_attribute_write_result(attrib, id, 0);
>>> +               return;
>>> +       }
>>> +
>> Could you elaborate more about this part of codes, why are you using
>> pipe_io_send to update value instead of dbus?
>
> This is doing the write via fd if write_io has been acquired, the
> whole point is avoiding D-Bus.
>
>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &it=
er)) {
>>> +               if (acquire_write(chrc, device, attrib, id, value, len)=
)
>>> +                       return;
>>> +       }
>>> +
>>
>> Currently my expected scenario is to get MTU when first Gatt Write(
>> write with response) comes in specific characteristic, for example, I
>> use iphone as ble central and write value in Characteristic A on Bluez
>> peripheral, Bluez peripheral get negotiated MTU when first write comes
>> in. From this part of codes, it seems it return immediately after
>> acquire_write is called, how can we move forward and write value to
>> peripheral application? What is the design idea here?
>
> After AcquireWrite the data is sent via fd as well:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database=
.c#n1891
>
>> The below is earlier draft idea for this MTU acquisition in server side.
>> https://www.spinics.net/lists/linux-bluetooth/msg71961.html
>>
>>>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>>>                 queue =3D chrc->pending_writes;
>>>         else
>>> --
>>> 2.13.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoo=
th" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Luiz Augusto von Dentz]

Hi Yunhan,

On Fri, Sep 29, 2017 at 10:14 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> For server, what is the benefit to use fd instead of DBUS to pass
> write value? Isn't it better that we put all bluez client and server
> only chat via DBUS upon bluetoothd? For mtu, definitely we can do it
> using DBUS message, if we use current acquire_write, in server
> application side, we will have to implement callback for
> "AcquireWrite" instead of "WriteValue=E2=80=9C=E3=80=81

For byte streams passing data over D-Bus is inefficient as that has is
delivered to dbus-daemon before the end application it will always be
slower than sending directly via fd, actually depending on the load on
the system this may impact other processes using D-Bus as the daemon
will be busy processing the WriteValue messages.

> Thanks
> Best wishes
> Yunhan
>
> On Thu, Sep 28, 2017 at 11:54 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Yunhan,
>>
>> On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@nestlabs.com> wrot=
e:
>>> Hi, Luiz
>>>
>>> May I ask several questions about acquire write implementation?
>>>
>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>
>>>> This enables IO via file descriptors using AcquireWrite if server
>>>> implements it.
>>>> ---
>>>>  src/gatt-database.c | 142 +++++++++++++++++++++++++++++++++++++++++++=
+++++++++
>>>>  1 file changed, 142 insertions(+)
>>>>
>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>>> index 61eed71d6..239fe5384 100644
>>>> --- a/src/gatt-database.c
>>>> +++ b/src/gatt-database.c
>>>> @@ -33,6 +33,7 @@
>>>>  #include "gdbus/gdbus.h"
>>>>  #include "src/shared/util.h"
>>>>  #include "src/shared/queue.h"
>>>> +#include "src/shared/io.h"
>>>>  #include "src/shared/att.h"
>>>>  #include "src/shared/gatt-db.h"
>>>>  #include "src/shared/gatt-server.h"
>>>> @@ -117,6 +118,7 @@ struct external_chrc {
>>>>         uint8_t props;
>>>>         uint8_t ext_props;
>>>>         uint32_t perm;
>>>> +       struct io *write_io;
>>>>         struct gatt_db_attribute *attrib;
>>>>         struct gatt_db_attribute *ccc;
>>>>         struct queue *pending_reads;
>>>> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>>>>  {
>>>>         struct external_chrc *chrc =3D data;
>>>>
>>>> +       io_destroy(chrc->write_io);
>>>> +
>>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>>
>>>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct bt=
d_device *device,
>>>>         return NULL;
>>>>  }
>>>>
>>>> +static bool pipe_hup(struct io *io, void *user_data)
>>>> +{
>>>> +       struct external_chrc *chrc =3D user_data;
>>>> +
>>>> +       DBG("%p closed\n", io);
>>>> +
>>>> +       if (io =3D=3D chrc->write_io)
>>>> +               chrc->write_io =3D NULL;
>>>> +
>>>> +       io_destroy(io);
>>>> +
>>>> +       return false;
>>>> +}
>>>> +
>>>> +static struct io *pipe_io_new(int fd, void *user_data)
>>>> +{
>>>> +       struct io *io;
>>>> +
>>>> +       io =3D io_new(fd);
>>>> +
>>>> +       io_set_close_on_destroy(io, true);
>>>> +
>>>> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>> +
>>>> +       return io;
>>>> +}
>>>> +
>>>> +static int pipe_io_send(struct io *io, const void *data, size_t len)
>>>> +{
>>>> +       struct iovec iov;
>>>> +
>>>> +       iov.iov_base =3D (void *) data;
>>>> +       iov.iov_len =3D len;
>>>> +
>>>> +       return io_send(io, &iov, 1);
>>>> +}
>>>> +
>>>> +static void acquire_write_reply(DBusMessage *message, void *user_data=
)
>>>> +{
>>>> +       struct pending_op *op =3D user_data;
>>>> +       struct external_chrc *chrc;
>>>> +       DBusError err;
>>>> +       int fd;
>>>> +       uint16_t mtu;
>>>> +
>>>> +       chrc =3D gatt_db_attribute_get_user_data(op->attrib);
>>>> +       dbus_error_init(&err);
>>>> +
>>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>>>> +               error("Failed to acquire write: %s\n", err.name);
>>>> +               dbus_error_free(&err);
>>>> +               goto retry;
>>>> +       }
>>>> +
>>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &=
fd,
>>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>>> +                                       DBUS_TYPE_INVALID) =3D=3D fals=
e)) {
>>>> +               error("Invalid AcquireWrite response\n");
>>>> +               goto retry;
>>>> +       }
>>>> +
>>>> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
>>>> +
>>>> +       chrc->write_io =3D pipe_io_new(fd, chrc);
>>>> +
>>>> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
>>>> +                               op->data.iov_len) < 0)
>>>> +               goto retry;
>>>> +
>>>> +       return;
>>>> +
>>>> +retry:
>>>> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
>>>> +                               op->data.iov_base, op->data.iov_len, 0=
);
>>>> +}
>>>> +
>>>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_dat=
a)
>>>> +{
>>>> +       struct pending_op *op =3D user_data;
>>>> +       DBusMessageIter dict;
>>>> +       struct bt_gatt_server *server;
>>>> +       uint16_t mtu;
>>>> +
>>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_=
STRING
>>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_ST=
RING,
>>>> +                                       &dict);
>>>> +
>>>> +       append_options(&dict, op);
>>>> +
>>>> +       server =3D btd_device_get_gatt_server(op->device);
>>>> +
>>>> +       mtu =3D bt_gatt_server_get_mtu(server);
>>>> +
>>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
>>>> +
>>>> +       dbus_message_iter_close_container(iter, &dict);
>>>> +}
>>>> +
>>>> +static struct pending_op *acquire_write(struct external_chrc *chrc,
>>>> +                                       struct btd_device *device,
>>>> +                                       struct gatt_db_attribute *attr=
ib,
>>>> +                                       unsigned int id,
>>>> +                                       const uint8_t *value, size_t l=
en)
>>>> +{
>>>> +       struct pending_op *op;
>>>> +
>>>> +       op =3D pending_write_new(device, NULL, attrib, id, value, len,=
 0);
>>>> +
>>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>>>> +                                       acquire_write_setup,
>>>> +                                       acquire_write_reply,
>>>> +                                       op, pending_op_free))
>>>> +               return op;
>>>> +
>>>> +       pending_op_free(op);
>>>> +
>>>> +       return NULL;
>>>> +}
>>>> +
>>>>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>>  {
>>>>         struct external_chrc *chrc =3D user_data;
>>>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attribu=
te *attrib,
>>>>         struct external_chrc *chrc =3D user_data;
>>>>         struct btd_device *device;
>>>>         struct queue *queue;
>>>> +       DBusMessageIter iter;
>>>>
>>>>         if (chrc->attrib !=3D attrib) {
>>>>                 error("Write callback called with incorrect attribute"=
);
>>>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attrib=
ute *attrib,
>>>>                 goto fail;
>>>>         }
>>>>
>>>> +       if (chrc->write_io) {
>>>> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
>>>> +                       error("Unable to write: %s", strerror(errno));
>>>> +                       goto fail;
>>>> +               }
>>>> +
>>>> +               gatt_db_attribute_write_result(attrib, id, 0);
>>>> +               return;
>>>> +       }
>>>> +
>>> Could you elaborate more about this part of codes, why are you using
>>> pipe_io_send to update value instead of dbus?
>>
>> This is doing the write via fd if write_io has been acquired, the
>> whole point is avoiding D-Bus.
>>
>>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &i=
ter)) {
>>>> +               if (acquire_write(chrc, device, attrib, id, value, len=
))
>>>> +                       return;
>>>> +       }
>>>> +
>>>
>>> Currently my expected scenario is to get MTU when first Gatt Write(
>>> write with response) comes in specific characteristic, for example, I
>>> use iphone as ble central and write value in Characteristic A on Bluez
>>> peripheral, Bluez peripheral get negotiated MTU when first write comes
>>> in. From this part of codes, it seems it return immediately after
>>> acquire_write is called, how can we move forward and write value to
>>> peripheral application? What is the design idea here?
>>
>> After AcquireWrite the data is sent via fd as well:
>>
>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-databas=
e.c#n1891
>>
>>> The below is earlier draft idea for this MTU acquisition in server side=
.
>>> https://www.spinics.net/lists/linux-bluetooth/msg71961.html
>>>
>>>>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>>>>                 queue =3D chrc->pending_writes;
>>>>         else
>>>> --
>>>> 2.13.5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-blueto=
oth" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz



--=20
Luiz Augusto von Dentz

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Yunhan Wang]

Hi, Luiz

I see. Great! Thanks for your explanation.

Maybe it is good to have both solutions in current bluez?
1. AcquireWrite, write data passing via fd, mtu passing by DBUS, we
can take advantage of fd under high load.
2. Write, write data and mtu passing over DBUS, we still can fully use
DBUS under low load.

which means the only light change is to add mtu passing over DBUS in 2. Wri=
te

Finally both solutions can be available to bluez users.

Thanks
Best wishes
Yunhan

On Fri, Sep 29, 2017 at 12:22 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunhan,
>
> On Fri, Sep 29, 2017 at 10:14 AM, Yunhan Wang <yunhanw@nestlabs.com> wrot=
e:
>> Hi, Luiz
>>
>> For server, what is the benefit to use fd instead of DBUS to pass
>> write value? Isn't it better that we put all bluez client and server
>> only chat via DBUS upon bluetoothd? For mtu, definitely we can do it
>> using DBUS message, if we use current acquire_write, in server
>> application side, we will have to implement callback for
>> "AcquireWrite" instead of "WriteValue=E2=80=9C=E3=80=81
>
> For byte streams passing data over D-Bus is inefficient as that has is
> delivered to dbus-daemon before the end application it will always be
> slower than sending directly via fd, actually depending on the load on
> the system this may impact other processes using D-Bus as the daemon
> will be busy processing the WriteValue messages.
>
>> Thanks
>> Best wishes
>> Yunhan
>>
>> On Thu, Sep 28, 2017 at 11:54 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Yunhan,
>>>
>>> On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@nestlabs.com> wro=
te:
>>>> Hi, Luiz
>>>>
>>>> May I ask several questions about acquire write implementation?
>>>>
>>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>
>>>>> This enables IO via file descriptors using AcquireWrite if server
>>>>> implements it.
>>>>> ---
>>>>>  src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++++=
++++++++++
>>>>>  1 file changed, 142 insertions(+)
>>>>>
>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>>>> index 61eed71d6..239fe5384 100644
>>>>> --- a/src/gatt-database.c
>>>>> +++ b/src/gatt-database.c
>>>>> @@ -33,6 +33,7 @@
>>>>>  #include "gdbus/gdbus.h"
>>>>>  #include "src/shared/util.h"
>>>>>  #include "src/shared/queue.h"
>>>>> +#include "src/shared/io.h"
>>>>>  #include "src/shared/att.h"
>>>>>  #include "src/shared/gatt-db.h"
>>>>>  #include "src/shared/gatt-server.h"
>>>>> @@ -117,6 +118,7 @@ struct external_chrc {
>>>>>         uint8_t props;
>>>>>         uint8_t ext_props;
>>>>>         uint32_t perm;
>>>>> +       struct io *write_io;
>>>>>         struct gatt_db_attribute *attrib;
>>>>>         struct gatt_db_attribute *ccc;
>>>>>         struct queue *pending_reads;
>>>>> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>>>>>  {
>>>>>         struct external_chrc *chrc =3D data;
>>>>>
>>>>> +       io_destroy(chrc->write_io);
>>>>> +
>>>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>>>
>>>>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct b=
td_device *device,
>>>>>         return NULL;
>>>>>  }
>>>>>
>>>>> +static bool pipe_hup(struct io *io, void *user_data)
>>>>> +{
>>>>> +       struct external_chrc *chrc =3D user_data;
>>>>> +
>>>>> +       DBG("%p closed\n", io);
>>>>> +
>>>>> +       if (io =3D=3D chrc->write_io)
>>>>> +               chrc->write_io =3D NULL;
>>>>> +
>>>>> +       io_destroy(io);
>>>>> +
>>>>> +       return false;
>>>>> +}
>>>>> +
>>>>> +static struct io *pipe_io_new(int fd, void *user_data)
>>>>> +{
>>>>> +       struct io *io;
>>>>> +
>>>>> +       io =3D io_new(fd);
>>>>> +
>>>>> +       io_set_close_on_destroy(io, true);
>>>>> +
>>>>> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>>> +
>>>>> +       return io;
>>>>> +}
>>>>> +
>>>>> +static int pipe_io_send(struct io *io, const void *data, size_t len)
>>>>> +{
>>>>> +       struct iovec iov;
>>>>> +
>>>>> +       iov.iov_base =3D (void *) data;
>>>>> +       iov.iov_len =3D len;
>>>>> +
>>>>> +       return io_send(io, &iov, 1);
>>>>> +}
>>>>> +
>>>>> +static void acquire_write_reply(DBusMessage *message, void *user_dat=
a)
>>>>> +{
>>>>> +       struct pending_op *op =3D user_data;
>>>>> +       struct external_chrc *chrc;
>>>>> +       DBusError err;
>>>>> +       int fd;
>>>>> +       uint16_t mtu;
>>>>> +
>>>>> +       chrc =3D gatt_db_attribute_get_user_data(op->attrib);
>>>>> +       dbus_error_init(&err);
>>>>> +
>>>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>>>>> +               error("Failed to acquire write: %s\n", err.name);
>>>>> +               dbus_error_free(&err);
>>>>> +               goto retry;
>>>>> +       }
>>>>> +
>>>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, =
&fd,
>>>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>>>> +                                       DBUS_TYPE_INVALID) =3D=3D fal=
se)) {
>>>>> +               error("Invalid AcquireWrite response\n");
>>>>> +               goto retry;
>>>>> +       }
>>>>> +
>>>>> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
>>>>> +
>>>>> +       chrc->write_io =3D pipe_io_new(fd, chrc);
>>>>> +
>>>>> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
>>>>> +                               op->data.iov_len) < 0)
>>>>> +               goto retry;
>>>>> +
>>>>> +       return;
>>>>> +
>>>>> +retry:
>>>>> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
>>>>> +                               op->data.iov_base, op->data.iov_len, =
0);
>>>>> +}
>>>>> +
>>>>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_da=
ta)
>>>>> +{
>>>>> +       struct pending_op *op =3D user_data;
>>>>> +       DBusMessageIter dict;
>>>>> +       struct bt_gatt_server *server;
>>>>> +       uint16_t mtu;
>>>>> +
>>>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS=
_STRING
>>>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_S=
TRING,
>>>>> +                                       &dict);
>>>>> +
>>>>> +       append_options(&dict, op);
>>>>> +
>>>>> +       server =3D btd_device_get_gatt_server(op->device);
>>>>> +
>>>>> +       mtu =3D bt_gatt_server_get_mtu(server);
>>>>> +
>>>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
>>>>> +
>>>>> +       dbus_message_iter_close_container(iter, &dict);
>>>>> +}
>>>>> +
>>>>> +static struct pending_op *acquire_write(struct external_chrc *chrc,
>>>>> +                                       struct btd_device *device,
>>>>> +                                       struct gatt_db_attribute *att=
rib,
>>>>> +                                       unsigned int id,
>>>>> +                                       const uint8_t *value, size_t =
len)
>>>>> +{
>>>>> +       struct pending_op *op;
>>>>> +
>>>>> +       op =3D pending_write_new(device, NULL, attrib, id, value, len=
, 0);
>>>>> +
>>>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>>>>> +                                       acquire_write_setup,
>>>>> +                                       acquire_write_reply,
>>>>> +                                       op, pending_op_free))
>>>>> +               return op;
>>>>> +
>>>>> +       pending_op_free(op);
>>>>> +
>>>>> +       return NULL;
>>>>> +}
>>>>> +
>>>>>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>>>  {
>>>>>         struct external_chrc *chrc =3D user_data;
>>>>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attrib=
ute *attrib,
>>>>>         struct external_chrc *chrc =3D user_data;
>>>>>         struct btd_device *device;
>>>>>         struct queue *queue;
>>>>> +       DBusMessageIter iter;
>>>>>
>>>>>         if (chrc->attrib !=3D attrib) {
>>>>>                 error("Write callback called with incorrect attribute=
");
>>>>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attri=
bute *attrib,
>>>>>                 goto fail;
>>>>>         }
>>>>>
>>>>> +       if (chrc->write_io) {
>>>>> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
>>>>> +                       error("Unable to write: %s", strerror(errno))=
;
>>>>> +                       goto fail;
>>>>> +               }
>>>>> +
>>>>> +               gatt_db_attribute_write_result(attrib, id, 0);
>>>>> +               return;
>>>>> +       }
>>>>> +
>>>> Could you elaborate more about this part of codes, why are you using
>>>> pipe_io_send to update value instead of dbus?
>>>
>>> This is doing the write via fd if write_io has been acquired, the
>>> whole point is avoiding D-Bus.
>>>
>>>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &=
iter)) {
>>>>> +               if (acquire_write(chrc, device, attrib, id, value, le=
n))
>>>>> +                       return;
>>>>> +       }
>>>>> +
>>>>
>>>> Currently my expected scenario is to get MTU when first Gatt Write(
>>>> write with response) comes in specific characteristic, for example, I
>>>> use iphone as ble central and write value in Characteristic A on Bluez
>>>> peripheral, Bluez peripheral get negotiated MTU when first write comes
>>>> in. From this part of codes, it seems it return immediately after
>>>> acquire_write is called, how can we move forward and write value to
>>>> peripheral application? What is the design idea here?
>>>
>>> After AcquireWrite the data is sent via fd as well:
>>>
>>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-databa=
se.c#n1891
>>>
>>>> The below is earlier draft idea for this MTU acquisition in server sid=
e.
>>>> https://www.spinics.net/lists/linux-bluetooth/msg71961.html
>>>>
>>>>>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>>>>>                 queue =3D chrc->pending_writes;
>>>>>         else
>>>>> --
>>>>> 2.13.5
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluet=
ooth" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Luiz Augusto von Dentz]

Hi Yunhan,

On Fri, Sep 29, 2017 at 10:50 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> I see. Great! Thanks for your explanation.

Please comment inline on the list.

> Maybe it is good to have both solutions in current bluez?
> 1. AcquireWrite, write data passing via fd, mtu passing by DBUS, we
> can take advantage of fd under high load.
> 2. Write, write data and mtu passing over DBUS, we still can fully use
> DBUS under low load.

Well that is actually a 3 type of transfer since regular WriteValue
already exists which defeat a little bit the purpose of WriteAcquired
since we can no longer determine if the server supports fd mode or
not, I think it is too much and the server has to either to decide if
unfragmented D-Bus transfer is fine is not, if not then fd passing
shall be the only mechanism.

Considering how easy it was to add support for fd passing on the
bluetoothctl I don't thing it is that big of deal really, or there is
something else preventing this to happen in your end?

> which means the only light change is to add mtu passing over DBUS in 2. W=
rite
>
> Finally both solutions can be available to bluez users.
>
> Thanks
> Best wishes
> Yunhan
>
> On Fri, Sep 29, 2017 at 12:22 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Yunhan,
>>
>> On Fri, Sep 29, 2017 at 10:14 AM, Yunhan Wang <yunhanw@nestlabs.com> wro=
te:
>>> Hi, Luiz
>>>
>>> For server, what is the benefit to use fd instead of DBUS to pass
>>> write value? Isn't it better that we put all bluez client and server
>>> only chat via DBUS upon bluetoothd? For mtu, definitely we can do it
>>> using DBUS message, if we use current acquire_write, in server
>>> application side, we will have to implement callback for
>>> "AcquireWrite" instead of "WriteValue=E2=80=9C=E3=80=81
>>
>> For byte streams passing data over D-Bus is inefficient as that has is
>> delivered to dbus-daemon before the end application it will always be
>> slower than sending directly via fd, actually depending on the load on
>> the system this may impact other processes using D-Bus as the daemon
>> will be busy processing the WriteValue messages.
>>
>>> Thanks
>>> Best wishes
>>> Yunhan
>>>
>>> On Thu, Sep 28, 2017 at 11:54 PM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> Hi Yunhan,
>>>>
>>>> On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@nestlabs.com> wr=
ote:
>>>>> Hi, Luiz
>>>>>
>>>>> May I ask several questions about acquire write implementation?
>>>>>
>>>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>>
>>>>>> This enables IO via file descriptors using AcquireWrite if server
>>>>>> implements it.
>>>>>> ---
>>>>>>  src/gatt-database.c | 142 +++++++++++++++++++++++++++++++++++++++++=
+++++++++++
>>>>>>  1 file changed, 142 insertions(+)
>>>>>>
>>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>>>>> index 61eed71d6..239fe5384 100644
>>>>>> --- a/src/gatt-database.c
>>>>>> +++ b/src/gatt-database.c
>>>>>> @@ -33,6 +33,7 @@
>>>>>>  #include "gdbus/gdbus.h"
>>>>>>  #include "src/shared/util.h"
>>>>>>  #include "src/shared/queue.h"
>>>>>> +#include "src/shared/io.h"
>>>>>>  #include "src/shared/att.h"
>>>>>>  #include "src/shared/gatt-db.h"
>>>>>>  #include "src/shared/gatt-server.h"
>>>>>> @@ -117,6 +118,7 @@ struct external_chrc {
>>>>>>         uint8_t props;
>>>>>>         uint8_t ext_props;
>>>>>>         uint32_t perm;
>>>>>> +       struct io *write_io;
>>>>>>         struct gatt_db_attribute *attrib;
>>>>>>         struct gatt_db_attribute *ccc;
>>>>>>         struct queue *pending_reads;
>>>>>> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>>>>>>  {
>>>>>>         struct external_chrc *chrc =3D data;
>>>>>>
>>>>>> +       io_destroy(chrc->write_io);
>>>>>> +
>>>>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>>>>
>>>>>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct =
btd_device *device,
>>>>>>         return NULL;
>>>>>>  }
>>>>>>
>>>>>> +static bool pipe_hup(struct io *io, void *user_data)
>>>>>> +{
>>>>>> +       struct external_chrc *chrc =3D user_data;
>>>>>> +
>>>>>> +       DBG("%p closed\n", io);
>>>>>> +
>>>>>> +       if (io =3D=3D chrc->write_io)
>>>>>> +               chrc->write_io =3D NULL;
>>>>>> +
>>>>>> +       io_destroy(io);
>>>>>> +
>>>>>> +       return false;
>>>>>> +}
>>>>>> +
>>>>>> +static struct io *pipe_io_new(int fd, void *user_data)
>>>>>> +{
>>>>>> +       struct io *io;
>>>>>> +
>>>>>> +       io =3D io_new(fd);
>>>>>> +
>>>>>> +       io_set_close_on_destroy(io, true);
>>>>>> +
>>>>>> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>>>> +
>>>>>> +       return io;
>>>>>> +}
>>>>>> +
>>>>>> +static int pipe_io_send(struct io *io, const void *data, size_t len=
)
>>>>>> +{
>>>>>> +       struct iovec iov;
>>>>>> +
>>>>>> +       iov.iov_base =3D (void *) data;
>>>>>> +       iov.iov_len =3D len;
>>>>>> +
>>>>>> +       return io_send(io, &iov, 1);
>>>>>> +}
>>>>>> +
>>>>>> +static void acquire_write_reply(DBusMessage *message, void *user_da=
ta)
>>>>>> +{
>>>>>> +       struct pending_op *op =3D user_data;
>>>>>> +       struct external_chrc *chrc;
>>>>>> +       DBusError err;
>>>>>> +       int fd;
>>>>>> +       uint16_t mtu;
>>>>>> +
>>>>>> +       chrc =3D gatt_db_attribute_get_user_data(op->attrib);
>>>>>> +       dbus_error_init(&err);
>>>>>> +
>>>>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) =
{
>>>>>> +               error("Failed to acquire write: %s\n", err.name);
>>>>>> +               dbus_error_free(&err);
>>>>>> +               goto retry;
>>>>>> +       }
>>>>>> +
>>>>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD,=
 &fd,
>>>>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>>>>> +                                       DBUS_TYPE_INVALID) =3D=3D fa=
lse)) {
>>>>>> +               error("Invalid AcquireWrite response\n");
>>>>>> +               goto retry;
>>>>>> +       }
>>>>>> +
>>>>>> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
>>>>>> +
>>>>>> +       chrc->write_io =3D pipe_io_new(fd, chrc);
>>>>>> +
>>>>>> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
>>>>>> +                               op->data.iov_len) < 0)
>>>>>> +               goto retry;
>>>>>> +
>>>>>> +       return;
>>>>>> +
>>>>>> +retry:
>>>>>> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id=
,
>>>>>> +                               op->data.iov_base, op->data.iov_len,=
 0);
>>>>>> +}
>>>>>> +
>>>>>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_d=
ata)
>>>>>> +{
>>>>>> +       struct pending_op *op =3D user_data;
>>>>>> +       DBusMessageIter dict;
>>>>>> +       struct bt_gatt_server *server;
>>>>>> +       uint16_t mtu;
>>>>>> +
>>>>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_A=
S_STRING
>>>>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>>>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>>>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_=
STRING,
>>>>>> +                                       &dict);
>>>>>> +
>>>>>> +       append_options(&dict, op);
>>>>>> +
>>>>>> +       server =3D btd_device_get_gatt_server(op->device);
>>>>>> +
>>>>>> +       mtu =3D bt_gatt_server_get_mtu(server);
>>>>>> +
>>>>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
>>>>>> +
>>>>>> +       dbus_message_iter_close_container(iter, &dict);
>>>>>> +}
>>>>>> +
>>>>>> +static struct pending_op *acquire_write(struct external_chrc *chrc,
>>>>>> +                                       struct btd_device *device,
>>>>>> +                                       struct gatt_db_attribute *at=
trib,
>>>>>> +                                       unsigned int id,
>>>>>> +                                       const uint8_t *value, size_t=
 len)
>>>>>> +{
>>>>>> +       struct pending_op *op;
>>>>>> +
>>>>>> +       op =3D pending_write_new(device, NULL, attrib, id, value, le=
n, 0);
>>>>>> +
>>>>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>>>>>> +                                       acquire_write_setup,
>>>>>> +                                       acquire_write_reply,
>>>>>> +                                       op, pending_op_free))
>>>>>> +               return op;
>>>>>> +
>>>>>> +       pending_op_free(op);
>>>>>> +
>>>>>> +       return NULL;
>>>>>> +}
>>>>>> +
>>>>>>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>>>>  {
>>>>>>         struct external_chrc *chrc =3D user_data;
>>>>>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attri=
bute *attrib,
>>>>>>         struct external_chrc *chrc =3D user_data;
>>>>>>         struct btd_device *device;
>>>>>>         struct queue *queue;
>>>>>> +       DBusMessageIter iter;
>>>>>>
>>>>>>         if (chrc->attrib !=3D attrib) {
>>>>>>                 error("Write callback called with incorrect attribut=
e");
>>>>>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attr=
ibute *attrib,
>>>>>>                 goto fail;
>>>>>>         }
>>>>>>
>>>>>> +       if (chrc->write_io) {
>>>>>> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
>>>>>> +                       error("Unable to write: %s", strerror(errno)=
);
>>>>>> +                       goto fail;
>>>>>> +               }
>>>>>> +
>>>>>> +               gatt_db_attribute_write_result(attrib, id, 0);
>>>>>> +               return;
>>>>>> +       }
>>>>>> +
>>>>> Could you elaborate more about this part of codes, why are you using
>>>>> pipe_io_send to update value instead of dbus?
>>>>
>>>> This is doing the write via fd if write_io has been acquired, the
>>>> whole point is avoiding D-Bus.
>>>>
>>>>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", =
&iter)) {
>>>>>> +               if (acquire_write(chrc, device, attrib, id, value, l=
en))
>>>>>> +                       return;
>>>>>> +       }
>>>>>> +
>>>>>
>>>>> Currently my expected scenario is to get MTU when first Gatt Write(
>>>>> write with response) comes in specific characteristic, for example, I
>>>>> use iphone as ble central and write value in Characteristic A on Blue=
z
>>>>> peripheral, Bluez peripheral get negotiated MTU when first write come=
s
>>>>> in. From this part of codes, it seems it return immediately after
>>>>> acquire_write is called, how can we move forward and write value to
>>>>> peripheral application? What is the design idea here?
>>>>
>>>> After AcquireWrite the data is sent via fd as well:
>>>>
>>>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-datab=
ase.c#n1891
>>>>
>>>>> The below is earlier draft idea for this MTU acquisition in server si=
de.
>>>>> https://www.spinics.net/lists/linux-bluetooth/msg71961.html
>>>>>
>>>>>>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>>>>>>                 queue =3D chrc->pending_writes;
>>>>>>         else
>>>>>> --
>>>>>> 2.13.5
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-blue=
tooth" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz



--=20
Luiz Augusto von Dentz

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Yunhan Wang]

Hi, Luiz

On Fri, Sep 29, 2017 at 1:02 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunhan,
>
> On Fri, Sep 29, 2017 at 10:50 AM, Yunhan Wang <yunhanw@nestlabs.com> wrot=
e:
>> Hi, Luiz
>>
>> I see. Great! Thanks for your explanation.
>
> Please comment inline on the list.
>
>> Maybe it is good to have both solutions in current bluez?
>> 1. AcquireWrite, write data passing via fd, mtu passing by DBUS, we
>> can take advantage of fd under high load.
>> 2. Write, write data and mtu passing over DBUS, we still can fully use
>> DBUS under low load.
>
> Well that is actually a 3 type of transfer since regular WriteValue
> already exists which defeat a little bit the purpose of WriteAcquired
> since we can no longer determine if the server supports fd mode or
> not, I think it is too much and the server has to either to decide if
> unfragmented D-Bus transfer is fine is not, if not then fd passing
> shall be the only mechanism.
>
I agree, it is three type of transfer,
regular write,
regular write with mtu support(application is responsible for packet
fragmentation)
fd with pipe (application is responsible for packet fragmentation),

now we have two kind of transfer(regular write, acquire write) in
code, add third one is pretty straightforward and only small code
change.

I read two articles(https://blogs.gnome.org/abustany/2010/05/20/ipc-perform=
ance-the-return-of-the-report/,
https://lists.freedesktop.org/archives/dbus/2014-October/016363.html),
I agree that fd with pipe is more efficent, and eliminate several
copy, and benefit for byte streaming although there is minor
vulnerability mentioned in above article,  I hope engineer in
peripheral application can still have another choice, regular write
with mtu, when traffic is low or it is experiment purpose. Considering
these are pretty similar, easy to integrate, engineer still can easily
switch them between fd with pipe and regular write.

> Considering how easy it was to add support for fd passing on the
> bluetoothctl I don't thing it is that big of deal really, or there is
> something else preventing this to happen in your end?
>
Yes, fd passing is pretty easy and efficent, it is not big deal to
integrate it, using this way, in my end, for gatt write with response
from central to peripheral, I can see written value and length in
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n688,
but I cannot see any response back for GATT write from peripheral to
central. It seems this only support GATT write without response even
though I have removed the check
here(https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#=
n1360).
Any idea?

In contrast, for regular WriteValue, I can see write response  from
peripheral to central.

>> which means the only light change is to add mtu passing over DBUS in 2. =
Write
>>
>> Finally both solutions can be available to bluez users.
>>
>> Thanks
>> Best wishes
>> Yunhan
>>
>> On Fri, Sep 29, 2017 at 12:22 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Yunhan,
>>>
>>> On Fri, Sep 29, 2017 at 10:14 AM, Yunhan Wang <yunhanw@nestlabs.com> wr=
ote:
>>>> Hi, Luiz
>>>>
>>>> For server, what is the benefit to use fd instead of DBUS to pass
>>>> write value? Isn't it better that we put all bluez client and server
>>>> only chat via DBUS upon bluetoothd? For mtu, definitely we can do it
>>>> using DBUS message, if we use current acquire_write, in server
>>>> application side, we will have to implement callback for
>>>> "AcquireWrite" instead of "WriteValue=E2=80=9C=E3=80=81
>>>
>>> For byte streams passing data over D-Bus is inefficient as that has is
>>> delivered to dbus-daemon before the end application it will always be
>>> slower than sending directly via fd, actually depending on the load on
>>> the system this may impact other processes using D-Bus as the daemon
>>> will be busy processing the WriteValue messages.
>>>
>>>> Thanks
>>>> Best wishes
>>>> Yunhan
>>>>
>>>> On Thu, Sep 28, 2017 at 11:54 PM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> Hi Yunhan,
>>>>>
>>>>> On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang <yunhanw@nestlabs.com> w=
rote:
>>>>>> Hi, Luiz
>>>>>>
>>>>>> May I ask several questions about acquire write implementation?
>>>>>>
>>>>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>>>
>>>>>>> This enables IO via file descriptors using AcquireWrite if server
>>>>>>> implements it.
>>>>>>> ---
>>>>>>>  src/gatt-database.c | 142 ++++++++++++++++++++++++++++++++++++++++=
++++++++++++
>>>>>>>  1 file changed, 142 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>>>>>> index 61eed71d6..239fe5384 100644
>>>>>>> --- a/src/gatt-database.c
>>>>>>> +++ b/src/gatt-database.c
>>>>>>> @@ -33,6 +33,7 @@
>>>>>>>  #include "gdbus/gdbus.h"
>>>>>>>  #include "src/shared/util.h"
>>>>>>>  #include "src/shared/queue.h"
>>>>>>> +#include "src/shared/io.h"
>>>>>>>  #include "src/shared/att.h"
>>>>>>>  #include "src/shared/gatt-db.h"
>>>>>>>  #include "src/shared/gatt-server.h"
>>>>>>> @@ -117,6 +118,7 @@ struct external_chrc {
>>>>>>>         uint8_t props;
>>>>>>>         uint8_t ext_props;
>>>>>>>         uint32_t perm;
>>>>>>> +       struct io *write_io;
>>>>>>>         struct gatt_db_attribute *attrib;
>>>>>>>         struct gatt_db_attribute *ccc;
>>>>>>>         struct queue *pending_reads;
>>>>>>> @@ -325,6 +327,8 @@ static void chrc_free(void *data)
>>>>>>>  {
>>>>>>>         struct external_chrc *chrc =3D data;
>>>>>>>
>>>>>>> +       io_destroy(chrc->write_io);
>>>>>>> +
>>>>>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>>>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>>>>>
>>>>>>> @@ -1789,6 +1793,128 @@ static struct pending_op *send_write(struct=
 btd_device *device,
>>>>>>>         return NULL;
>>>>>>>  }
>>>>>>>
>>>>>>> +static bool pipe_hup(struct io *io, void *user_data)
>>>>>>> +{
>>>>>>> +       struct external_chrc *chrc =3D user_data;
>>>>>>> +
>>>>>>> +       DBG("%p closed\n", io);
>>>>>>> +
>>>>>>> +       if (io =3D=3D chrc->write_io)
>>>>>>> +               chrc->write_io =3D NULL;
>>>>>>> +
>>>>>>> +       io_destroy(io);
>>>>>>> +
>>>>>>> +       return false;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static struct io *pipe_io_new(int fd, void *user_data)
>>>>>>> +{
>>>>>>> +       struct io *io;
>>>>>>> +
>>>>>>> +       io =3D io_new(fd);
>>>>>>> +
>>>>>>> +       io_set_close_on_destroy(io, true);
>>>>>>> +
>>>>>>> +       io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>>>>> +
>>>>>>> +       return io;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int pipe_io_send(struct io *io, const void *data, size_t le=
n)
>>>>>>> +{
>>>>>>> +       struct iovec iov;
>>>>>>> +
>>>>>>> +       iov.iov_base =3D (void *) data;
>>>>>>> +       iov.iov_len =3D len;
>>>>>>> +
>>>>>>> +       return io_send(io, &iov, 1);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void acquire_write_reply(DBusMessage *message, void *user_d=
ata)
>>>>>>> +{
>>>>>>> +       struct pending_op *op =3D user_data;
>>>>>>> +       struct external_chrc *chrc;
>>>>>>> +       DBusError err;
>>>>>>> +       int fd;
>>>>>>> +       uint16_t mtu;
>>>>>>> +
>>>>>>> +       chrc =3D gatt_db_attribute_get_user_data(op->attrib);
>>>>>>> +       dbus_error_init(&err);
>>>>>>> +
>>>>>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE)=
 {
>>>>>>> +               error("Failed to acquire write: %s\n", err.name);
>>>>>>> +               dbus_error_free(&err);
>>>>>>> +               goto retry;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD=
, &fd,
>>>>>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>>>>>> +                                       DBUS_TYPE_INVALID) =3D=3D f=
alse)) {
>>>>>>> +               error("Invalid AcquireWrite response\n");
>>>>>>> +               goto retry;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);
>>>>>>> +
>>>>>>> +       chrc->write_io =3D pipe_io_new(fd, chrc);
>>>>>>> +
>>>>>>> +       if (pipe_io_send(chrc->write_io, op->data.iov_base,
>>>>>>> +                               op->data.iov_len) < 0)
>>>>>>> +               goto retry;
>>>>>>> +
>>>>>>> +       return;
>>>>>>> +
>>>>>>> +retry:
>>>>>>> +       send_write(op->device, op->attrib, chrc->proxy, NULL, op->i=
d,
>>>>>>> +                               op->data.iov_base, op->data.iov_len=
, 0);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void acquire_write_setup(DBusMessageIter *iter, void *user_=
data)
>>>>>>> +{
>>>>>>> +       struct pending_op *op =3D user_data;
>>>>>>> +       DBusMessageIter dict;
>>>>>>> +       struct bt_gatt_server *server;
>>>>>>> +       uint16_t mtu;
>>>>>>> +
>>>>>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>>>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_=
AS_STRING
>>>>>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>>>>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>>>>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS=
_STRING,
>>>>>>> +                                       &dict);
>>>>>>> +
>>>>>>> +       append_options(&dict, op);
>>>>>>> +
>>>>>>> +       server =3D btd_device_get_gatt_server(op->device);
>>>>>>> +
>>>>>>> +       mtu =3D bt_gatt_server_get_mtu(server);
>>>>>>> +
>>>>>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &mtu);
>>>>>>> +
>>>>>>> +       dbus_message_iter_close_container(iter, &dict);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static struct pending_op *acquire_write(struct external_chrc *chrc=
,
>>>>>>> +                                       struct btd_device *device,
>>>>>>> +                                       struct gatt_db_attribute *a=
ttrib,
>>>>>>> +                                       unsigned int id,
>>>>>>> +                                       const uint8_t *value, size_=
t len)
>>>>>>> +{
>>>>>>> +       struct pending_op *op;
>>>>>>> +
>>>>>>> +       op =3D pending_write_new(device, NULL, attrib, id, value, l=
en, 0);
>>>>>>> +
>>>>>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>>>>>>> +                                       acquire_write_setup,
>>>>>>> +                                       acquire_write_reply,
>>>>>>> +                                       op, pending_op_free))
>>>>>>> +               return op;
>>>>>>> +
>>>>>>> +       pending_op_free(op);
>>>>>>> +
>>>>>>> +       return NULL;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>>>>>  {
>>>>>>>         struct external_chrc *chrc =3D user_data;
>>>>>>> @@ -2090,6 +2216,7 @@ static void chrc_write_cb(struct gatt_db_attr=
ibute *attrib,
>>>>>>>         struct external_chrc *chrc =3D user_data;
>>>>>>>         struct btd_device *device;
>>>>>>>         struct queue *queue;
>>>>>>> +       DBusMessageIter iter;
>>>>>>>
>>>>>>>         if (chrc->attrib !=3D attrib) {
>>>>>>>                 error("Write callback called with incorrect attribu=
te");
>>>>>>> @@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_att=
ribute *attrib,
>>>>>>>                 goto fail;
>>>>>>>         }
>>>>>>>
>>>>>>> +       if (chrc->write_io) {
>>>>>>> +               if (pipe_io_send(chrc->write_io, value, len) < 0) {
>>>>>>> +                       error("Unable to write: %s", strerror(errno=
));
>>>>>>> +                       goto fail;
>>>>>>> +               }
>>>>>>> +
>>>>>>> +               gatt_db_attribute_write_result(attrib, id, 0);
>>>>>>> +               return;
>>>>>>> +       }
>>>>>>> +
>>>>>> Could you elaborate more about this part of codes, why are you using
>>>>>> pipe_io_send to update value instead of dbus?
>>>>>
>>>>> This is doing the write via fd if write_io has been acquired, the
>>>>> whole point is avoiding D-Bus.
>>>>>
>>>>>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired",=
 &iter)) {
>>>>>>> +               if (acquire_write(chrc, device, attrib, id, value, =
len))
>>>>>>> +                       return;
>>>>>>> +       }
>>>>>>> +
>>>>>>
>>>>>> Currently my expected scenario is to get MTU when first Gatt Write(
>>>>>> write with response) comes in specific characteristic, for example, =
I
>>>>>> use iphone as ble central and write value in Characteristic A on Blu=
ez
>>>>>> peripheral, Bluez peripheral get negotiated MTU when first write com=
es
>>>>>> in. From this part of codes, it seems it return immediately after
>>>>>> acquire_write is called, how can we move forward and write value to
>>>>>> peripheral application? What is the design idea here?
>>>>>
>>>>> After AcquireWrite the data is sent via fd as well:
>>>>>
>>>>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-data=
base.c#n1891
>>>>>
>>>>>> The below is earlier draft idea for this MTU acquisition in server s=
ide.
>>>>>> https://www.spinics.net/lists/linux-bluetooth/msg71961.html
>>>>>>
>>>>>>>         if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
>>>>>>>                 queue =3D chrc->pending_writes;
>>>>>>>         else
>>>>>>> --
>>>>>>> 2.13.5
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-blu=
etooth" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Luiz Augusto von Dentz
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Luiz Augusto von Dentz]

Hi Yunhan,

On Sat, Sep 30, 2017 at 6:39 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> On Fri, Sep 29, 2017 at 1:02 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Yunhan,
>>
>> On Fri, Sep 29, 2017 at 10:50 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
>>> Hi, Luiz
>>>
>>> I see. Great! Thanks for your explanation.
>>
>> Please comment inline on the list.
>>
>>> Maybe it is good to have both solutions in current bluez?
>>> 1. AcquireWrite, write data passing via fd, mtu passing by DBUS, we
>>> can take advantage of fd under high load.
>>> 2. Write, write data and mtu passing over DBUS, we still can fully use
>>> DBUS under low load.
>>
>> Well that is actually a 3 type of transfer since regular WriteValue
>> already exists which defeat a little bit the purpose of WriteAcquired
>> since we can no longer determine if the server supports fd mode or
>> not, I think it is too much and the server has to either to decide if
>> unfragmented D-Bus transfer is fine is not, if not then fd passing
>> shall be the only mechanism.
>>
> I agree, it is three type of transfer,
> regular write,
> regular write with mtu support(application is responsible for packet
> fragmentation)
> fd with pipe (application is responsible for packet fragmentation),

There is no advantage on having to call AcquireWrite and then _not_
use the fd but WriteValue, so IMO it makes no sense to offer something
that has no benefit.

> now we have two kind of transfer(regular write, acquire write) in
> code, add third one is pretty straightforward and only small code
> change.
>
> I read two articles(https://blogs.gnome.org/abustany/2010/05/20/ipc-performance-the-return-of-the-report/,
> https://lists.freedesktop.org/archives/dbus/2014-October/016363.html),
> I agree that fd with pipe is more efficent, and eliminate several
> copy, and benefit for byte streaming although there is minor
> vulnerability mentioned in above article,  I hope engineer in
> peripheral application can still have another choice, regular write
> with mtu, when traffic is low or it is experiment purpose. Considering
> these are pretty similar, easy to integrate, engineer still can easily
> switch them between fd with pipe and regular write.

Regular D-Bus WriteValue and fd passing via AcquireWrite do cover all
cases... too many choices is actually a bad thing to an API as it
increases the complexity of the code and number of bugs, so normally
we target to have as little APIs as possible.

>> Considering how easy it was to add support for fd passing on the
>> bluetoothctl I don't thing it is that big of deal really, or there is
>> something else preventing this to happen in your end?
>>
> Yes, fd passing is pretty easy and efficent, it is not big deal to
> integrate it, using this way, in my end, for gatt write with response
> from central to peripheral, I can see written value and length in
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n688,
> but I cannot see any response back for GATT write from peripheral to
> central. It seems this only support GATT write without response even
> though I have removed the check
> here(https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n1360).
> Any idea?
>
> In contrast, for regular WriteValue, I can see write response  from
> peripheral to central.

Ive sent a fix for this, the responses shall be generated with either
WriteValue or fd write, though we the later we assume that if the data
could be written to the pipe then it should be okay to reply right
there while with WriteValue we actually wait for the reply (adds
latency).

>>> which means the only light change is to add mtu passing over DBUS in 2. Write
>>>
>>> Finally both solutions can be available to bluez users.
>>>
>>> Thanks
>>> Best wishes
>>> Yunhan

Re: [PATCH v2 06/10] gatt: Implement AcquireWrite for server

[Yunhan Wang]

Hi, Luiz

On Mon, Oct 2, 2017 at 4:57 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunhan,
>
> On Sat, Sep 30, 2017 at 6:39 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
>> Hi, Luiz
>>
>> On Fri, Sep 29, 2017 at 1:02 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Yunhan,
>>>
>>> On Fri, Sep 29, 2017 at 10:50 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
>>>> Hi, Luiz
>>>>
>>>> I see. Great! Thanks for your explanation.
>>>
>>> Please comment inline on the list.
>>>
>>>> Maybe it is good to have both solutions in current bluez?
>>>> 1. AcquireWrite, write data passing via fd, mtu passing by DBUS, we
>>>> can take advantage of fd under high load.
>>>> 2. Write, write data and mtu passing over DBUS, we still can fully use
>>>> DBUS under low load.
>>>
>>> Well that is actually a 3 type of transfer since regular WriteValue
>>> already exists which defeat a little bit the purpose of WriteAcquired
>>> since we can no longer determine if the server supports fd mode or
>>> not, I think it is too much and the server has to either to decide if
>>> unfragmented D-Bus transfer is fine is not, if not then fd passing
>>> shall be the only mechanism.
>>>
>> I agree, it is three type of transfer,
>> regular write,
>> regular write with mtu support(application is responsible for packet
>> fragmentation)
>> fd with pipe (application is responsible for packet fragmentation),
>
> There is no advantage on having to call AcquireWrite and then _not_
> use the fd but WriteValue, so IMO it makes no sense to offer something
> that has no benefit.
>
>> now we have two kind of transfer(regular write, acquire write) in
>> code, add third one is pretty straightforward and only small code
>> change.
>>
>> I read two articles(https://blogs.gnome.org/abustany/2010/05/20/ipc-performance-the-return-of-the-report/,
>> https://lists.freedesktop.org/archives/dbus/2014-October/016363.html),
>> I agree that fd with pipe is more efficent, and eliminate several
>> copy, and benefit for byte streaming although there is minor
>> vulnerability mentioned in above article,  I hope engineer in
>> peripheral application can still have another choice, regular write
>> with mtu, when traffic is low or it is experiment purpose. Considering
>> these are pretty similar, easy to integrate, engineer still can easily
>> switch them between fd with pipe and regular write.
>
> Regular D-Bus WriteValue and fd passing via AcquireWrite do cover all
> cases... too many choices is actually a bad thing to an API as it
> increases the complexity of the code and number of bugs, so normally
> we target to have as little APIs as possible.
>
Ok, Understood. Thank you. Let's keep current design and use fd
passing via AcquireWrite this way.

>>> Considering how easy it was to add support for fd passing on the
>>> bluetoothctl I don't thing it is that big of deal really, or there is
>>> something else preventing this to happen in your end?
>>>
>> Yes, fd passing is pretty easy and efficent, it is not big deal to
>> integrate it, using this way, in my end, for gatt write with response
>> from central to peripheral, I can see written value and length in
>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n688,
>> but I cannot see any response back for GATT write from peripheral to
>> central. It seems this only support GATT write without response even
>> though I have removed the check
>> here(https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n1360).
>> Any idea?
>>
>> In contrast, for regular WriteValue, I can see write response  from
>> peripheral to central.
>
> Ive sent a fix for this, the responses shall be generated with either
> WriteValue or fd write, though we the later we assume that if the data
> could be written to the pipe then it should be okay to reply right
> there while with WriteValue we actually wait for the reply (adds
> latency).
>
Yes, just tried, write is working now with your fix. Please merge it to master.

Thank you.

>>>> which means the only light change is to add mtu passing over DBUS in 2. Write
>>>>
>>>> Finally both solutions can be available to bluez users.
>>>>
>>>> Thanks
>>>> Best wishes
>>>> Yunhan

Re: [PATCH v2 08/10] gatt: Implement AcquireNotify for server

[Yunhan Wang]

Hi, Luiz

On Fri, Sep 29, 2017 at 12:03 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunhan,
>
> On Fri, Sep 29, 2017 at 7:47 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote=
:
>> Hi, Luiz
>>
>> May I have several questions about this feature?
>>
>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This enables IO via file descriptors using AcquireWrite if server
>>> implements it.
>>> ---
>>>  src/gatt-database.c | 116 ++++++++++++++++++++++++++++++++++++++++++++=
+++++---
>>>  1 file changed, 111 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>> index 239fe5384..4932568dd 100644
>>> --- a/src/gatt-database.c
>>> +++ b/src/gatt-database.c
>>> @@ -24,6 +24,7 @@
>>>  #include <stdint.h>
>>>  #include <stdlib.h>
>>>  #include <errno.h>
>>> +#include <unistd.h>
>>>
>>>  #include "lib/bluetooth.h"
>>>  #include "lib/sdp.h"
>>> @@ -118,7 +119,9 @@ struct external_chrc {
>>>         uint8_t props;
>>>         uint8_t ext_props;
>>>         uint32_t perm;
>>> +       uint16_t mtu;
>>>         struct io *write_io;
>>> +       struct io *notify_io;
>>>         struct gatt_db_attribute *attrib;
>>>         struct gatt_db_attribute *ccc;
>>>         struct queue *pending_reads;
>>> @@ -152,7 +155,8 @@ struct device_state {
>>>         struct queue *ccc_states;
>>>  };
>>>
>>> -typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value,
>>> +typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att,
>>> +                                                       uint16_t value,
>>>                                                         void *user_data=
);
>>>  typedef void (*btd_gatt_database_destroy_t) (void *data);
>>>
>>> @@ -328,6 +332,7 @@ static void chrc_free(void *data)
>>>         struct external_chrc *chrc =3D data;
>>>
>>>         io_destroy(chrc->write_io);
>>> +       io_destroy(chrc->notify_io);
>>>
>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>> @@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attrib=
ute *attrib,
>>>                 goto done;
>>>
>>>         if (ccc_cb->callback)
>>> -               ecode =3D ccc_cb->callback(get_le16(value), ccc_cb->use=
r_data);
>>> +               ecode =3D ccc_cb->callback(att, get_le16(value),
>>> +                                               ccc_cb->user_data);
>>>
>>>         if (!ecode) {
>>>                 ccc->value[0] =3D value[0];
>>> @@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *user_=
data)
>>>
>>>         if (io =3D=3D chrc->write_io)
>>>                 chrc->write_io =3D NULL;
>>> +       else
>>> +               chrc->notify_io =3D NULL;
>>>
>>>         io_destroy(io);
>>>
>>>         return false;
>>>  }
>>>
>>> +static bool pipe_io_read(struct io *io, void *user_data)
>>> +{
>>> +       struct external_chrc *chrc =3D user_data;
>>> +       uint8_t buf[512];
>>> +       int fd =3D io_get_fd(io);
>>> +       ssize_t bytes_read;
>>> +
>>> +       bytes_read =3D read(fd, buf, sizeof(buf));
>>> +       if (bytes_read < 0)
>>> +               return false;
>>> +
>>> +       send_notification_to_devices(chrc->service->app->database,
>>> +                               gatt_db_attribute_get_handle(chrc->attr=
ib),
>>> +                               buf, bytes_read,
>>> +                               gatt_db_attribute_get_handle(chrc->ccc)=
,
>>> +                               false, NULL);
>>> +
>>> +       return true;
>>> +}
>>> +
>>>  static struct io *pipe_io_new(int fd, void *user_data)
>>>  {
>>>         struct io *io;
>>> @@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *user_=
data)
>>>
>>>         io_set_close_on_destroy(io, true);
>>>
>>> +       io_set_read_handler(io, pipe_io_read, user_data, NULL);
>>> +
>>>         io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>
>>>         return io;
>>> @@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct e=
xternal_chrc *chrc,
>>>         return NULL;
>>>  }
>>>
>>> -static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>> +static void acquire_notify_reply(DBusMessage *message, void *user_data=
)
>>>  {
>>>         struct external_chrc *chrc =3D user_data;
>>> +       DBusError err;
>>> +       int fd;
>>> +       uint16_t mtu;
>>> +
>>> +       dbus_error_init(&err);
>>> +
>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>>> +               error("Failed to acquire notify: %s\n", err.name);
>>> +               dbus_error_free(&err);
>>> +               goto retry;
>>> +       }
>>> +
>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &f=
d,
>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>> +                                       DBUS_TYPE_INVALID) =3D=3D false=
)) {
>>> +               error("Invalid AcquirNotify response\n");
>>> +               goto retry;
>>> +       }
>>> +
>>> +       DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);
>>> +
>>> +       chrc->notify_io =3D pipe_io_new(fd, chrc);
>>
>> Why do we need to use pipe and notify_io when implementing this
>> feature? I think fd and mtu has been pass using dbus message. Could
>> you clarify more details?
>
> notify_io is the receiving end of the notification, the application
> will write its notifications using the fd which is captured by
> checking if the fd is readable.
>
>>
>>> +
>>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>> +
>>> +       return;
>>> +
>>> +retry:
>>> +       g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NULL=
,
>>> +                                                       NULL, NULL);
>>> +
>>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>> +}
>>> +
>>> +static void acquire_notify_setup(DBusMessageIter *iter, void *user_dat=
a)
>>> +{
>>> +       DBusMessageIter dict;
>>> +       struct external_chrc *chrc =3D user_data;
>>> +
>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_S=
TRING
>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_STR=
ING,
>>> +                                       &dict);
>>> +
>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu);
>>> +
>>> +       dbus_message_iter_close_container(iter, &dict);
>>> +}
>>> +
>>> +static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void *=
user_data)
>>> +{
>>> +       struct external_chrc *chrc =3D user_data;
>>> +       DBusMessageIter iter;
>>>
>>>         DBG("External CCC write received with value: 0x%04x", value);
>>>
>>> @@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, void=
 *user_data)
>>>                 if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1))
>>>                         return 0;
>>>
>>> +               if (chrc->notify_io) {
>>> +                       io_destroy(chrc->notify_io);
>>> +                       chrc->notify_io =3D NULL;
>>> +                       return 0;
>>> +               }
>>> +
>>>                 /*
>>>                  * Send request to stop notifying. This is best-effort
>>>                  * operation, so simply ignore the return the value.
>>> @@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, voi=
d *user_data)
>>>                 (value =3D=3D 2 && !(chrc->props & BT_GATT_CHRC_PROP_IN=
DICATE)))
>>>                 return BT_ERROR_CCC_IMPROPERLY_CONFIGURED;
>>>
>>> +       if (chrc->notify_io) {
>>> +               __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>> +               return 0;
>>> +       }
>> What is the purpose to call __sync_fetch_and_add when there is
>> charc->notify_io? What is the scenario here=EF=BC=9F
>
> This is using the number of subscribed client as a refcount so when
> there is no client left we can drop the notify_io as well.
>
>>
>>> +
>>> +       chrc->mtu =3D bt_att_get_mtu(att);
>>> +
>>> +       /* Make use of AcquireNotify if supported */
>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", &i=
ter)) {
>>> +               if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNotif=
y",
>>> +                                               acquire_notify_setup,
>>> +                                               acquire_notify_reply,
>>> +                                               chrc, NULL))
>>> +                       return 0;
>>> +       }
>> When ble central subscribe this characteristic,  if NotifyAcquired is
>> set in ble peripheral application, then it return, but how can we
>> subscribe this characteristic? May we call "AcquireNotify" when ble
>> central subscribe this characteristic or enable notification?
>
> If NotifyAcquired is supported it will call AcquireNotify, or are you
> saying you tried that? The bluetoothctl has code to utilize this
> feature, you just have to register a characteristic with notify flag.
>
In previous email, I have confirmed AcquireWrite is working, now I
wanna send indication via fd using AcquireNotify in application, I
have removed "check" for "notify" in
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n1387
Then I try to use  pipe_io_send to send indication in application, but
it seems data cannot be sent out from peripheral to central, any idea?
it seems there is no reference code to send data in bluetoothctl.

static int pipe_io_send(struct io *io, const void *data, size_t len)
{
struct iovec iov;

iov.iov_base =3D (void *) data;
iov.iov_len =3D len;

return io_send(io, &iov, 1);
}

>>> +
>>>         /*
>>>          * Always call StartNotify for an incoming enable and ignore th=
e return
>>>          * value for now.
>>>          */
>>> -       if (g_dbus_proxy_method_call(chrc->proxy,
>>> -                                               "StartNotify", NULL, NU=
LL,
>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, =
NULL,
>>>                                                 NULL, NULL) =3D=3D FALS=
E)
>>>                 return BT_ATT_ERROR_UNLIKELY;
>>>
>>> --
>>> 2.13.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetoo=
th" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz

thansk
best wishes
yunhan

Re: [PATCH v2 08/10] gatt: Implement AcquireNotify for server

[Luiz Augusto von Dentz]

Hi Yunhan,

On Tue, Oct 3, 2017 at 9:13 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
> Hi, Luiz
>
> On Fri, Sep 29, 2017 at 12:03 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Yunhan,
>>
>> On Fri, Sep 29, 2017 at 7:47 AM, Yunhan Wang <yunhanw@nestlabs.com> wrot=
e:
>>> Hi, Luiz
>>>
>>> May I have several questions about this feature?
>>>
>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>
>>>> This enables IO via file descriptors using AcquireWrite if server
>>>> implements it.
>>>> ---
>>>>  src/gatt-database.c | 116 +++++++++++++++++++++++++++++++++++++++++++=
++++++---
>>>>  1 file changed, 111 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>>> index 239fe5384..4932568dd 100644
>>>> --- a/src/gatt-database.c
>>>> +++ b/src/gatt-database.c
>>>> @@ -24,6 +24,7 @@
>>>>  #include <stdint.h>
>>>>  #include <stdlib.h>
>>>>  #include <errno.h>
>>>> +#include <unistd.h>
>>>>
>>>>  #include "lib/bluetooth.h"
>>>>  #include "lib/sdp.h"
>>>> @@ -118,7 +119,9 @@ struct external_chrc {
>>>>         uint8_t props;
>>>>         uint8_t ext_props;
>>>>         uint32_t perm;
>>>> +       uint16_t mtu;
>>>>         struct io *write_io;
>>>> +       struct io *notify_io;
>>>>         struct gatt_db_attribute *attrib;
>>>>         struct gatt_db_attribute *ccc;
>>>>         struct queue *pending_reads;
>>>> @@ -152,7 +155,8 @@ struct device_state {
>>>>         struct queue *ccc_states;
>>>>  };
>>>>
>>>> -typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value,
>>>> +typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att,
>>>> +                                                       uint16_t value=
,
>>>>                                                         void *user_dat=
a);
>>>>  typedef void (*btd_gatt_database_destroy_t) (void *data);
>>>>
>>>> @@ -328,6 +332,7 @@ static void chrc_free(void *data)
>>>>         struct external_chrc *chrc =3D data;
>>>>
>>>>         io_destroy(chrc->write_io);
>>>> +       io_destroy(chrc->notify_io);
>>>>
>>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>> @@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attri=
bute *attrib,
>>>>                 goto done;
>>>>
>>>>         if (ccc_cb->callback)
>>>> -               ecode =3D ccc_cb->callback(get_le16(value), ccc_cb->us=
er_data);
>>>> +               ecode =3D ccc_cb->callback(att, get_le16(value),
>>>> +                                               ccc_cb->user_data);
>>>>
>>>>         if (!ecode) {
>>>>                 ccc->value[0] =3D value[0];
>>>> @@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *user=
_data)
>>>>
>>>>         if (io =3D=3D chrc->write_io)
>>>>                 chrc->write_io =3D NULL;
>>>> +       else
>>>> +               chrc->notify_io =3D NULL;
>>>>
>>>>         io_destroy(io);
>>>>
>>>>         return false;
>>>>  }
>>>>
>>>> +static bool pipe_io_read(struct io *io, void *user_data)
>>>> +{
>>>> +       struct external_chrc *chrc =3D user_data;
>>>> +       uint8_t buf[512];
>>>> +       int fd =3D io_get_fd(io);
>>>> +       ssize_t bytes_read;
>>>> +
>>>> +       bytes_read =3D read(fd, buf, sizeof(buf));
>>>> +       if (bytes_read < 0)
>>>> +               return false;
>>>> +
>>>> +       send_notification_to_devices(chrc->service->app->database,
>>>> +                               gatt_db_attribute_get_handle(chrc->att=
rib),
>>>> +                               buf, bytes_read,
>>>> +                               gatt_db_attribute_get_handle(chrc->ccc=
),
>>>> +                               false, NULL);
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>>  static struct io *pipe_io_new(int fd, void *user_data)
>>>>  {
>>>>         struct io *io;
>>>> @@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *user=
_data)
>>>>
>>>>         io_set_close_on_destroy(io, true);
>>>>
>>>> +       io_set_read_handler(io, pipe_io_read, user_data, NULL);
>>>> +
>>>>         io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>>
>>>>         return io;
>>>> @@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct =
external_chrc *chrc,
>>>>         return NULL;
>>>>  }
>>>>
>>>> -static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>> +static void acquire_notify_reply(DBusMessage *message, void *user_dat=
a)
>>>>  {
>>>>         struct external_chrc *chrc =3D user_data;
>>>> +       DBusError err;
>>>> +       int fd;
>>>> +       uint16_t mtu;
>>>> +
>>>> +       dbus_error_init(&err);
>>>> +
>>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>>>> +               error("Failed to acquire notify: %s\n", err.name);
>>>> +               dbus_error_free(&err);
>>>> +               goto retry;
>>>> +       }
>>>> +
>>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, &=
fd,
>>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>>> +                                       DBUS_TYPE_INVALID) =3D=3D fals=
e)) {
>>>> +               error("Invalid AcquirNotify response\n");
>>>> +               goto retry;
>>>> +       }
>>>> +
>>>> +       DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);
>>>> +
>>>> +       chrc->notify_io =3D pipe_io_new(fd, chrc);
>>>
>>> Why do we need to use pipe and notify_io when implementing this
>>> feature? I think fd and mtu has been pass using dbus message. Could
>>> you clarify more details?
>>
>> notify_io is the receiving end of the notification, the application
>> will write its notifications using the fd which is captured by
>> checking if the fd is readable.
>>
>>>
>>>> +
>>>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>>> +
>>>> +       return;
>>>> +
>>>> +retry:
>>>> +       g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NUL=
L,
>>>> +                                                       NULL, NULL);
>>>> +
>>>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>>> +}
>>>> +
>>>> +static void acquire_notify_setup(DBusMessageIter *iter, void *user_da=
ta)
>>>> +{
>>>> +       DBusMessageIter dict;
>>>> +       struct external_chrc *chrc =3D user_data;
>>>> +
>>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS_=
STRING
>>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_ST=
RING,
>>>> +                                       &dict);
>>>> +
>>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu);
>>>> +
>>>> +       dbus_message_iter_close_container(iter, &dict);
>>>> +}
>>>> +
>>>> +static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void =
*user_data)
>>>> +{
>>>> +       struct external_chrc *chrc =3D user_data;
>>>> +       DBusMessageIter iter;
>>>>
>>>>         DBG("External CCC write received with value: 0x%04x", value);
>>>>
>>>> @@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, voi=
d *user_data)
>>>>                 if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1))
>>>>                         return 0;
>>>>
>>>> +               if (chrc->notify_io) {
>>>> +                       io_destroy(chrc->notify_io);
>>>> +                       chrc->notify_io =3D NULL;
>>>> +                       return 0;
>>>> +               }
>>>> +
>>>>                 /*
>>>>                  * Send request to stop notifying. This is best-effort
>>>>                  * operation, so simply ignore the return the value.
>>>> @@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, vo=
id *user_data)
>>>>                 (value =3D=3D 2 && !(chrc->props & BT_GATT_CHRC_PROP_I=
NDICATE)))
>>>>                 return BT_ERROR_CCC_IMPROPERLY_CONFIGURED;
>>>>
>>>> +       if (chrc->notify_io) {
>>>> +               __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>>> +               return 0;
>>>> +       }
>>> What is the purpose to call __sync_fetch_and_add when there is
>>> charc->notify_io? What is the scenario here=EF=BC=9F
>>
>> This is using the number of subscribed client as a refcount so when
>> there is no client left we can drop the notify_io as well.
>>
>>>
>>>> +
>>>> +       chrc->mtu =3D bt_att_get_mtu(att);
>>>> +
>>>> +       /* Make use of AcquireNotify if supported */
>>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", &=
iter)) {
>>>> +               if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNoti=
fy",
>>>> +                                               acquire_notify_setup,
>>>> +                                               acquire_notify_reply,
>>>> +                                               chrc, NULL))
>>>> +                       return 0;
>>>> +       }
>>> When ble central subscribe this characteristic,  if NotifyAcquired is
>>> set in ble peripheral application, then it return, but how can we
>>> subscribe this characteristic? May we call "AcquireNotify" when ble
>>> central subscribe this characteristic or enable notification?
>>
>> If NotifyAcquired is supported it will call AcquireNotify, or are you
>> saying you tried that? The bluetoothctl has code to utilize this
>> feature, you just have to register a characteristic with notify flag.
>>
> In previous email, I have confirmed AcquireWrite is working, now I
> wanna send indication via fd using AcquireNotify in application, I
> have removed "check" for "notify" in
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n13=
87

Does this means you don't have the notify flag at all, or you are
using indications? I guess we should enable this for indications as
well, though it won't have any means to confirm.

> Then I try to use  pipe_io_send to send indication in application, but
> it seems data cannot be sent out from peripheral to central, any idea?
> it seems there is no reference code to send data in bluetoothctl.
>
> static int pipe_io_send(struct io *io, const void *data, size_t len)
> {
> struct iovec iov;
>
> iov.iov_base =3D (void *) data;
> iov.iov_len =3D len;
>
> return io_send(io, &iov, 1);
> }

Isn't this because of the following check:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database.c=
#n906

Im afraid you have quite a different use case than we were targeting,
it seems you do you GATT for acking delivery instead of doing it
directly over the protocol which is what most solutions I know would
do when emulating serial over GATT.

>>>> +
>>>>         /*
>>>>          * Always call StartNotify for an incoming enable and ignore t=
he return
>>>>          * value for now.
>>>>          */
>>>> -       if (g_dbus_proxy_method_call(chrc->proxy,
>>>> -                                               "StartNotify", NULL, N=
ULL,
>>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL,=
 NULL,
>>>>                                                 NULL, NULL) =3D=3D FAL=
SE)
>>>>                 return BT_ATT_ERROR_UNLIKELY;
>>>>
>>>> --
>>>> 2.13.5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-blueto=
oth" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> thansk
> best wishes
> yunhan



--=20
Luiz Augusto von Dentz

Re: [PATCH v2 08/10] gatt: Implement AcquireNotify for server

[Yunhan Wang]

Hi, Luiz

On Tue, Oct 3, 2017 at 12:36 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunhan,
>
> On Tue, Oct 3, 2017 at 9:13 AM, Yunhan Wang <yunhanw@nestlabs.com> wrote:
>> Hi, Luiz
>>
>> On Fri, Sep 29, 2017 at 12:03 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Yunhan,
>>>
>>> On Fri, Sep 29, 2017 at 7:47 AM, Yunhan Wang <yunhanw@nestlabs.com> wro=
te:
>>>> Hi, Luiz
>>>>
>>>> May I have several questions about this feature?
>>>>
>>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>
>>>>> This enables IO via file descriptors using AcquireWrite if server
>>>>> implements it.
>>>>> ---
>>>>>  src/gatt-database.c | 116 ++++++++++++++++++++++++++++++++++++++++++=
+++++++---
>>>>>  1 file changed, 111 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>>>> index 239fe5384..4932568dd 100644
>>>>> --- a/src/gatt-database.c
>>>>> +++ b/src/gatt-database.c
>>>>> @@ -24,6 +24,7 @@
>>>>>  #include <stdint.h>
>>>>>  #include <stdlib.h>
>>>>>  #include <errno.h>
>>>>> +#include <unistd.h>
>>>>>
>>>>>  #include "lib/bluetooth.h"
>>>>>  #include "lib/sdp.h"
>>>>> @@ -118,7 +119,9 @@ struct external_chrc {
>>>>>         uint8_t props;
>>>>>         uint8_t ext_props;
>>>>>         uint32_t perm;
>>>>> +       uint16_t mtu;
>>>>>         struct io *write_io;
>>>>> +       struct io *notify_io;
>>>>>         struct gatt_db_attribute *attrib;
>>>>>         struct gatt_db_attribute *ccc;
>>>>>         struct queue *pending_reads;
>>>>> @@ -152,7 +155,8 @@ struct device_state {
>>>>>         struct queue *ccc_states;
>>>>>  };
>>>>>
>>>>> -typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value,
>>>>> +typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att=
,
>>>>> +                                                       uint16_t valu=
e,
>>>>>                                                         void *user_da=
ta);
>>>>>  typedef void (*btd_gatt_database_destroy_t) (void *data);
>>>>>
>>>>> @@ -328,6 +332,7 @@ static void chrc_free(void *data)
>>>>>         struct external_chrc *chrc =3D data;
>>>>>
>>>>>         io_destroy(chrc->write_io);
>>>>> +       io_destroy(chrc->notify_io);
>>>>>
>>>>>         queue_destroy(chrc->pending_reads, cancel_pending_read);
>>>>>         queue_destroy(chrc->pending_writes, cancel_pending_write);
>>>>> @@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attr=
ibute *attrib,
>>>>>                 goto done;
>>>>>
>>>>>         if (ccc_cb->callback)
>>>>> -               ecode =3D ccc_cb->callback(get_le16(value), ccc_cb->u=
ser_data);
>>>>> +               ecode =3D ccc_cb->callback(att, get_le16(value),
>>>>> +                                               ccc_cb->user_data);
>>>>>
>>>>>         if (!ecode) {
>>>>>                 ccc->value[0] =3D value[0];
>>>>> @@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *use=
r_data)
>>>>>
>>>>>         if (io =3D=3D chrc->write_io)
>>>>>                 chrc->write_io =3D NULL;
>>>>> +       else
>>>>> +               chrc->notify_io =3D NULL;
>>>>>
>>>>>         io_destroy(io);
>>>>>
>>>>>         return false;
>>>>>  }
>>>>>
>>>>> +static bool pipe_io_read(struct io *io, void *user_data)
>>>>> +{
>>>>> +       struct external_chrc *chrc =3D user_data;
>>>>> +       uint8_t buf[512];
>>>>> +       int fd =3D io_get_fd(io);
>>>>> +       ssize_t bytes_read;
>>>>> +
>>>>> +       bytes_read =3D read(fd, buf, sizeof(buf));
>>>>> +       if (bytes_read < 0)
>>>>> +               return false;
>>>>> +
>>>>> +       send_notification_to_devices(chrc->service->app->database,
>>>>> +                               gatt_db_attribute_get_handle(chrc->at=
trib),
>>>>> +                               buf, bytes_read,
>>>>> +                               gatt_db_attribute_get_handle(chrc->cc=
c),
>>>>> +                               false, NULL);
>>>>> +
>>>>> +       return true;
>>>>> +}
>>>>> +
>>>>>  static struct io *pipe_io_new(int fd, void *user_data)
>>>>>  {
>>>>>         struct io *io;
>>>>> @@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *use=
r_data)
>>>>>
>>>>>         io_set_close_on_destroy(io, true);
>>>>>
>>>>> +       io_set_read_handler(io, pipe_io_read, user_data, NULL);
>>>>> +
>>>>>         io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
>>>>>
>>>>>         return io;
>>>>> @@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct=
 external_chrc *chrc,
>>>>>         return NULL;
>>>>>  }
>>>>>
>>>>> -static uint8_t ccc_write_cb(uint16_t value, void *user_data)
>>>>> +static void acquire_notify_reply(DBusMessage *message, void *user_da=
ta)
>>>>>  {
>>>>>         struct external_chrc *chrc =3D user_data;
>>>>> +       DBusError err;
>>>>> +       int fd;
>>>>> +       uint16_t mtu;
>>>>> +
>>>>> +       dbus_error_init(&err);
>>>>> +
>>>>> +       if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) {
>>>>> +               error("Failed to acquire notify: %s\n", err.name);
>>>>> +               dbus_error_free(&err);
>>>>> +               goto retry;
>>>>> +       }
>>>>> +
>>>>> +       if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, =
&fd,
>>>>> +                                       DBUS_TYPE_UINT16, &mtu,
>>>>> +                                       DBUS_TYPE_INVALID) =3D=3D fal=
se)) {
>>>>> +               error("Invalid AcquirNotify response\n");
>>>>> +               goto retry;
>>>>> +       }
>>>>> +
>>>>> +       DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);
>>>>> +
>>>>> +       chrc->notify_io =3D pipe_io_new(fd, chrc);
>>>>
>>>> Why do we need to use pipe and notify_io when implementing this
>>>> feature? I think fd and mtu has been pass using dbus message. Could
>>>> you clarify more details?
>>>
>>> notify_io is the receiving end of the notification, the application
>>> will write its notifications using the fd which is captured by
>>> checking if the fd is readable.
>>>
>>>>
>>>>> +
>>>>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>>>> +
>>>>> +       return;
>>>>> +
>>>>> +retry:
>>>>> +       g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NU=
LL,
>>>>> +                                                       NULL, NULL);
>>>>> +
>>>>> +       __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>>>> +}
>>>>> +
>>>>> +static void acquire_notify_setup(DBusMessageIter *iter, void *user_d=
ata)
>>>>> +{
>>>>> +       DBusMessageIter dict;
>>>>> +       struct external_chrc *chrc =3D user_data;
>>>>> +
>>>>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
>>>>> +                                       DBUS_DICT_ENTRY_BEGIN_CHAR_AS=
_STRING
>>>>> +                                       DBUS_TYPE_STRING_AS_STRING
>>>>> +                                       DBUS_TYPE_VARIANT_AS_STRING
>>>>> +                                       DBUS_DICT_ENTRY_END_CHAR_AS_S=
TRING,
>>>>> +                                       &dict);
>>>>> +
>>>>> +       dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu)=
;
>>>>> +
>>>>> +       dbus_message_iter_close_container(iter, &dict);
>>>>> +}
>>>>> +
>>>>> +static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void=
 *user_data)
>>>>> +{
>>>>> +       struct external_chrc *chrc =3D user_data;
>>>>> +       DBusMessageIter iter;
>>>>>
>>>>>         DBG("External CCC write received with value: 0x%04x", value);
>>>>>
>>>>> @@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, vo=
id *user_data)
>>>>>                 if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1))
>>>>>                         return 0;
>>>>>
>>>>> +               if (chrc->notify_io) {
>>>>> +                       io_destroy(chrc->notify_io);
>>>>> +                       chrc->notify_io =3D NULL;
>>>>> +                       return 0;
>>>>> +               }
>>>>> +
>>>>>                 /*
>>>>>                  * Send request to stop notifying. This is best-effor=
t
>>>>>                  * operation, so simply ignore the return the value.
>>>>> @@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, v=
oid *user_data)
>>>>>                 (value =3D=3D 2 && !(chrc->props & BT_GATT_CHRC_PROP_=
INDICATE)))
>>>>>                 return BT_ERROR_CCC_IMPROPERLY_CONFIGURED;
>>>>>
>>>>> +       if (chrc->notify_io) {
>>>>> +               __sync_fetch_and_add(&chrc->ntfy_cnt, 1);
>>>>> +               return 0;
>>>>> +       }
>>>> What is the purpose to call __sync_fetch_and_add when there is
>>>> charc->notify_io? What is the scenario here=EF=BC=9F
>>>
>>> This is using the number of subscribed client as a refcount so when
>>> there is no client left we can drop the notify_io as well.
>>>
>>>>
>>>>> +
>>>>> +       chrc->mtu =3D bt_att_get_mtu(att);
>>>>> +
>>>>> +       /* Make use of AcquireNotify if supported */
>>>>> +       if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", =
&iter)) {
>>>>> +               if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNot=
ify",
>>>>> +                                               acquire_notify_setup,
>>>>> +                                               acquire_notify_reply,
>>>>> +                                               chrc, NULL))
>>>>> +                       return 0;
>>>>> +       }
>>>> When ble central subscribe this characteristic,  if NotifyAcquired is
>>>> set in ble peripheral application, then it return, but how can we
>>>> subscribe this characteristic? May we call "AcquireNotify" when ble
>>>> central subscribe this characteristic or enable notification?
>>>
>>> If NotifyAcquired is supported it will call AcquireNotify, or are you
>>> saying you tried that? The bluetoothctl has code to utilize this
>>> feature, you just have to register a characteristic with notify flag.
>>>
>> In previous email, I have confirmed AcquireWrite is working, now I
>> wanna send indication via fd using AcquireNotify in application, I
>> have removed "check" for "notify" in
>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n1=
387
>
> Does this means you don't have the notify flag at all, or you are
> using indications? I guess we should enable this for indications as
> well, though it won't have any means to confirm.
>
I am using indicate flag that expects the confirmation. In regular
indication operation, the indication confirmation would be received in
server side per the code,
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database.c=
#n935.
I just send a follow-up patch to add proxy in
send_notification_to_devices call that fix indication confirmation
using pipe_io_read. It is working now. Thank you

>> Then I try to use  pipe_io_send to send indication in application, but
>> it seems data cannot be sent out from peripheral to central, any idea?
>> it seems there is no reference code to send data in bluetoothctl.
>>
>> static int pipe_io_send(struct io *io, const void *data, size_t len)
>> {
>> struct iovec iov;
>>
>> iov.iov_base =3D (void *) data;
>> iov.iov_len =3D len;
>>
>> return io_send(io, &iov, 1);
>> }
>
> Isn't this because of the following check:
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database=
.c#n906
>
> Im afraid you have quite a different use case than we were targeting,
> it seems you do you GATT for acking delivery instead of doing it
> directly over the protocol which is what most solutions I know would
> do when emulating serial over GATT.
>

>>>>> +
>>>>>         /*
>>>>>          * Always call StartNotify for an incoming enable and ignore =
the return
>>>>>          * value for now.
>>>>>          */
>>>>> -       if (g_dbus_proxy_method_call(chrc->proxy,
>>>>> -                                               "StartNotify", NULL, =
NULL,
>>>>> +       if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL=
, NULL,
>>>>>                                                 NULL, NULL) =3D=3D FA=
LSE)
>>>>>                 return BT_ATT_ERROR_UNLIKELY;
>>>>>
>>>>> --
>>>>> 2.13.5
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluet=
ooth" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> thansk
>> best wishes
>> yunhan
>
>
>
> --
> Luiz Augusto von Dentz


Thanks
Best wishes
Yunhan