Re: [Bluez PATCH v2 1/2] device: add device_remove_bonding function

Hi Archie,

On Wed, Jul 15, 2020 at 7:15 AM Archie Pusaka [off-list ref] wrote:

Hi Luiz,

If we mark it as temporary, then the device will immediately get
deleted upon disconnection.
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n6875
This is the same situation as directly calling device_remove.

May I know the reason why we want to put the device as temporary?

If we currently don't have a way to keep a "previously connected but
no longer bonded" device, then removing the device perhaps is the next
best option. It still makes the user scan for the virtually
disconnected device though.

We keep a cache of previously known devices, but we only display them
once they are actually found otherwise it may grow too big which is
inconvenient, I wonder where this concept of "previously connected but
no longer bonded" comes from though, we had the temporary to map
devices that would not be persisted on the storage which I guess this
is what it is about, that said we could perhaps have a timeout before
setting it as temporary but we might want to integrate with the logic
of detecting devices disappearing.

Thanks,
Archie

On Wed, 15 Jul 2020 at 01:10, Luiz Augusto von Dentz
[off-list ref] wrote:

quoted

Hi Archie,

On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka [off-list ref] wrote:

quoted

Hi Luiz,

As far as the spec is concerned, we can also remove the device by
calling device_remove. However, I suppose it would be confusing for
end users if they can no longer find their HID device on the device
list just because the device previously sent a virtual cable
disconnection.
The HID 1.0 spec part 6.4.2 also gives an example of a possible
scenario when a virtually cabled device is removed: "Unplugged devices
shall be marked as known and put into a “most recently used list” of
known devices to facilitate future re-connecting".

Then perhaps we shall have it marked as temporary as well, that said
we do want to introduce disappearing logic so temporary devices are
not left dangling for too long.

quoted

Thanks,
Archie


On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz
[off-list ref] wrote:

quoted

Hi Archie,

On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka [off-list ref] wrote:

quoted

From: Archie Pusaka <redacted>

This patch splits the "bonding removal" function in device.c,
because we need to remove bonding information when receiving
"virtual cable unplug" in HID profile.

Reviewed-by: Alain Michaud <redacted>
---

Changes in v2: None

 src/device.c | 25 +++++++++++++++----------
 src/device.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/device.c b/src/device.c
index 7b0eb256e..9fb0e018c 100644
--- a/src/device.c
+++ b/src/device.c

@@ -4162,6 +4162,17 @@ static void delete_folder_tree(const char *dirname)
        rmdir(dirname);
 }

+void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type)
+{
+       if (bdaddr_type == BDADDR_BREDR)
+               device->bredr_state.bonded = false;
+       else
+               device->le_state.bonded = false;
+
+       btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
+                                                       bdaddr_type);
+}
+
 static void device_remove_stored(struct btd_device *device)
 {
        char device_addr[18];

@@ -4170,17 +4181,11 @@ static void device_remove_stored(struct btd_device *device)
        char *data;
        gsize length = 0;

-       if (device->bredr_state.bonded) {
-               device->bredr_state.bonded = false;
-               btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
-                                                               BDADDR_BREDR);
-       }
+       if (device->bredr_state.bonded)
+               device_remove_bonding(device, BDADDR_BREDR);

-       if (device->le_state.bonded) {
-               device->le_state.bonded = false;
-               btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
-                                                       device->bdaddr_type);
-       }
+       if (device->le_state.bonded)
+               device_remove_bonding(device, device->bdaddr_type);

        device->bredr_state.paired = false;
        device->le_state.paired = false;

diff --git a/src/device.h b/src/device.h
index 06b100499..907c7c5c4 100644
--- a/src/device.h
+++ b/src/device.h

@@ -49,6 +49,7 @@ uint16_t btd_device_get_vendor(struct btd_device *device);
 uint16_t btd_device_get_vendor_src(struct btd_device *device);
 uint16_t btd_device_get_product(struct btd_device *device);
 uint16_t btd_device_get_version(struct btd_device *device);
+void device_remove_bonding(struct btd_device *device, uint8_t bdaddr_type);
 void device_remove(struct btd_device *device, gboolean remove_stored);

Is there any particular reason why device_remove is not enough here? I
don't see any reason to leave the device object around after removing
its bonding.

quoted

 int device_address_cmp(gconstpointer a, gconstpointer b);
 int device_bdaddr_cmp(gconstpointer a, gconstpointer b);
--
2.27.0.212.ge8ba1cc988-goog

--
Luiz Augusto von Dentz

--
Luiz Augusto von Dentz

-- 
Luiz Augusto von Dentz