2014-03-18 11:22:56

by Ravi kumar Veeramally

Subject: [PATCH_v4] android/hal-health: Add HDP .register_application method

---
v4: Added utility to copy null termiated strings to hal_string.
---
android/hal-health.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/android/hal-health.c b/android/hal-health.c
index e6fe65a..e7bb91c 100644
--- a/android/hal-health.c
+++ b/android/hal-health.c
@@ -28,6 +28,27 @@

static const bthl_callbacks_t *cbacks = NULL;

+static bool string_to_hal(const char *str, struct hal_string *hstr,
+ ssize_t *len)
+{
+ if (!hstr || !len)
+ return false;
+
+ if (!str) {
+ hstr->len = 0;
+ return true;
+ }
+
+ hstr->len = strlen(str) + 1;
+ *len += hstr->len;
+ if (*len > IPC_MTU)
+ return false;
+
+ memcpy(hstr->data, str, hstr->len);
+
+ return true;
+}
+
static bool interface_ready(void)
{
return cbacks != NULL;
@@ -38,6 +59,63 @@ static bool interface_ready(void)
static const struct hal_ipc_handler ev_handlers[] = {
};

+static bt_status_t register_application(bthl_reg_param_t *reg, int *app_id)
+{
+ char buf[IPC_MTU];
+ struct hal_cmd_health_reg_app *cmd = (void *) buf;
+ struct hal_rsp_health_reg_app rsp;
+ size_t len = sizeof(rsp);
+ bt_status_t status;
+ ssize_t cmd_len;
+ uint8_t i;
+
+ DBG("");
+
+ if (!interface_ready())
+ return BT_STATUS_NOT_READY;
+
+ if (!reg || !app_id)
+ return BT_STATUS_PARM_INVALID;
+
+ cmd_len = sizeof(*cmd);
+
+ if (!string_to_hal(reg->application_name, &cmd->app_name, &cmd_len))
+ return BT_STATUS_PARM_INVALID;
+
+ if (!string_to_hal(reg->provider_name, &cmd->provider_name, &cmd_len))
+ return BT_STATUS_PARM_INVALID;
+
+ if (!string_to_hal(reg->srv_name, &cmd->service_name, &cmd_len))
+ return BT_STATUS_PARM_INVALID;
+
+ if (!string_to_hal(reg->srv_desp, &cmd->service_descr, &cmd_len))
+ return BT_STATUS_PARM_INVALID;
+
+ cmd->num_of_mdep = reg->number_of_mdeps;
+
+ for (i = 0; i < reg->number_of_mdeps; i++) {
+ cmd_len += sizeof(cmd->mdep_cfg);
+ if (cmd_len > IPC_MTU)
+ return BT_STATUS_PARM_INVALID;
+
+ cmd->mdep_cfg[i].role = reg->mdep_cfg[i].mdep_role;
+ cmd->mdep_cfg[i].data_type = reg->mdep_cfg[i].data_type;
+ cmd->mdep_cfg[i].channel_type = reg->mdep_cfg[i].channel_type;
+
+ if (!string_to_hal(reg->mdep_cfg[i].mdep_description,
+ &cmd->mdep_cfg[i].descr, &cmd_len))
+ return BT_STATUS_PARM_INVALID;
+ }
+
+ status = hal_ipc_cmd(HAL_SERVICE_ID_HEALTH, HAL_OP_HEALTH_REG_APP,
+ cmd_len, cmd, &len, &rsp, NULL);
+
+ if (status == HAL_STATUS_SUCCESS)
+ *app_id = rsp.app_id;
+
+ return status;
+}
+
static bt_status_t unregister_application(int app_id)
{
struct hal_cmd_health_unreg_app cmd;
@@ -149,7 +227,7 @@ static void cleanup(void)
static bthl_interface_t health_if = {
.size = sizeof(health_if),
.init = init,
- .register_application = NULL,
+ .register_application = register_application,
.unregister_application = unregister_application,
.connect_channel = connect_channel,
.destroy_channel = destroy_channel,
--
1.8.3.2

2014-03-18 12:00:42

by Ravi kumar Veeramally

Subject: Re: [PATCH_v4] android/hal-health: Add HDP .register_application method

Hi Johan,

On 03/18/2014 01:48 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Mar 18, 2014, Ravi kumar Veeramally wrote:
>> +static bool string_to_hal(const char *str, struct hal_string *hstr,
>> + ssize_t *len)
>> +{
>> + if (!hstr || !len)
>> + return false;
>> +
>> + if (!str) {
>> + hstr->len = 0;
>> + return true;
>> + }
>> +
>> + hstr->len = strlen(str) + 1;
>> + *len += hstr->len;
>> + if (*len > IPC_MTU)
>> + return false;
> Usually we design APIs so that a failure means the return parameters
> were left untouched. The way you've implemented this here the caller
> couldn't know if *len or hstr->len were modified in case false is
> returned. We should be more predictable than that. So please fix up the
> function so that it only touches the parameters if true is returned.
>
Ok, I'll fix that.

Thanks,
Ravi.

2014-03-18 11:48:22

by Johan Hedberg

Subject: Re: [PATCH_v4] android/hal-health: Add HDP .register_application method

Hi Ravi,

On Tue, Mar 18, 2014, Ravi kumar Veeramally wrote:
> +static bool string_to_hal(const char *str, struct hal_string *hstr,
> + ssize_t *len)
> +{
> + if (!hstr || !len)
> + return false;
> +
> + if (!str) {
> + hstr->len = 0;
> + return true;
> + }
> +
> + hstr->len = strlen(str) + 1;
> + *len += hstr->len;
> + if (*len > IPC_MTU)
> + return false;

Usually we design APIs so that a failure means the return parameters
were left untouched. The way you've implemented this here the caller
couldn't know if *len or hstr->len were modified in case false is
returned. We should be more predictable than that. So please fix up the
function so that it only touches the parameters if true is returned.

Johan