Re: [PATCH 2/6] android/health: Cache health application data on app register call

[Ravi kumar Veeramally <ravikumar.veeramally@linux.intel.com>]

Hi Szymon,

On 06/16/2014 11:23 AM, Szymon Janc wrote:
> Hi Ravi,
>
> On Thursday 12 of June 2014 16:10:14 Ravi kumar Veeramally wrote:
>> ---
>>   android/health.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 182 insertions(+), 6 deletions(-)
>>
>> diff --git a/android/health.c b/android/health.c
>> index 655d9f9..a117390 100644
>> --- a/android/health.c
>> +++ b/android/health.c
>> @@ -35,6 +35,8 @@
>>   #include "lib/sdp.h"
>>   #include "lib/sdp_lib.h"
>>   #include "src/log.h"
>> +#include "src/shared/util.h"
>> +#include "src/shared/queue.h"
>>   
>>   #include "hal-msg.h"
>>   #include "ipc-common.h"
>> @@ -45,21 +47,193 @@
>>   
>>   static bdaddr_t adapter_addr;
>>   static struct ipc *hal_ipc = NULL;
>> +static struct queue *apps = NULL;
>>   
>> -static void bt_health_register_app(const void *buf, uint16_t len)
>> +struct mdep_cfg {
>> +	uint8_t role;
>> +	uint16_t data_type;
>> +	uint8_t channel_type;
>> +	char *descr;
>> +
>> +	uint8_t id; /* mdep id */
>> +};
>> +
>> +struct health_app {
>> +	char *app_name;
>> +	char *provider_name;
>> +	char *service_name;
>> +	char *service_descr;
>> +	uint8_t num_of_mdep;
>> +	struct queue *mdeps;
>> +
>> +	uint8_t id; /* app id */
>> +};
>> +
>> +static void free_mdep_cfg(void *data)
>>   {
>> -	DBG("Not implemented");
>> +	struct mdep_cfg *cfg = data;
>>   
>> -	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_REG_APP,
>> -							HAL_STATUS_UNSUPPORTED);
>> +	if (!cfg)
>> +		return;
>> +
>> +	free(cfg->descr);
>> +	free(cfg);
>> +}
>> +
>> +static void free_health_app(void *data)
>> +{
>> +	struct health_app *app = data;
>> +
>> +	if (!app)
>> +		return;
>> +
>> +	free(app->app_name);
>> +	free(app->provider_name);
>> +	free(app->service_name);
>> +	free(app->service_descr);
>> +	queue_destroy(app->mdeps, free_mdep_cfg);
>> +	free(app);
>> +}
>> +
>> +static bool app_by_app_id(const void *data, const void *user_data)
>> +{
>> +	const struct health_app *app = data;
>> +	uint16_t app_id = PTR_TO_INT(user_data);
>> +
>> +	return app->id == app_id;
>> +}
>> +
>> +static void bt_health_register_app(const void *buf, uint16_t buf_len)
>> +{
>> +	const struct hal_cmd_health_reg_app *cmd = buf;
>> +	struct hal_rsp_health_reg_app rsp;
>> +	struct health_app *app;
>> +	uint16_t off, len;
>> +
>> +	DBG("");
>> +
>> +	app = new0(struct health_app, 1);
> Check if allocation succeed.
  Ok.
>
>> +	app->id = queue_length(apps) + 1;
>> +	app->num_of_mdep = cmd->num_of_mdep;
>> +
>> +	off = 0;
>> +
>> +	if (cmd->provider_name_off)
>> +		len = cmd->provider_name_off;
>> +	else if (cmd->service_name_off)
>> +		len = cmd->service_name_off;
>> +	else if (cmd->service_descr_off)
>> +		len = cmd->service_descr_off;
>> +	else
>> +		len = cmd->len;
> Offsets can be only increased so length is always next offset - current offset
> ie.
>
> app_name_len = provider_name_off - app_name_off
> provider_name_len = service_name_off - provider_name_off;
> etc.
>
> Also how about factoring this to helper ie. create_health_app() ?
> struct health_app *create_health_app(const uint8_t *app_name, uint16_t app_len,
>                                       const uint8_t *provider_name, uint16_t provider_len, ...., int mdeps)
>
> and then just call it like:
>
> app_name = cmd->data + cmd->app_name_off;
> app_len = cmd->provider_name_off - cmd->app_name_off;
> ....
>
> app = create_health_app(app_name, app_len, ....);
> if (!app) {
>      status = HAL_STATUS_FAILED;
>      goto failed;
> }
>
> This should make code more readable.
  Ok, make sense.
>
>> +
>> +	app->app_name = malloc0(len);
> Check if allocation succeed.
  Ok.
>> +	memcpy(app->app_name, cmd->data, len);
>> +	off += len;
>> +
>> +	if (cmd->provider_name_off) {
> You should be checking for length, not offset here.
  Ok.
>
>> +		if (cmd->service_name_off)
>> +			len = cmd->service_name_off - cmd->provider_name_off;
>> +		else if (cmd->service_descr_off)
>> +			len = cmd->service_descr_off - cmd->provider_name_off;
>> +		else
>> +			len = cmd->len - cmd->provider_name_off;
>> +
>> +		app->provider_name = malloc0(len);
>> +		memcpy(app->provider_name, cmd->data + off, len);
>> +		off += len;
>> +	} else {
>> +		app->provider_name = NULL;
> Else is not needed as struct is already zeroed.
   Ok.
>> +	}
>> +
>> +	if (cmd->service_name_off) {
>> +		if (cmd->service_descr_off)
>> +			len = cmd->service_descr_off - cmd->service_name_off;
>> +		else
>> +			len = cmd->len - cmd->service_name_off;
>> +
>> +		app->service_name = malloc0(len);
>> +		memcpy(app->service_name, cmd->data + off, len);
>> +		off += len;
>> +	} else {
>> +		app->service_name = NULL;
>> +	}
>> +
>> +	if (cmd->service_descr_off) {
>> +		len = cmd->len - cmd->service_descr_off;
>> +
>> +		app->service_descr = malloc0(len);
>> +		memcpy(app->service_descr, cmd->data + off, len);
>> +		off += len;
>> +	} else {
>> +		app->service_descr = NULL;
>> +	}
>> +
>> +	if (app->num_of_mdep > 0)
>> +		app->mdeps = queue_new();
> I'd always allocate queue for sanity. It will just be empty if no mdeps.
  Ok.
>
>> +
>> +	rsp.app_id = app->id;
>> +
>> +	if (!queue_push_tail(apps, app)) {
>> +		free_health_app(app);
>> +		ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH,
>> +				HAL_OP_HEALTH_REG_APP, HAL_STATUS_FAILED);
>> +		return;
>> +	}
>> +
>> +	ipc_send_rsp_full(hal_ipc, HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_REG_APP,
>> +							sizeof(rsp), &rsp, -1);
>>   }
>>   
>>   static void bt_health_mdep_cfg_data(const void *buf, uint16_t len)
>>   {
>> -	DBG("Not implemented");
>> +	const struct hal_cmd_health_mdep *cmd = buf;
>> +	struct health_app *app;
>> +	struct mdep_cfg *mdep;
>> +	uint8_t status;
>> +
>> +	DBG("");
>> +
>> +	app = queue_find(apps, app_by_app_id, INT_TO_PTR(cmd->app_id));
>> +	if (!app) {
>> +		status = HAL_STATUS_INVALID;
>> +		goto fail;
>> +	}
>> +
>> +	mdep = new0(struct mdep_cfg, 1);
>> +	mdep->role = cmd->role;
>> +	mdep->data_type = cmd->data_type;
>> +	mdep->channel_type = cmd->channel_type;
>> +	mdep->id = queue_length(app->mdeps) + 1;
>>   
>> +	if (cmd->descr_len > 0) {
>> +		mdep->descr = malloc0(cmd->descr_len);
>> +		memcpy(mdep->descr, cmd->descr, cmd->descr_len);
>> +	}
>> +
>> +	if (!queue_push_tail(app->mdeps, mdep)) {
>> +		free_mdep_cfg(mdep);
>> +		status = HAL_STATUS_FAILED;
>> +		goto fail;
>> +	}
>> +
>> +	if (app->num_of_mdep != queue_length(app->mdeps)) {
>> +		status = HAL_STATUS_SUCCESS;
>> +		goto end;
>> +	}
>> +
>> +	/* TODO: Create MCAP instance and prepare SDP profile */
>> +	status = HAL_STATUS_SUCCESS;
>> +
>> +fail:
>> +	if (status != HAL_STATUS_SUCCESS) {
> Don't do that. If this is failed label make it handle fail case only.
> ie
>
> ipc_send_rsp(.., SUCCESS);
> return;
>
> failed:
>   clean_ip();
>   ips_send_rsp(..., status);
>
  Further code is in coming patches, I thought it would be good to put
TODO comments. Anyway I will change in next version.
>> +		queue_remove(apps, app);
>> +		free_health_app(app);
>> +	}
>> +
>> +end:
>>   	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_MDEP,
>> -							HAL_STATUS_UNSUPPORTED);
>> +								status);
>>   }
>>   
>>   static void bt_health_unregister_app(const void *buf, uint16_t len)
>> @@ -111,6 +285,7 @@ bool bt_health_register(struct ipc *ipc, const bdaddr_t *addr, uint8_t mode)
>>   	bacpy(&adapter_addr, addr);
>>   
>>   	hal_ipc = ipc;
>> +	apps = queue_new();
> For sanity, you should check if this succeed.
  Ok.
>
>>   	ipc_register(hal_ipc, HAL_SERVICE_ID_HEALTH, cmd_handlers,
>>   						G_N_ELEMENTS(cmd_handlers));
>>   
>> @@ -121,6 +296,7 @@ void bt_health_unregister(void)
>>   {
>>   	DBG("");
>>   
>> +	queue_destroy(apps, free_health_app);
>>   	ipc_unregister(hal_ipc, HAL_SERVICE_ID_HEALTH);
>>   	hal_ipc = NULL;
>>   }
>>
  Thanks,
  Ravi.