[PATCH 1/4] android/health: Fix connect channel failure case

[PATCH 1/4] android/health: Fix connect channel failure case

[Ravi kumar Veeramally]

There is a possibilty of created channel is being added to queue,
but on failure case it does channel free but not removed from queue.
---
 android/health.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/android/health.c b/android/health.c
index f4edc3e..f86fd26 100644
--- a/android/health.c
+++ b/android/health.c
@@ -1978,6 +1978,7 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 	return;
 
 fail:
+	queue_remove(channel->dev->channels, channel);
 	free_health_channel(channel);
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
 			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);
-- 
1.9.1

[PATCH 2/4] android/health: Remove mcl instance on mcl_disconnected call back

[Ravi kumar Veeramally]

MCAP library is maintaining two sets of mcl list. On mcl_disconnected
call mcl instance is removed from regular mcl list and cached in mcl_cache
list. health.c doesn't maintain any cached mcls list. So mcl_conn variable
doesn't make any sense of not being connected on mcl_disconnected call back.
So unref mcl on mcl_disconnected and don't do anything in mcl_unached callback.
---
 android/health.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/android/health.c b/android/health.c
index f86fd26..361b043 100644
--- a/android/health.c
+++ b/android/health.c
@@ -85,7 +85,6 @@ struct health_device {
 	uint16_t app_id;
 
 	struct mcap_mcl *mcl;
-	bool mcl_conn;
 
 	struct queue *channels;     /* data channels */
 
@@ -193,7 +192,6 @@ static void unref_mcl(struct health_device *dev)
 	mcap_close_mcl(dev->mcl, FALSE);
 	mcap_mcl_unref(dev->mcl);
 	dev->mcl = NULL;
-	dev->mcl_conn = false;
 }
 
 static void free_health_device(void *data)
@@ -1824,7 +1822,6 @@ static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
 	if (!channel->dev->mcl)
 		channel->dev->mcl = mcap_mcl_ref(mcl);
 
-	channel->dev->mcl_conn = true;
 	info("MCL connected");
 
 	ret = set_mcl_cb(channel->dev->mcl, channel, &gerr);
@@ -1949,7 +1946,7 @@ static void bt_health_connect_channel(const void *buf, uint16_t len)
 		}
 	}
 
-	if (!dev->mcl || (dev->mcl && !dev->mcl_conn)) {
+	if (!dev->mcl) {
 		if (connect_mcl(channel) < 0) {
 			error("health: error retrieving HDP SDP record");
 			goto fail;
@@ -2069,8 +2066,6 @@ static void mcl_reconnected(struct mcap_mcl *mcl, gpointer data)
 		error("device data does not exists");
 		return;
 	}
-
-	dev->mcl_conn = true;
 }
 
 static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
@@ -2080,18 +2075,13 @@ static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
 	DBG("");
 
 	dev = search_dev_by_mcl(mcl);
-	if (dev)
-		dev->mcl_conn = false;
+	mcap_mcl_unref(dev->mcl);
+	dev->mcl = NULL;
 }
 
 static void mcl_uncached(struct mcap_mcl *mcl, gpointer data)
 {
-	struct health_device *dev;
-
-	DBG("");
-
-	dev = search_dev_by_mcl(mcl);
-	free_health_device(dev);
+	/* mcap library maintains cache of mcls, not required here*/
 }
 
 bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
-- 
1.9.1

[PATCH 3/4] android/health: Add some information messages

[Ravi kumar Veeramally]

---
 android/health.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/android/health.c b/android/health.c
index 361b043..e188683 100644
--- a/android/health.c
+++ b/android/health.c
@@ -1233,6 +1233,7 @@ static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data)
 		return;
 	}
 
+	info("health: MDL connected");
 	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_CONNECTED, fd);
 
 	return;
@@ -1245,7 +1246,7 @@ static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data)
 {
 	struct health_channel *channel = data;
 
-	info("MDL closed");
+	info("health: MDL closed");
 
 	if (!channel)
 		return;
@@ -1271,6 +1272,7 @@ static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data)
 	dev = channel->dev;
 
 	DBG("device %p channel %p mdl %p", dev, channel, mdl);
+	info("health: MDL deleted");
 
 	/* mdl == NULL means, delete all mdls */
 	if (!mdl) {
@@ -1822,7 +1824,7 @@ static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data)
 	if (!channel->dev->mcl)
 		channel->dev->mcl = mcap_mcl_ref(mcl);
 
-	info("MCL connected");
+	info("health: MCL connected");
 
 	ret = set_mcl_cb(channel->dev->mcl, channel, &gerr);
 	if (!ret) {
@@ -2048,6 +2050,7 @@ static void mcl_connected(struct mcap_mcl *mcl, gpointer data)
 
 	DBG("");
 
+	info("health: MCL connected");
 	ret = set_mcl_cb(mcl, NULL, &gerr);
 	if (!ret) {
 		error("health: error setting mcl callbacks: %s", gerr->message);
@@ -2061,6 +2064,7 @@ static void mcl_reconnected(struct mcap_mcl *mcl, gpointer data)
 
 	DBG("");
 
+	info("health: MCL reconnected");
 	dev = search_dev_by_mcl(mcl);
 	if (!dev) {
 		error("device data does not exists");
@@ -2074,6 +2078,7 @@ static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
 
 	DBG("");
 
+	info("health: MCL disconnected");
 	dev = search_dev_by_mcl(mcl);
 	mcap_mcl_unref(dev->mcl);
 	dev->mcl = NULL;
-- 
1.9.1

[PATCH 4/4] android/health: Simplify search_cb failure case

[Ravi kumar Veeramally]

---
 android/health.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/android/health.c b/android/health.c
index e188683..09b55ff 100644
--- a/android/health.c
+++ b/android/health.c
@@ -1877,10 +1877,7 @@ static void search_cb(sdp_list_t *recs, int err, gpointer data)
 	return;
 
 fail:
-	send_channel_state_notify(channel, HAL_HEALTH_CHANNEL_DESTROYED, -1);
-
-	queue_remove(channel->dev->channels, channel);
-	free_health_channel(channel);
+	destroy_channel(channel);
 }
 
 static int connect_mcl(struct health_channel *channel)
-- 
1.9.1

Re: [PATCH 1/4] android/health: Fix connect channel failure case

[Szymon Janc]

Hi Ravi,

On Monday 18 of August 2014 12:06:59 Ravi kumar Veeramally wrote:
> There is a possibilty of created channel is being added to queue,
> but on failure case it does channel free but not removed from queue.
> ---
>  android/health.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/android/health.c b/android/health.c
> index f4edc3e..f86fd26 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -1978,6 +1978,7 @@ static void bt_health_connect_channel(const void *buf,
> uint16_t len) return;
> 
>  fail:
> +	queue_remove(channel->dev->channels, channel);
>  	free_health_channel(channel);
>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
>  			HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED);

I've applied patches 1, 3 and 4. Thanks.

-- 
BR
Szymon Janc

Re: [PATCH 2/4] android/health: Remove mcl instance on mcl_disconnected call back

[Szymon Janc]

Hi Ravi,

On Monday 18 of August 2014 12:07:00 Ravi kumar Veeramally wrote:
> MCAP library is maintaining two sets of mcl list. On mcl_disconnected
> call mcl instance is removed from regular mcl list and cached in mcl_cache
> list. health.c doesn't maintain any cached mcls list. So mcl_conn variable
> doesn't make any sense of not being connected on mcl_disconnected call back.
> So unref mcl on mcl_disconnected and don't do anything in mcl_unached
> callback. ---
>  android/health.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/android/health.c b/android/health.c
> index f86fd26..361b043 100644
> --- a/android/health.c
> +++ b/android/health.c
> @@ -85,7 +85,6 @@ struct health_device {
>  	uint16_t app_id;
> 
>  	struct mcap_mcl *mcl;
> -	bool mcl_conn;
> 
>  	struct queue *channels;     /* data channels */
> 
> @@ -193,7 +192,6 @@ static void unref_mcl(struct health_device *dev)
>  	mcap_close_mcl(dev->mcl, FALSE);
>  	mcap_mcl_unref(dev->mcl);
>  	dev->mcl = NULL;
> -	dev->mcl_conn = false;
>  }
> 
>  static void free_health_device(void *data)
> @@ -1824,7 +1822,6 @@ static void create_mcl_cb(struct mcap_mcl *mcl, GError
> *err, gpointer data) if (!channel->dev->mcl)
>  		channel->dev->mcl = mcap_mcl_ref(mcl);
> 
> -	channel->dev->mcl_conn = true;
>  	info("MCL connected");
> 
>  	ret = set_mcl_cb(channel->dev->mcl, channel, &gerr);
> @@ -1949,7 +1946,7 @@ static void bt_health_connect_channel(const void *buf,
> uint16_t len) }
>  	}
> 
> -	if (!dev->mcl || (dev->mcl && !dev->mcl_conn)) {
> +	if (!dev->mcl) {
>  		if (connect_mcl(channel) < 0) {
>  			error("health: error retrieving HDP SDP record");
>  			goto fail;
> @@ -2069,8 +2066,6 @@ static void mcl_reconnected(struct mcap_mcl *mcl,
> gpointer data) error("device data does not exists");
>  		return;
>  	}
> -
> -	dev->mcl_conn = true;
>  }
> 
>  static void mcl_disconnected(struct mcap_mcl *mcl, gpointer data)
> @@ -2080,18 +2075,13 @@ static void mcl_disconnected(struct mcap_mcl *mcl,
> gpointer data) DBG("");
> 
>  	dev = search_dev_by_mcl(mcl);
> -	if (dev)
> -		dev->mcl_conn = false;
> +	mcap_mcl_unref(dev->mcl);
> +	dev->mcl = NULL;

So, previously dev was check for NULL before accessing it. Now it is not.
Is this always true that dev is not NULL here?

>  }
> 
>  static void mcl_uncached(struct mcap_mcl *mcl, gpointer data)
>  {
> -	struct health_device *dev;
> -
> -	DBG("");
> -
> -	dev = search_dev_by_mcl(mcl);
> -	free_health_device(dev);
> +	/* mcap library maintains cache of mcls, not required here*/
>  }
> 
>  bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t
> mode)

-- 
BR
Szymon Janc