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

From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: 2017-09-29 07:22:54

Hi Yunhan,

On Fri, Sep 29, 2017 at 10:14 AM, Yunhan Wang [off-list ref] 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
[off-list ref] wrote:

quoted

Hi Yunhan,

On Fri, Sep 29, 2017 at 8:04 AM, Yunhan Wang [off-list ref] wrot=

e:

quoted

Hi, Luiz

May I ask several questions about acquire write implementation?

On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz
[off-list ref] wrote:

quoted

From: Luiz Augusto von Dentz <redacted>

This enables IO via file descriptors using AcquireWrite if server
implements it.
---
 src/gatt-database.c | 142 +++++++++++++++++++++++++++++++++++++++++++=

+++++++++

quoted

 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,

quoted

        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=

quoted

+{
+       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,

quoted

+                                       DBUS_TYPE_UINT16, &mtu,
+                                       DBUS_TYPE_INVALID) =3D=3D fals=

e)) {

quoted

+               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=

);

quoted

+}
+
+static void acquire_write_setup(DBusMessageIter *iter, void *user_dat=

a)

quoted

+{
+       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

quoted

+                                       DBUS_TYPE_STRING_AS_STRING
+                                       DBUS_TYPE_VARIANT_AS_STRING
+                                       DBUS_DICT_ENTRY_END_CHAR_AS_ST=

RING,

quoted

+                                       &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,

quoted

+                                       unsigned int id,
+                                       const uint8_t *value, size_t l=

en)

quoted

+{
+       struct pending_op *op;
+
+       op =3D pending_write_new(device, NULL, attrib, id, value, len,=

0);

quoted

+
+       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,

quoted

        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"=

);

quoted

@@ -2102,6 +2229,21 @@ static void chrc_write_cb(struct gatt_db_attrib=

ute *attrib,

quoted

                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.

quoted

+       if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &i=

ter)) {

quoted

+               if (acquire_write(chrc, device, attrib, id, value, len=

))

quoted

+                       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

quoted

The below is earlier draft idea for this MTU acquisition in server side=

quoted

https://www.spinics.net/lists/linux-bluetooth/msg71961.html

quoted

        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

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help