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

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Archie,

On Thu, Jul 16, 2020 at 12:28 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> I think spec writers' idea is to have a list of previously connected
> devices which is ordered by most recently connected. The size of this
> list may be limited to a number, meaning that the least recently
> connected device will be removed from the list. The devices in this
> list may or may not be bonded. This list is accessible to users, so
> they can easily reconnect to the most recently used device.
>
> I don't suppose we currently have this list, so I'm happy with just
> removing the virtually unplugged device.

So with the latest developments Im leaning towards using
device_set_temporary since that would trigger a timer to remove the
device after it has expired.

> Thanks,
> Archie
>
> On Thu, 16 Jul 2020 at 01:31, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Wed, Jul 15, 2020 at 7:15 AM Archie Pusaka <apusaka@google.com> 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
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Archie,
> > > >
> > > > On Tue, Jul 7, 2020 at 9:30 PM Archie Pusaka <apusaka@google.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > > > > Thanks,
> > > > > Archie
> > > > >
> > > > >
> > > > > On Wed, 8 Jul 2020 at 02:03, Luiz Augusto von Dentz
> > > > > <luiz.dentz@gmail.com> wrote:
> > > > > >
> > > > > > Hi Archie,
> > > > > >
> > > > > > On Sat, Jun 27, 2020 at 8:54 AM Archie Pusaka <apusaka@google.com> wrote:
> > > > > > >
> > > > > > > From: Archie Pusaka <apusaka@chromium.org>
> > > > > > >
> > > > > > > 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 <alainm@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > >  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



-- 
Luiz Augusto von Dentz