Return-Path: Message-ID: <539EB741.9030800@linux.intel.com> Date: Mon, 16 Jun 2014 12:22:09 +0300 From: Ravi kumar Veeramally MIME-Version: 1.0 To: Szymon Janc CC: linux-bluetooth@vger.kernel.org Subject: Re: [PATCH 2/6] android/health: Cache health application data on app register call References: <1402578618-31484-1-git-send-email-ravikumar.veeramally@linux.intel.com> <1402578618-31484-3-git-send-email-ravikumar.veeramally@linux.intel.com> <1536603.hQuXxHqAUl@uw000953> In-Reply-To: <1536603.hQuXxHqAUl@uw000953> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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.