[PATCH BlueZ] gatt: Clear subscriptions for device not paired

[PATCH BlueZ] gatt: Clear subscriptions for device not paired

[Luiz Augusto von Dentz]

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

If the device is no longer valid or is not considered bonded anymore
clear its CCC states before removing otherwise application may continue
to notify when there are no devices listening.
---
 src/gatt-database.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 47304704a..d6d1e4d13 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -891,6 +891,23 @@ static void conf_cb(void *user_data)
 	}
 }
 
+static void clear_ccc_state(void *data, void *user_data)
+{
+	struct ccc_state *ccc = data;
+	struct btd_gatt_database *db = user_data;
+	struct ccc_cb_data *ccc_cb;
+
+	if (!ccc->value[0])
+		return;
+
+	ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle,
+						UINT_TO_PTR(ccc->handle));
+	if (!ccc_cb)
+		return;
+
+	ccc_cb->callback(NULL, 0, ccc_cb->user_data);
+}
+
 static void send_notification_to_device(void *data, void *user_data)
 {
 	struct device_state *device_state = data;
@@ -940,8 +957,11 @@ static void send_notification_to_device(void *data, void *user_data)
 
 remove:
 	/* Remove device state if device no longer exists or is not paired */
-	if (queue_remove(notify->database->device_states, device_state))
+	if (queue_remove(notify->database->device_states, device_state)) {
+		queue_foreach(device_state->ccc_states, clear_ccc_state,
+						notify->database);
 		device_state_free(device_state);
+	}
 }
 
 static void send_notification_to_devices(struct btd_gatt_database *database,
-- 
2.13.6

Re: [PATCH BlueZ] gatt: Clear subscriptions for device not paired

[Yunhan Wang]

Hi, Luiz

I assume that you wanna see that server application try to do
send_notification_to_device,  device is NULL/disconnected, then
bluetoothd would clear status for subscription.

However, when ble is disconnected, send_notification_to_device might
not be triggered any more so that device state cannot be cleaned. In
server I have used StartNotify and AcquireNotify to verify it, and
send_notification_to_device cannot be triggered after ble is
disconnected even though server application has tried to send data.

Here are two ideas:

In gatt-datebase.c, could you invoke clear_ccc_state inside pipe_up?
When server application destroy pipe, the corresponding pipe_up in
bluetoothd could remove GATT states that related to notify_io.

Another idea:
When disconnected event is detected in adapter.c or device.c, could
you call clear_ccc_state to clear all states for all characteristic,
reset them to original status.

Thanks
Best wishes
Yunhan

On Fri, Oct 20, 2017 at 5:21 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> If the device is no longer valid or is not considered bonded anymore
> clear its CCC states before removing otherwise application may continue
> to notify when there are no devices listening.
> ---
>  src/gatt-database.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 47304704a..d6d1e4d13 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -891,6 +891,23 @@ static void conf_cb(void *user_data)
>         }
>  }
>
> +static void clear_ccc_state(void *data, void *user_data)
> +{
> +       struct ccc_state *ccc = data;
> +       struct btd_gatt_database *db = user_data;
> +       struct ccc_cb_data *ccc_cb;
> +
> +       if (!ccc->value[0])
> +               return;
> +
> +       ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle,
> +                                               UINT_TO_PTR(ccc->handle));
> +       if (!ccc_cb)
> +               return;
> +
> +       ccc_cb->callback(NULL, 0, ccc_cb->user_data);
> +}
> +
>  static void send_notification_to_device(void *data, void *user_data)
>  {
>         struct device_state *device_state = data;
> @@ -940,8 +957,11 @@ static void send_notification_to_device(void *data, void *user_data)
>
>  remove:
>         /* Remove device state if device no longer exists or is not paired */
> -       if (queue_remove(notify->database->device_states, device_state))
> +       if (queue_remove(notify->database->device_states, device_state)) {
> +               queue_foreach(device_state->ccc_states, clear_ccc_state,
> +                                               notify->database);
>                 device_state_free(device_state);
> +       }
>  }
>
>  static void send_notification_to_devices(struct btd_gatt_database *database,
> --
> 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

Re: [PATCH BlueZ] gatt: Clear subscriptions for device not paired

[Luiz Augusto von Dentz]

Hi Yunha,

On Sat, Oct 21, 2017 at 3:26 AM, Yunhan Wang <yunhanw@google.com> wrote:
> Hi, Luiz
>
> I assume that you wanna see that server application try to do
> send_notification_to_device,  device is NULL/disconnected, then
> bluetoothd would clear status for subscription.
>
> However, when ble is disconnected, send_notification_to_device might
> not be triggered any more so that device state cannot be cleaned. In
> server I have used StartNotify and AcquireNotify to verify it, and
> send_notification_to_device cannot be triggered after ble is
> disconnected even though server application has tried to send data.

bluetoothd[28769]: src/gatt-client.c:btd_gatt_client_disconnected()
Device disconnected. Cleaning up.
bluetoothd[28769]: src/device.c:att_disconnected_cb() Automatic
connection disabled
...
bluetoothd[28769]: src/gatt-database.c:ccc_write_cb() External CCC
write received with value: 0x0000

It seems to be working alright here, perhaps you are saying it doesn't
work if the notification is not called while the device is disconnect
then we have a problem, so we need a proper hook for a device being
disconnect not only on demand when the device attempts to send a
notification?

> Here are two ideas:
>
> In gatt-datebase.c, could you invoke clear_ccc_state inside pipe_up?
> When server application destroy pipe, the corresponding pipe_up in
> bluetoothd could remove GATT states that related to notify_io.

If we got a hup while the remote believes it is subscribed this can
cause it to go out of sync, so this should only really happen if you
are removing the whole service so the remote can notice the change via
service changed indication.

> Another idea:
> When disconnected event is detected in adapter.c or device.c, could
> you call clear_ccc_state to clear all states for all characteristic,
> reset them to original status.

Only if the device has not been paired, then yes we should clear any
ccc state related to the device being disconnected.

>
> Thanks
> Best wishes
> Yunhan
>
> On Fri, Oct 20, 2017 at 5:21 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> If the device is no longer valid or is not considered bonded anymore
>> clear its CCC states before removing otherwise application may continue
>> to notify when there are no devices listening.
>> ---
>>  src/gatt-database.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>> index 47304704a..d6d1e4d13 100644
>> --- a/src/gatt-database.c
>> +++ b/src/gatt-database.c
>> @@ -891,6 +891,23 @@ static void conf_cb(void *user_data)
>>         }
>>  }
>>
>> +static void clear_ccc_state(void *data, void *user_data)
>> +{
>> +       struct ccc_state *ccc = data;
>> +       struct btd_gatt_database *db = user_data;
>> +       struct ccc_cb_data *ccc_cb;
>> +
>> +       if (!ccc->value[0])
>> +               return;
>> +
>> +       ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle,
>> +                                               UINT_TO_PTR(ccc->handle));
>> +       if (!ccc_cb)
>> +               return;
>> +
>> +       ccc_cb->callback(NULL, 0, ccc_cb->user_data);
>> +}
>> +
>>  static void send_notification_to_device(void *data, void *user_data)
>>  {
>>         struct device_state *device_state = data;
>> @@ -940,8 +957,11 @@ static void send_notification_to_device(void *data, void *user_data)
>>
>>  remove:
>>         /* Remove device state if device no longer exists or is not paired */
>> -       if (queue_remove(notify->database->device_states, device_state))
>> +       if (queue_remove(notify->database->device_states, device_state)) {
>> +               queue_foreach(device_state->ccc_states, clear_ccc_state,
>> +                                               notify->database);
>>                 device_state_free(device_state);
>> +       }
>>  }
>>
>>  static void send_notification_to_devices(struct btd_gatt_database *database,
>> --
>> 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] gatt: Clear subscriptions for device not paired

[Yunhan Wang]

Hi, Luiz

On Mon, Oct 23, 2017 at 1:12 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Yunha,
>
> On Sat, Oct 21, 2017 at 3:26 AM, Yunhan Wang <yunhanw@google.com> wrote:
>> Hi, Luiz
>>
>> I assume that you wanna see that server application try to do
>> send_notification_to_device,  device is NULL/disconnected, then
>> bluetoothd would clear status for subscription.
>>
>> However, when ble is disconnected, send_notification_to_device might
>> not be triggered any more so that device state cannot be cleaned. In
>> server I have used StartNotify and AcquireNotify to verify it, and
>> send_notification_to_device cannot be triggered after ble is
>> disconnected even though server application has tried to send data.
>
> bluetoothd[28769]: src/gatt-client.c:btd_gatt_client_disconnected()
> Device disconnected. Cleaning up.
> bluetoothd[28769]: src/device.c:att_disconnected_cb() Automatic
> connection disabled
> ...
> bluetoothd[28769]: src/gatt-database.c:ccc_write_cb() External CCC
> write received with value: 0x0000
>
> It seems to be working alright here, perhaps you are saying it doesn't
> work if the notification is not called while the device is disconnect
> then we have a problem, so we need a proper hook for a device being
> disconnect not only on demand when the device attempts to send a
> notification?

Yes

>
>> Here are two ideas:
>>
>> In gatt-datebase.c, could you invoke clear_ccc_state inside pipe_up?
>> When server application destroy pipe, the corresponding pipe_up in
>> bluetoothd could remove GATT states that related to notify_io.
>
> If we got a hup while the remote believes it is subscribed this can
> cause it to go out of sync, so this should only really happen if you
> are removing the whole service so the remote can notice the change via
> service changed indication.
>
Yes, I agree.

>> Another idea:
>> When disconnected event is detected in adapter.c or device.c, could
>> you call clear_ccc_state to clear all states for all characteristic,
>> reset them to original status.
>
> Only if the device has not been paired, then yes we should clear any
> ccc state related to the device being disconnected.
>
Yes, I agree.
>>
>> Thanks
>> Best wishes
>> Yunhan
>>
>> On Fri, Oct 20, 2017 at 5:21 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> If the device is no longer valid or is not considered bonded anymore
>>> clear its CCC states before removing otherwise application may continue
>>> to notify when there are no devices listening.
>>> ---
>>>  src/gatt-database.c | 22 +++++++++++++++++++++-
>>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gatt-database.c b/src/gatt-database.c
>>> index 47304704a..d6d1e4d13 100644
>>> --- a/src/gatt-database.c
>>> +++ b/src/gatt-database.c
>>> @@ -891,6 +891,23 @@ static void conf_cb(void *user_data)
>>>         }
>>>  }
>>>
>>> +static void clear_ccc_state(void *data, void *user_data)
>>> +{
>>> +       struct ccc_state *ccc = data;
>>> +       struct btd_gatt_database *db = user_data;
>>> +       struct ccc_cb_data *ccc_cb;
>>> +
>>> +       if (!ccc->value[0])
>>> +               return;
>>> +
>>> +       ccc_cb = queue_find(db->ccc_callbacks, ccc_cb_match_handle,
>>> +                                               UINT_TO_PTR(ccc->handle));
>>> +       if (!ccc_cb)
>>> +               return;
>>> +
>>> +       ccc_cb->callback(NULL, 0, ccc_cb->user_data);
>>> +}
>>> +
>>>  static void send_notification_to_device(void *data, void *user_data)
>>>  {
>>>         struct device_state *device_state = data;
>>> @@ -940,8 +957,11 @@ static void send_notification_to_device(void *data, void *user_data)
>>>
>>>  remove:
>>>         /* Remove device state if device no longer exists or is not paired */
>>> -       if (queue_remove(notify->database->device_states, device_state))
>>> +       if (queue_remove(notify->database->device_states, device_state)) {
>>> +               queue_foreach(device_state->ccc_states, clear_ccc_state,
>>> +                                               notify->database);
>>>                 device_state_free(device_state);
>>> +       }
>>>  }
>>>
>>>  static void send_notification_to_devices(struct btd_gatt_database *database,
>>> --
>>> 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