[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()

[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()

[Forrest Zhao]

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

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()

[Johan Hedberg]

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

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()

[Zhao Forrest]

>
> 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

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()

[Zhao Forrest]

>>
>> 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