[PATCH BlueZ v2 1/4] client: Add missing newline character to shell printfs

[PATCH BlueZ v2 1/4] client: Add missing newline character to shell printfs

[Grzegorz Kolodziejczyk]

This patch adds missing newline characters to shell printfs. It fixes
printig issues.
---
 client/gatt.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 7103c4f83..52a999dc9 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -627,7 +627,8 @@ static void write_attribute(GDBusProxy *proxy, char *arg)
 		bt_shell_printf("Attempting to write fd %d\n",
 						io_get_fd(write_io.io));
 		if (io_send(write_io.io, &iov, 1) < 0) {
-			bt_shell_printf("Failed to write: %s", strerror(errno));
+			bt_shell_printf("Failed to write: %s\n",
+							strerror(errno));
 			return bt_shell_noninteractive_quit(EXIT_FAILURE);
 		}
 		return;
@@ -1622,7 +1623,7 @@ static void authorize_write_response(const char *input, void *user_data)
 		goto error;
 	}
 
-	bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , chrc->path);
+	bt_shell_printf("[" COLORED_CHG "] Attribute %s written\n", chrc->path);
 
 	g_dbus_emit_property_changed(aad->conn, chrc->path, CHRC_INTERFACE,
 								"Value");
@@ -1671,7 +1672,7 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
 					"org.bluez.Error.InvalidArguments",
 					NULL);
 
-	bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , chrc->path);
+	bt_shell_printf("[" COLORED_CHG "] Attribute %s written\n", chrc->path);
 
 	g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE, "Value");
 
@@ -1789,8 +1790,8 @@ static DBusMessage *chrc_start_notify(DBusConnection *conn, DBusMessage *msg,
 		return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 
 	chrc->notifying = true;
-	bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications enabled",
-							chrc->path);
+	bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications "
+						"enabled\n", chrc->path);
 	g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
 							"Notifying");
 
@@ -1806,8 +1807,8 @@ static DBusMessage *chrc_stop_notify(DBusConnection *conn, DBusMessage *msg,
 		return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 
 	chrc->notifying = false;
-	bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications disabled",
-							chrc->path);
+	bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications "
+						"disabled\n", chrc->path);
 	g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
 							"Notifying");
 
@@ -1819,7 +1820,8 @@ static DBusMessage *chrc_confirm(DBusConnection *conn, DBusMessage *msg,
 {
 	struct chrc *chrc = user_data;
 
-	bt_shell_printf("Attribute %s indication confirm received", chrc->path);
+	bt_shell_printf("Attribute %s indication confirm received\n",
+								chrc->path);
 
 	return dbus_message_new_method_return(msg);
 }
@@ -2012,7 +2014,7 @@ static DBusMessage *desc_write_value(DBusConnection *conn, DBusMessage *msg,
 	bt_shell_printf("WriteValue: %s offset %u link %s\n",
 			path_to_address(device), offset, link);
 
-	bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , desc->path);
+	bt_shell_printf("[" COLORED_CHG "] Attribute %s written\n" , desc->path);
 
 	g_dbus_emit_property_changed(conn, desc->path, CHRC_INTERFACE, "Value");
 
-- 
2.13.6

[PATCH BlueZ v2 2/4] tools/hcitool: Change connection handle condition for lecup

[Grzegorz Kolodziejczyk]

According to BLUETOOTH SPECIFICATION Version 5.0 | Vol 2, Part E
7.8.18 LE Connection Update Command, connection handle range is
0x0000-0x0EFF.
---
 tools/hcitool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hcitool.c b/tools/hcitool.c
index 02c4ebe1b..945f675b0 100644
--- a/tools/hcitool.c
+++ b/tools/hcitool.c
@@ -3351,7 +3351,7 @@ static void cmd_lecup(int dev_id, int argc, char **argv)
 		timeout = strtoul(argv[4], NULL, 0);
 	}
 
-	if (handle == 0) {
+	if (handle > 0x0EFF) {
 		printf("%s", lecup_help);
 		return;
 	}
-- 
2.13.6

[PATCH BlueZ v2 3/4] client: Fix writing attribute values

[Grzegorz Kolodziejczyk]

Attribute values is not copied with dbus_message_iter_get_fixed_array,
so gatt write callback needs to replace old value with reallocation and
copy.
---
 client/gatt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/client/gatt.c b/client/gatt.c
index 52a999dc9..102c11437 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -1589,12 +1589,18 @@ static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
 static int parse_value_arg(DBusMessageIter *iter, uint8_t **value, int *len)
 {
 	DBusMessageIter array;
+	uint8_t *read_value;
+	int read_len;
 
 	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
 		return -EINVAL;
 
 	dbus_message_iter_recurse(iter, &array);
-	dbus_message_iter_get_fixed_array(&array, value, len);
+	dbus_message_iter_get_fixed_array(&array, &read_value, &read_len);
+
+	g_free(*value);
+	*value = g_memdup(read_value, read_len);
+	*len = read_len;
 
 	return 0;
 }
-- 
2.13.6

[PATCH BlueZ v2 4/4] shared/gatt-server: Fix prepare write queuing

[Grzegorz Kolodziejczyk]

Multiple prepare writes may be done for multiple attributes. Queue
mechanism must be aware of handle under which preparation is done.
---
 src/shared/gatt-server.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 4b554f665..d1efa83a1 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1190,6 +1190,14 @@ static bool prep_data_new(struct bt_gatt_server *server,
 	return true;
 }
 
+static bool match_prep_attr_handle(const void *data, const void *match_data)
+{
+	const struct prep_write_data *prep_data = data;
+	const uint16_t *match_handle = match_data;
+
+	return prep_data->handle == *match_handle;
+}
+
 static bool store_prep_data(struct bt_gatt_server *server,
 					uint16_t handle, uint16_t offset,
 					uint16_t length, uint8_t *value)
@@ -1200,9 +1208,10 @@ static bool store_prep_data(struct bt_gatt_server *server,
 	 * Now lets check if prep write is a continuation of long write
 	 * If so do aggregation of data
 	 */
-	prep_data = queue_peek_tail(server->prep_queue);
-	if (prep_data && (prep_data->handle == handle) &&
-			(offset == (prep_data->length + prep_data->offset)))
+	prep_data = queue_find(server->prep_queue, match_prep_attr_handle,
+								&handle);
+
+	if (prep_data && (offset == (prep_data->length + prep_data->offset)))
 		return append_prep_data(prep_data, handle, length, value);
 
 	return prep_data_new(server, handle, offset, length, value);
-- 
2.13.6

Re: [PATCH BlueZ v2 4/4] shared/gatt-server: Fix prepare write queuing

[Luiz Augusto von Dentz]

Hi Grzegorz,

On Thu, Apr 19, 2018 at 5:03 PM, Grzegorz Kolodziejczyk
<grzegorz.kolodziejczyk@codecoup.pl> wrote:
> Multiple prepare writes may be done for multiple attributes. Queue
> mechanism must be aware of handle under which preparation is done.
> ---
>  src/shared/gatt-server.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 4b554f665..d1efa83a1 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1190,6 +1190,14 @@ static bool prep_data_new(struct bt_gatt_server *server,
>         return true;
>  }
>
> +static bool match_prep_attr_handle(const void *data, const void *match_data)
> +{
> +       const struct prep_write_data *prep_data = data;
> +       const uint16_t *match_handle = match_data;
> +
> +       return prep_data->handle == *match_handle;
> +}
> +
>  static bool store_prep_data(struct bt_gatt_server *server,
>                                         uint16_t handle, uint16_t offset,
>                                         uint16_t length, uint8_t *value)
> @@ -1200,9 +1208,10 @@ static bool store_prep_data(struct bt_gatt_server *server,
>          * Now lets check if prep write is a continuation of long write
>          * If so do aggregation of data
>          */
> -       prep_data = queue_peek_tail(server->prep_queue);
> -       if (prep_data && (prep_data->handle == handle) &&
> -                       (offset == (prep_data->length + prep_data->offset)))
> +       prep_data = queue_find(server->prep_queue, match_prep_attr_handle,
> +                                                               &handle);

If I got this right it is not that the code is broken, it just don't
group together the writes if there are other handles in the prepare
queue. Afaik that was done because the order we write the handles may
matter, so it is up to the remote stack to make the order proper if it
wants long writes to all be executed in sequence, or interleaved which
means we would have to store new chunks in the order they are
prepared.

> +       if (prep_data && (offset == (prep_data->length + prep_data->offset)))
>                 return append_prep_data(prep_data, handle, length, value);
>
>         return prep_data_new(server, handle, offset, length, value);
> --
> 2.13.6
>
> --
> 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 BlueZ v2 1/4] client: Add missing newline character to shell printfs

[Luiz Augusto von Dentz]

Hi Grzegorz,

On Thu, Apr 19, 2018 at 5:03 PM, Grzegorz Kolodziejczyk
<grzegorz.kolodziejczyk@codecoup.pl> wrote:
> This patch adds missing newline characters to shell printfs. It fixes
> printig issues.

We might be able to solve this sort of problem by introducing a macro,
e.g. bt_shell_msg, that adds the new line, for bt_shell_printf Id like
to keep as it is since there could be tools that need to break the
messages.

> ---
>  client/gatt.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/client/gatt.c b/client/gatt.c
> index 7103c4f83..52a999dc9 100644
> --- a/client/gatt.c
> +++ b/client/gatt.c
> @@ -627,7 +627,8 @@ static void write_attribute(GDBusProxy *proxy, char *arg)
>                 bt_shell_printf("Attempting to write fd %d\n",
>                                                 io_get_fd(write_io.io));
>                 if (io_send(write_io.io, &iov, 1) < 0) {
> -                       bt_shell_printf("Failed to write: %s", strerror(errno));
> +                       bt_shell_printf("Failed to write: %s\n",
> +                                                       strerror(errno));
>                         return bt_shell_noninteractive_quit(EXIT_FAILURE);
>                 }
>                 return;
> @@ -1622,7 +1623,7 @@ static void authorize_write_response(const char *input, void *user_data)
>                 goto error;
>         }
>
> -       bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , chrc->path);
> +       bt_shell_printf("[" COLORED_CHG "] Attribute %s written\n", chrc->path);
>
>         g_dbus_emit_property_changed(aad->conn, chrc->path, CHRC_INTERFACE,
>                                                                 "Value");
> @@ -1671,7 +1672,7 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
>                                         "org.bluez.Error.InvalidArguments",
>                                         NULL);
>
> -       bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , chrc->path);
> +       bt_shell_printf("[" COLORED_CHG "] Attribute %s written\n", chrc->path);
>
>         g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE, "Value");
>
> @@ -1789,8 +1790,8 @@ static DBusMessage *chrc_start_notify(DBusConnection *conn, DBusMessage *msg,
>                 return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
>
>         chrc->notifying = true;
> -       bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications enabled",
> -                                                       chrc->path);
> +       bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications "
> +                                               "enabled\n", chrc->path);
>         g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
>                                                         "Notifying");
>
> @@ -1806,8 +1807,8 @@ static DBusMessage *chrc_stop_notify(DBusConnection *conn, DBusMessage *msg,
>                 return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
>
>         chrc->notifying = false;
> -       bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications disabled",
> -                                                       chrc->path);
> +       bt_shell_printf("[" COLORED_CHG "] Attribute %s notifications "
> +                                               "disabled\n", chrc->path);
>         g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
>                                                         "Notifying");
>
> @@ -1819,7 +1820,8 @@ static DBusMessage *chrc_confirm(DBusConnection *conn, DBusMessage *msg,
>  {
>         struct chrc *chrc = user_data;
>
> -       bt_shell_printf("Attribute %s indication confirm received", chrc->path);
> +       bt_shell_printf("Attribute %s indication confirm received\n",
> +                                                               chrc->path);
>
>         return dbus_message_new_method_return(msg);
>  }
> @@ -2012,7 +2014,7 @@ static DBusMessage *desc_write_value(DBusConnection *conn, DBusMessage *msg,
>         bt_shell_printf("WriteValue: %s offset %u link %s\n",
>                         path_to_address(device), offset, link);
>
> -       bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , desc->path);
> +       bt_shell_printf("[" COLORED_CHG "] Attribute %s written\n" , desc->path);
>
>         g_dbus_emit_property_changed(conn, desc->path, CHRC_INTERFACE, "Value");
>
> --
> 2.13.6
>
> --
> 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 BlueZ v2 4/4] shared/gatt-server: Fix prepare write queuing

[Grzegorz Kołodziejczyk]

Hi Luiz,

I did this fix while testing it agains PTS test case "4.6.39
GATT/SR/GAW/BV-10-C [Nested Long Characteristic Value Reliable Writes
- to Server]"

Without this fix bluez behaves as described:
1) PTS do prepare write to CHARACTERISTIC1 with offset =3D 0 and value
with size =3D ATT_MTU-5.
2) Bluez do new prepare write for CHARACTERISTIC1
3) PTS do prepare write to CHARACTERISTIC2 with offset =3D 0 and value
with size =3D ATT_MTU-5.
4) Bluez do new prepare write for CHARACTERISTIC2
5) PTS do prepare write to CHARACTERISTIC1 with offset =3D ATT_MTU-5 and
value with size < ATT_MTU-5.
6) Bluez do new prepare write for CHARACTERISTIC1
7) PTS do prepare write to CHARACTERISTIC2 with offset =3D ATT_MTU-5 and
value with size < ATT_MTU-5.
8) Bluez do new prepare write for CHARACTERISTIC2

9) Execut write is issued with flag "Immediately write all pend-ing
prepared values".
10) PTS do read of those two CHARACTERISTICS and got response with
value from last prepare write to CHARACTERISTIC (this from second
prepare write form offset ATT_MTU-5) cause thos are 4 seperate prepare
writes queues (bluez appends only if last prepare write was to the
same handle).
11) Test result fails cause expected value isn't the same as read.

What's more, spec says"
"If a Characteristic Value is prepared two or more times during this sub-
procedure, then all prepared values are written to the same Characteristic
Value in the order that they were prepared." BLUETOOTH SPECIFICATION
Version 5.0 | Vol 3, Part G (4.9.5)

"Each client=E2=80=99s queued values are separate; the execution of one que=
ue shall not
affect the preparation or execution of any other client=E2=80=99s queued
values." BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part F (3.4.6.1)

Regards,
Grzegorz

2018-04-19 17:11 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
> Hi Grzegorz,
>
> On Thu, Apr 19, 2018 at 5:03 PM, Grzegorz Kolodziejczyk
> <grzegorz.kolodziejczyk@codecoup.pl> wrote:
>> Multiple prepare writes may be done for multiple attributes. Queue
>> mechanism must be aware of handle under which preparation is done.
>> ---
>>  src/shared/gatt-server.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 4b554f665..d1efa83a1 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -1190,6 +1190,14 @@ static bool prep_data_new(struct bt_gatt_server *=
server,
>>         return true;
>>  }
>>
>> +static bool match_prep_attr_handle(const void *data, const void *match_=
data)
>> +{
>> +       const struct prep_write_data *prep_data =3D data;
>> +       const uint16_t *match_handle =3D match_data;
>> +
>> +       return prep_data->handle =3D=3D *match_handle;
>> +}
>> +
>>  static bool store_prep_data(struct bt_gatt_server *server,
>>                                         uint16_t handle, uint16_t offset=
,
>>                                         uint16_t length, uint8_t *value)
>> @@ -1200,9 +1208,10 @@ static bool store_prep_data(struct bt_gatt_server=
 *server,
>>          * Now lets check if prep write is a continuation of long write
>>          * If so do aggregation of data
>>          */
>> -       prep_data =3D queue_peek_tail(server->prep_queue);
>> -       if (prep_data && (prep_data->handle =3D=3D handle) &&
>> -                       (offset =3D=3D (prep_data->length + prep_data->o=
ffset)))
>> +       prep_data =3D queue_find(server->prep_queue, match_prep_attr_han=
dle,
>> +                                                               &handle)=
;
>
> If I got this right it is not that the code is broken, it just don't
> group together the writes if there are other handles in the prepare
> queue. Afaik that was done because the order we write the handles may
> matter, so it is up to the remote stack to make the order proper if it
> wants long writes to all be executed in sequence, or interleaved which
> means we would have to store new chunks in the order they are
> prepared.
>
>> +       if (prep_data && (offset =3D=3D (prep_data->length + prep_data->=
offset)))
>>                 return append_prep_data(prep_data, handle, length, value=
);
>>
>>         return prep_data_new(server, handle, offset, length, value);
>> --
>> 2.13.6
>>
>> --
>> 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
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/4] shared/gatt-server: Fix prepare write queuing

[Luiz Augusto von Dentz]

Hi Grzegorz,

On Fri, Apr 20, 2018 at 11:11 AM, Grzegorz Ko=C5=82odziejczyk
<grzegorz.kolodziejczyk@codecoup.pl> wrote:
> Hi Luiz,
>
> I did this fix while testing it agains PTS test case "4.6.39
> GATT/SR/GAW/BV-10-C [Nested Long Characteristic Value Reliable Writes
> - to Server]"
>
> Without this fix bluez behaves as described:
> 1) PTS do prepare write to CHARACTERISTIC1 with offset =3D 0 and value
> with size =3D ATT_MTU-5.
> 2) Bluez do new prepare write for CHARACTERISTIC1
> 3) PTS do prepare write to CHARACTERISTIC2 with offset =3D 0 and value
> with size =3D ATT_MTU-5.
> 4) Bluez do new prepare write for CHARACTERISTIC2
> 5) PTS do prepare write to CHARACTERISTIC1 with offset =3D ATT_MTU-5 and
> value with size < ATT_MTU-5.
> 6) Bluez do new prepare write for CHARACTERISTIC1
> 7) PTS do prepare write to CHARACTERISTIC2 with offset =3D ATT_MTU-5 and
> value with size < ATT_MTU-5.
> 8) Bluez do new prepare write for CHARACTERISTIC2
>
> 9) Execut write is issued with flag "Immediately write all pend-ing
> prepared values".
> 10) PTS do read of those two CHARACTERISTICS and got response with
> value from last prepare write to CHARACTERISTIC (this from second
> prepare write form offset ATT_MTU-5) cause thos are 4 seperate prepare
> writes queues (bluez appends only if last prepare write was to the
> same handle).
> 11) Test result fails cause expected value isn't the same as read.


Have you checked what went wrong with the value written? Perhaps this
is broken on the client side, afaik you do have a fix for that as
well? I imagine when you changed to do the aggregation only one write
will be send, so perhaps the client is not really appeding data using
the offset. Also if the characteristic do support AcquireWrite then
this whole thing is guaranteed not to work because writing to the pipe
fd does not support setting an offset currently, so perhaps that is
the real issue. Fixing that shall be possible using sendmsg and
setting the offset in the msg_control then the client has to read that
back with recvmsg.

> What's more, spec says"
> "If a Characteristic Value is prepared two or more times during this sub-
> procedure, then all prepared values are written to the same Characteristi=
c
> Value in the order that they were prepared." BLUETOOTH SPECIFICATION
> Version 5.0 | Vol 3, Part G (4.9.5)
>
> "Each client=E2=80=99s queued values are separate; the execution of one q=
ueue shall not
> affect the preparation or execution of any other client=E2=80=99s queued
> values." BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part F (3.4.6.1)
>
> Regards,
> Grzegorz
>
> 2018-04-19 17:11 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
>> Hi Grzegorz,
>>
>> On Thu, Apr 19, 2018 at 5:03 PM, Grzegorz Kolodziejczyk
>> <grzegorz.kolodziejczyk@codecoup.pl> wrote:
>>> Multiple prepare writes may be done for multiple attributes. Queue
>>> mechanism must be aware of handle under which preparation is done.
>>> ---
>>>  src/shared/gatt-server.c | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>> index 4b554f665..d1efa83a1 100644
>>> --- a/src/shared/gatt-server.c
>>> +++ b/src/shared/gatt-server.c
>>> @@ -1190,6 +1190,14 @@ static bool prep_data_new(struct bt_gatt_server =
*server,
>>>         return true;
>>>  }
>>>
>>> +static bool match_prep_attr_handle(const void *data, const void *match=
_data)
>>> +{
>>> +       const struct prep_write_data *prep_data =3D data;
>>> +       const uint16_t *match_handle =3D match_data;
>>> +
>>> +       return prep_data->handle =3D=3D *match_handle;
>>> +}
>>> +
>>>  static bool store_prep_data(struct bt_gatt_server *server,
>>>                                         uint16_t handle, uint16_t offse=
t,
>>>                                         uint16_t length, uint8_t *value=
)
>>> @@ -1200,9 +1208,10 @@ static bool store_prep_data(struct bt_gatt_serve=
r *server,
>>>          * Now lets check if prep write is a continuation of long write
>>>          * If so do aggregation of data
>>>          */
>>> -       prep_data =3D queue_peek_tail(server->prep_queue);
>>> -       if (prep_data && (prep_data->handle =3D=3D handle) &&
>>> -                       (offset =3D=3D (prep_data->length + prep_data->=
offset)))
>>> +       prep_data =3D queue_find(server->prep_queue, match_prep_attr_ha=
ndle,
>>> +                                                               &handle=
);
>>
>> If I got this right it is not that the code is broken, it just don't
>> group together the writes if there are other handles in the prepare
>> queue. Afaik that was done because the order we write the handles may
>> matter, so it is up to the remote stack to make the order proper if it
>> wants long writes to all be executed in sequence, or interleaved which
>> means we would have to store new chunks in the order they are
>> prepared.
>>
>>> +       if (prep_data && (offset =3D=3D (prep_data->length + prep_data-=
>offset)))
>>>                 return append_prep_data(prep_data, handle, length, valu=
e);
>>>
>>>         return prep_data_new(server, handle, offset, length, value);
>>> --
>>> 2.13.6
>>>
>>> --
>>> 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



--=20
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/4] shared/gatt-server: Fix prepare write queuing

[Grzegorz Kołodziejczyk]

Hi Luiz,

Written value was replaced by last prepare write (queued by BlueZ) on
bluetoothctl, cause bluetoothctl is not aware of write offset (what is
the root cause of this issue I think). I belive it's reasonable to
keep gatt-server as is and try to fix it only on bluetoothctl then.

I'll do v3 with bluetoothctl fix instead of queuing on BlueZ side.

2018-04-20 13:19 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
> Hi Grzegorz,
>
> On Fri, Apr 20, 2018 at 11:11 AM, Grzegorz Ko=C5=82odziejczyk
> <grzegorz.kolodziejczyk@codecoup.pl> wrote:
>> Hi Luiz,
>>
>> I did this fix while testing it agains PTS test case "4.6.39
>> GATT/SR/GAW/BV-10-C [Nested Long Characteristic Value Reliable Writes
>> - to Server]"
>>
>> Without this fix bluez behaves as described:
>> 1) PTS do prepare write to CHARACTERISTIC1 with offset =3D 0 and value
>> with size =3D ATT_MTU-5.
>> 2) Bluez do new prepare write for CHARACTERISTIC1
>> 3) PTS do prepare write to CHARACTERISTIC2 with offset =3D 0 and value
>> with size =3D ATT_MTU-5.
>> 4) Bluez do new prepare write for CHARACTERISTIC2
>> 5) PTS do prepare write to CHARACTERISTIC1 with offset =3D ATT_MTU-5 and
>> value with size < ATT_MTU-5.
>> 6) Bluez do new prepare write for CHARACTERISTIC1
>> 7) PTS do prepare write to CHARACTERISTIC2 with offset =3D ATT_MTU-5 and
>> value with size < ATT_MTU-5.
>> 8) Bluez do new prepare write for CHARACTERISTIC2
>>
>> 9) Execut write is issued with flag "Immediately write all pend-ing
>> prepared values".
>> 10) PTS do read of those two CHARACTERISTICS and got response with
>> value from last prepare write to CHARACTERISTIC (this from second
>> prepare write form offset ATT_MTU-5) cause thos are 4 seperate prepare
>> writes queues (bluez appends only if last prepare write was to the
>> same handle).
>> 11) Test result fails cause expected value isn't the same as read.
>
>
> Have you checked what went wrong with the value written? Perhaps this
> is broken on the client side, afaik you do have a fix for that as
> well? I imagine when you changed to do the aggregation only one write
> will be send, so perhaps the client is not really appeding data using
> the offset. Also if the characteristic do support AcquireWrite then
> this whole thing is guaranteed not to work because writing to the pipe
> fd does not support setting an offset currently, so perhaps that is
> the real issue. Fixing that shall be possible using sendmsg and
> setting the offset in the msg_control then the client has to read that
> back with recvmsg.
>
>> What's more, spec says"
>> "If a Characteristic Value is prepared two or more times during this sub=
-
>> procedure, then all prepared values are written to the same Characterist=
ic
>> Value in the order that they were prepared." BLUETOOTH SPECIFICATION
>> Version 5.0 | Vol 3, Part G (4.9.5)
>>
>> "Each client=E2=80=99s queued values are separate; the execution of one =
queue shall not
>> affect the preparation or execution of any other client=E2=80=99s queued
>> values." BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part F (3.4.6.1)
>>
>> Regards,
>> Grzegorz
>>
>> 2018-04-19 17:11 GMT+02:00 Luiz Augusto von Dentz <luiz.dentz@gmail.com>=
:
>>> Hi Grzegorz,
>>>
>>> On Thu, Apr 19, 2018 at 5:03 PM, Grzegorz Kolodziejczyk
>>> <grzegorz.kolodziejczyk@codecoup.pl> wrote:
>>>> Multiple prepare writes may be done for multiple attributes. Queue
>>>> mechanism must be aware of handle under which preparation is done.
>>>> ---
>>>>  src/shared/gatt-server.c | 15 ++++++++++++---
>>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>>> index 4b554f665..d1efa83a1 100644
>>>> --- a/src/shared/gatt-server.c
>>>> +++ b/src/shared/gatt-server.c
>>>> @@ -1190,6 +1190,14 @@ static bool prep_data_new(struct bt_gatt_server=
 *server,
>>>>         return true;
>>>>  }
>>>>
>>>> +static bool match_prep_attr_handle(const void *data, const void *matc=
h_data)
>>>> +{
>>>> +       const struct prep_write_data *prep_data =3D data;
>>>> +       const uint16_t *match_handle =3D match_data;
>>>> +
>>>> +       return prep_data->handle =3D=3D *match_handle;
>>>> +}
>>>> +
>>>>  static bool store_prep_data(struct bt_gatt_server *server,
>>>>                                         uint16_t handle, uint16_t offs=
et,
>>>>                                         uint16_t length, uint8_t *valu=
e)
>>>> @@ -1200,9 +1208,10 @@ static bool store_prep_data(struct bt_gatt_serv=
er *server,
>>>>          * Now lets check if prep write is a continuation of long writ=
e
>>>>          * If so do aggregation of data
>>>>          */
>>>> -       prep_data =3D queue_peek_tail(server->prep_queue);
>>>> -       if (prep_data && (prep_data->handle =3D=3D handle) &&
>>>> -                       (offset =3D=3D (prep_data->length + prep_data-=
>offset)))
>>>> +       prep_data =3D queue_find(server->prep_queue, match_prep_attr_h=
andle,
>>>> +                                                               &handl=
e);
>>>
>>> If I got this right it is not that the code is broken, it just don't
>>> group together the writes if there are other handles in the prepare
>>> queue. Afaik that was done because the order we write the handles may
>>> matter, so it is up to the remote stack to make the order proper if it
>>> wants long writes to all be executed in sequence, or interleaved which
>>> means we would have to store new chunks in the order they are
>>> prepared.
>>>
>>>> +       if (prep_data && (offset =3D=3D (prep_data->length + prep_data=
->offset)))
>>>>                 return append_prep_data(prep_data, handle, length, val=
ue);
>>>>
>>>>         return prep_data_new(server, handle, offset, length, value);
>>>> --
>>>> 2.13.6
>>>>
>>>> --
>>>> 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
>
>
>
> --
> Luiz Augusto von Dentz

Grzegorz