Hi Javier,
On Sat, Aug 26, 2023 at 1:08 PM Javier de San Pedro
<dev.git@xxxxxxxxxxxxxx> wrote:
>
> I noticed after upgrading 5.66->5.68 that Bluez was no longer sending
> confirmations (ATT opcode 0x1E) in response to indication opcodes (0x1D).
>
> Commit fde32ff9c9c0 ("shared/gatt-client: Allow registering with NULL
> callback") added an early return to the notify_cb function when the
> current client's notify_list is empty. However, in this case, we will
> also not send the confirmation back. This breaks protocol.
>
> The devices I have generally respond to this by stopping
> any new indications until ~15sec timeout or disconnection.
>
> As far as I can see, when using D-Bus API all notify handlers are always
> added on client clones, never on the 'root' client itself (the one
> with !client->parent), so for the root client the notify_list is always
> empty, making this issue very easy to trigger using D-Bus GATT API.
>
> Ensure that we always send the confirmation, even if notify_list is empty.
>
> Fixes: fde32ff9c9c0 ("shared/gatt-client: Allow registering with NULL callback")
> ---
> src/shared/gatt-client.c | 57 ++++++++++++++++++++--------------------
> 1 file changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index efc013a20..56ecc6a8f 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -2230,45 +2230,46 @@ static void notify_cb(struct bt_att_chan *chan, uint8_t opcode,
> void *user_data)
> {
> struct bt_gatt_client *client = user_data;
> - struct value_data data;
> -
> - if (queue_isempty(client->notify_list))
> - return;
Instead reversing the if logic here you could just use goto to where
we handle the confirmation.
>
> bt_gatt_client_ref(client);
>
> - memset(&data, 0, sizeof(data));
> + if (!queue_isempty(client->notify_list)) {
> + struct value_data data;
>
> - if (opcode == BT_ATT_OP_HANDLE_NFY_MULT) {
> - while (length >= 4) {
> - data.handle = get_le16(pdu);
> - length -= 2;
> - pdu += 2;
> + memset(&data, 0, sizeof(data));
>
> - data.len = get_le16(pdu);
> - length -= 2;
> - pdu += 2;
> + if (opcode == BT_ATT_OP_HANDLE_NFY_MULT) {
> + while (length >= 4) {
> + data.handle = get_le16(pdu);
> + length -= 2;
> + pdu += 2;
>
> - if (data.len > length)
> - data.len = length;
> + data.len = get_le16(pdu);
> + length -= 2;
> + pdu += 2;
>
> - data.data = pdu;
> + if (data.len > length)
> + data.len = length;
>
> - queue_foreach(client->notify_list, notify_handler,
> - &data);
> + data.data = pdu;
>
> - length -= data.len;
> - pdu += data.len;
> - }
> - } else {
> - data.handle = get_le16(pdu);
> - length -= 2;
> - pdu += 2;
> + queue_foreach(client->notify_list,
> + notify_handler, &data);
>
> - data.len = length;
> - data.data = pdu;
> + length -= data.len;
> + pdu += data.len;
> + }
> + } else {
> + data.handle = get_le16(pdu);
> + length -= 2;
> + pdu += 2;
> +
> + data.len = length;
> + data.data = pdu;
>
> - queue_foreach(client->notify_list, notify_handler, &data);
> + queue_foreach(client->notify_list,
> + notify_handler, &data);
> + }
> }
>
> if (opcode == BT_ATT_OP_HANDLE_IND && !client->parent)
> --
> 2.41.0
>
--
Luiz Augusto von Dentz
