2020-08-17 23:01:55

by Yu Liu

[permalink] [raw]
Subject: [Bluez PATCH v2] adapter- Device needs to be in the temporary state after pairing failure

This caused the device hanging around as a discovered device forever
even if it is turned off or not in present.

Reviewed-by: [email protected]

Signed-off-by: Yu Liu <[email protected]>
---

Changes in v2:
- Fix title length and format issue

Changes in v1:
- Initial change

src/device.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/src/device.c b/src/device.c
index bb8e07e8f..93e71850c 100644
--- a/src/device.c
+++ b/src/device.c
@@ -6008,6 +6008,12 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,

if (status) {
device_cancel_authentication(device, TRUE);
+
+ // Put the device back to the temporary state so that it will be
+ // treated as a newly discovered device.
+ if (!device_is_paired(device, bdaddr_type))
+ btd_device_set_temporary(device, true);
+
device_bonding_failed(device, status);
return;
}
--
2.28.0.220.ged08abb693-goog


2020-08-17 23:24:51

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez PATCH v2] adapter- Device needs to be in the temporary state after pairing failure

Hi Yu Liu,

On Mon, Aug 17, 2020 at 4:04 PM Yu Liu <[email protected]> wrote:
>
> This caused the device hanging around as a discovered device forever
> even if it is turned off or not in present.
>
> Reviewed-by: [email protected]
>
> Signed-off-by: Yu Liu <[email protected]>
> ---
>
> Changes in v2:
> - Fix title length and format issue
>
> Changes in v1:
> - Initial change
>
> src/device.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index bb8e07e8f..93e71850c 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -6008,6 +6008,12 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
>
> if (status) {
> device_cancel_authentication(device, TRUE);
> +
> + // Put the device back to the temporary state so that it will be
> + // treated as a newly discovered device.

Use C style comments /* */ above.

> + if (!device_is_paired(device, bdaddr_type))
> + btd_device_set_temporary(device, true);

Hmm, are we perhaps removing the temporary flag too early? Or this is
a result of calling Connect which clears the temporary flag? Then
perhaps we should at least if the upper layer has marked the device as
trusted as it should indicate the device object should be kept even if
not paired.

> device_bonding_failed(device, status);
> return;
> }
> --
> 2.28.0.220.ged08abb693-goog
>


--
Luiz Augusto von Dentz

2020-08-17 23:38:10

by Yu Liu

[permalink] [raw]
Subject: Re: [Bluez PATCH v2] adapter- Device needs to be in the temporary state after pairing failure

that could be a reason and a potential fix, we remove the temp flag in
dev_connect and pair_device very early on, but i suspect changing that
would potentially have bigger impact and needs more due diligence and
testing.

On Mon, Aug 17, 2020 at 4:24 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Yu Liu,
>
> On Mon, Aug 17, 2020 at 4:04 PM Yu Liu <[email protected]> wrote:
> >
> > This caused the device hanging around as a discovered device forever
> > even if it is turned off or not in present.
> >
> > Reviewed-by: [email protected]
> >
> > Signed-off-by: Yu Liu <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Fix title length and format issue
> >
> > Changes in v1:
> > - Initial change
> >
> > src/device.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/src/device.c b/src/device.c
> > index bb8e07e8f..93e71850c 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -6008,6 +6008,12 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> >
> > if (status) {
> > device_cancel_authentication(device, TRUE);
> > +
> > + // Put the device back to the temporary state so that it will be
> > + // treated as a newly discovered device.
>
> Use C style comments /* */ above.
>
> > + if (!device_is_paired(device, bdaddr_type))
> > + btd_device_set_temporary(device, true);
>
> Hmm, are we perhaps removing the temporary flag too early? Or this is
> a result of calling Connect which clears the temporary flag? Then
> perhaps we should at least if the upper layer has marked the device as
> trusted as it should indicate the device object should be kept even if
> not paired.
>
> > device_bonding_failed(device, status);
> > return;
> > }
> > --
> > 2.28.0.220.ged08abb693-goog
> >
>
>
> --
> Luiz Augusto von Dentz

2020-08-24 18:29:45

by Yu Liu

[permalink] [raw]
Subject: Re: [Bluez PATCH v2] adapter- Device needs to be in the temporary state after pairing failure

Hi Luiz,

I just sent another patch to address your comments. The issue this cl
is trying to address is that when we pair a device by calling
pair_device, it removes the temporary flag very early on, then it the
pairing failed due to reasons such as failed to connect the temp flag
won't be removed and the device will be hanging around forever. Please
review again. Thanks.

On Mon, Aug 17, 2020 at 4:24 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Yu Liu,
>
> On Mon, Aug 17, 2020 at 4:04 PM Yu Liu <[email protected]> wrote:
> >
> > This caused the device hanging around as a discovered device forever
> > even if it is turned off or not in present.
> >
> > Reviewed-by: [email protected]
> >
> > Signed-off-by: Yu Liu <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Fix title length and format issue
> >
> > Changes in v1:
> > - Initial change
> >
> > src/device.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/src/device.c b/src/device.c
> > index bb8e07e8f..93e71850c 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -6008,6 +6008,12 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> >
> > if (status) {
> > device_cancel_authentication(device, TRUE);
> > +
> > + // Put the device back to the temporary state so that it will be
> > + // treated as a newly discovered device.
>
> Use C style comments /* */ above.
>
> > + if (!device_is_paired(device, bdaddr_type))
> > + btd_device_set_temporary(device, true);
>
> Hmm, are we perhaps removing the temporary flag too early? Or this is
> a result of calling Connect which clears the temporary flag? Then
> perhaps we should at least if the upper layer has marked the device as
> trusted as it should indicate the device object should be kept even if
> not paired.
>
> > device_bonding_failed(device, status);
> > return;
> > }
> > --
> > 2.28.0.220.ged08abb693-goog
> >
>
>
> --
> Luiz Augusto von Dentz