2009-05-06 09:19:00

by Forrest Zhao

Subject: [PATCH] in headset_init(), it's possible that hs->hfp_handle is not set, so we should not check hs->hfp_handle in headset_connect_cb()

We found this bug when developing the HFP plugin for telephonyd.

---
audio/headset.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/audio/headset.c b/audio/headset.c
index 9f6b736..9b9be17 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -1304,8 +1304,7 @@ void headset_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
else
hs->auto_dc = FALSE;

- if (server_is_enabled(&dev->src, HANDSFREE_SVCLASS_ID) &&
- hs->hfp_handle != 0)
+ if (server_is_enabled(&dev->src, HANDSFREE_SVCLASS_ID))
hs->hfp_active = TRUE;
else
hs->hfp_active = FALSE;
--
1.5.4.5

2009-05-07 02:03:06

by Zhao Forrest

Subject: Re: [PATCH] in headset_init(), it's possible that hs->hfp_handle is not set, so we should not check hs->hfp_handle in headset_connect_cb()

>>
>> I don't see how that's a bug. hs->hfp_active should be true only when we
>> are connected to HFP. server_is_enabled(src, HANDSFREE_SVCLASS_ID) is
>> supposed to return true if HFP support is enabled in general. These are
>> two separate things (we could be connected to HSP even though HFP support
>> is enabled if the headset only supports HSP, i.e. hs->hfp_handle == 0 in
>> this case).
>>
> Let me describe how the bug is triggered by the following steps in our
> lab:
> 1 at HFP AW side bluetoothd is started, and headset_init() is called.
> However btd_device_get_record() returns NULL, so hs->hfp_handle is
> NULL.
> 2 at HFP HF unit side bluetoothd is started, and initiated connection to HFP AW
> 3 HFP AW accepts the connection, and headset_connect_cb() is called.
> At this time hs->hfp_handle is NULL, so hs->hfp_active is set to
> FALSE.
>
> This way the bug is triggered: a real HFP connection is initiated by HFP HF
> unit, but hs->hfp_active is set to FALSE by HFP AW. It seems that this patch
> is not a real fix, could you share the ideas of fixing this bug?
>
Johan,

After cleaning /var/lib/bluetooth/* and doing pairing from scratch,
this bug can't be reproduced. It seems that a messed-up SDP database
triggered the issue.

Thanks,
Forrest

2009-05-06 12:28:17

by Zhao Forrest

Subject: Re: [PATCH] in headset_init(), it's possible that hs->hfp_handle is not set, so we should not check hs->hfp_handle in headset_connect_cb()

>
> I don't see how that's a bug. hs->hfp_active should be true only when we
> are connected to HFP. server_is_enabled(src, HANDSFREE_SVCLASS_ID) is
> supposed to return true if HFP support is enabled in general. These are
> two separate things (we could be connected to HSP even though HFP support
> is enabled if the headset only supports HSP, i.e. hs->hfp_handle == 0 in
> this case).
>
Let me describe how the bug is triggered by the following steps in our
lab:
1 at HFP AW side bluetoothd is started, and headset_init() is called.
However btd_device_get_record() returns NULL, so hs->hfp_handle is
NULL.
2 at HFP HF unit side bluetoothd is started, and initiated connection to HFP AW
3 HFP AW accepts the connection, and headset_connect_cb() is called.
At this time hs->hfp_handle is NULL, so hs->hfp_active is set to
FALSE.

This way the bug is triggered: a real HFP connection is initiated by HFP HF
unit, but hs->hfp_active is set to FALSE by HFP AW. It seems that this patch
is not a real fix, could you share the ideas of fixing this bug?

Thanks,
Forrest

2009-05-06 09:30:49

by Johan Hedberg

Subject: Re: [PATCH] in headset_init(), it's possible that hs->hfp_handle is not set, so we should not check hs->hfp_handle in headset_connect_cb()

Hi Forrest,

On Wed, May 06, 2009, Forrest Zhao wrote:
> We found this bug when developing the HFP plugin for telephonyd.
>
> ---
> audio/headset.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/audio/headset.c b/audio/headset.c
> index 9f6b736..9b9be17 100644
> --- a/audio/headset.c
> +++ b/audio/headset.c
> @@ -1304,8 +1304,7 @@ void headset_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
> else
> hs->auto_dc = FALSE;
>
> - if (server_is_enabled(&dev->src, HANDSFREE_SVCLASS_ID) &&
> - hs->hfp_handle != 0)
> + if (server_is_enabled(&dev->src, HANDSFREE_SVCLASS_ID))
> hs->hfp_active = TRUE;
> else
> hs->hfp_active = FALSE;

I don't see how that's a bug. hs->hfp_active should be true only when we
are connected to HFP. server_is_enabled(src, HANDSFREE_SVCLASS_ID) is
supposed to return true if HFP support is enabled in general. These are
two separate things (we could be connected to HSP even though HFP support
is enabled if the headset only supports HSP, i.e. hs->hfp_handle == 0 in
this case).

Johan