Return-Path: From: Szymon Janc To: Ravi kumar Veeramally Cc: linux-bluetooth@vger.kernel.org Subject: Re: [PATCH 2/6] android/health: Cache health application data on app register call Date: Mon, 16 Jun 2014 10:23:54 +0200 Message-ID: <1536603.hQuXxHqAUl@uw000953> In-Reply-To: <1402578618-31484-3-git-send-email-ravikumar.veeramally@linux.intel.com> References: <1402578618-31484-1-git-send-email-ravikumar.veeramally@linux.intel.com> <1402578618-31484-3-git-send-email-ravikumar.veeramally@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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. > + 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. > + > + app->app_name = malloc0(len); Check if allocation succeed. > + memcpy(app->app_name, cmd->data, len); > + off += len; > + > + if (cmd->provider_name_off) { You should be checking for length, not offset here. > + 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. > + } > + > + 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. > + > + 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); > + 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. > 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; > } > -- Best regards, Szymon Janc