Return-Path: Message-ID: <53A01D17.3080306@linux.intel.com> Date: Tue, 17 Jun 2014 13:48:55 +0300 From: Ravi kumar Veeramally MIME-Version: 1.0 To: Szymon Janc CC: linux-bluetooth@vger.kernel.org Subject: Re: [PATCH_v2 3/4] android/health: Initial connect channel implementation References: <1402956170-18356-1-git-send-email-ravikumar.veeramally@linux.intel.com> <1402956170-18356-4-git-send-email-ravikumar.veeramally@linux.intel.com> <1797512.LTblY0q3x1@uw000953> In-Reply-To: <1797512.LTblY0q3x1@uw000953> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Szymon, On 06/17/2014 01:16 PM, Szymon Janc wrote: > Hi Ravi, > > On Tuesday 17 of June 2014 01:02:49 Ravi kumar Veeramally wrote: >> Fetches remote sdp record and initiates MCL connection. >> --- >> android/health.c | 415 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 413 insertions(+), 2 deletions(-) >> >> diff --git a/android/health.c b/android/health.c >> index 5a87089..94154c3 100644 >> --- a/android/health.c >> +++ b/android/health.c >> @@ -36,9 +36,13 @@ >> #include "lib/bluetooth.h" >> #include "lib/sdp.h" >> #include "lib/sdp_lib.h" >> +#include "lib/uuid.h" >> +#include "lib/l2cap.h" >> #include "src/log.h" >> #include "src/shared/util.h" >> #include "src/shared/queue.h" >> +#include "src/uuid-helper.h" >> +#include "src/sdp-client.h" >> >> #include "hal-msg.h" >> #include "ipc-common.h" >> @@ -72,6 +76,28 @@ struct mdep_cfg { >> uint8_t id; /* mdep id */ >> }; >> >> +struct health_device { >> + bdaddr_t dst; >> + uint16_t app_id; >> + >> + struct mcap_mcl *mcl; >> + bool mcl_conn; >> + >> + struct queue *channels; /* data channels */ >> + >> + uint16_t ccpsm; >> + uint16_t dcpsm; >> +}; >> + >> +struct health_channel { >> + uint8_t mdep_id; >> + uint8_t type; >> + >> + struct health_device *dev; >> + >> + uint16_t id; /* channel id */ >> +}; >> + >> struct health_app { >> char *app_name; >> char *provider_name; >> @@ -81,8 +107,54 @@ struct health_app { >> struct queue *mdeps; >> >> uint16_t id; /* app id */ >> + struct queue *devices; >> }; >> >> +static void free_health_channel(void *data) >> +{ >> + struct health_channel *channel = data; >> + >> + if (!channel) >> + return; >> + >> + free(channel); >> +} >> + >> +static void destroy_channel(void *data) >> +{ >> + struct health_channel *channel = data; >> + >> + if (!channel) >> + return; >> + >> + /* TODO: Notify channel connection status DESTROYED */ >> + queue_remove(channel->dev->channels, channel); >> + free_health_channel(channel); >> +} >> + >> +static void unref_mcl(struct health_device *dev) >> +{ >> + if (!dev && !dev->mcl) > Shouldn't this be "if (!dev || !dev->mcl)" ? yes. > > >> + return; >> + >> + mcap_close_mcl(dev->mcl, FALSE); >> + mcap_mcl_unref(dev->mcl); >> + dev->mcl = NULL; >> + dev->mcl_conn = FALSE; > mlc_conn is bool, use 'false'. Ok. > >> +} >> + >> +static void free_health_device(void *data) >> +{ >> + struct health_device *dev = data; >> + >> + if (!dev) >> + return; >> + >> + unref_mcl(dev); >> + queue_destroy(dev->channels, free_health_channel); >> + free(dev); >> +} >> + >> static void free_mdep_cfg(void *data) >> { >> struct mdep_cfg *cfg = data; >> @@ -106,6 +178,7 @@ static void free_health_app(void *data) >> free(app->service_name); >> free(app->service_descr); >> queue_destroy(app->mdeps, free_mdep_cfg); >> + queue_destroy(app->devices, free_health_device); >> free(app); >> } >> >> @@ -130,6 +203,14 @@ static bool mdep_by_mdep_role(const void *data, const void *user_data) >> return mdep->role == role; >> } >> >> +static bool mdep_by_mdep_id(const void *data, const void *user_data) >> +{ >> + const struct mdep_cfg *mdep = data; >> + uint16_t mdep_id = PTR_TO_INT(user_data); >> + >> + return mdep->id == mdep_id; >> +} >> + >> static bool app_by_app_id(const void *data, const void *user_data) >> { >> const struct health_app *app = data; >> @@ -760,12 +841,342 @@ static void bt_health_unregister_app(const void *buf, uint16_t len) >> HAL_OP_HEALTH_UNREG_APP, HAL_STATUS_SUCCESS); >> } >> >> +static int get_prot_desc_entry(sdp_data_t *entry, int type, guint16 *val) >> +{ >> + sdp_data_t *iter; >> + int proto; >> + >> + if (!entry || !SDP_IS_SEQ(entry->dtd)) >> + return -1; >> + >> + iter = entry->val.dataseq; >> + if (!(iter->dtd & SDP_UUID_UNSPEC)) >> + return -1; > Add empty line here. Ok. > >> + proto = sdp_uuid_to_proto(&iter->val.uuid); >> + if (proto != type) >> + return -1; >> + >> + if (!val) >> + return 0; >> + >> + iter = iter->next; >> + if (iter->dtd != SDP_UINT16) >> + return -1; >> + >> + *val = iter->val.uint16; >> + >> + return 0; >> +} >> + >> +static int get_prot_desc_list(const sdp_record_t *rec, uint16_t *psm, >> + uint16_t *version) >> +{ >> + sdp_data_t *pdl, *p0, *p1; >> + >> + if (!psm && !version) >> + return -1; >> + >> + pdl = sdp_data_get(rec, SDP_ATTR_PROTO_DESC_LIST); >> + if (!pdl || !SDP_IS_SEQ(pdl->dtd)) >> + return -1; >> + >> + p0 = pdl->val.dataseq; >> + if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0) >> + return -1; >> + >> + p1 = p0->next; >> + if (get_prot_desc_entry(p1, MCAP_CTRL_UUID, version) < 0) >> + return -1; >> + >> + return 0; >> +} >> + >> +static int get_ccpsm(sdp_list_t *recs, uint16_t *ccpsm) >> +{ >> + sdp_list_t *l; >> + >> + for (l = recs; l; l = l->next) { >> + sdp_record_t *rec = l->data; >> + >> + if (!get_prot_desc_list(rec, ccpsm, NULL)) >> + return 0; >> + } >> + >> + return -1; >> +} >> + >> +static int get_add_prot_desc_list(const sdp_record_t *rec, uint16_t *psm) >> +{ >> + sdp_data_t *pdl, *p0, *p1; >> + >> + if (!psm) >> + return -1; >> + >> + pdl = sdp_data_get(rec, SDP_ATTR_ADD_PROTO_DESC_LIST); >> + if (!pdl || pdl->dtd != SDP_SEQ8) >> + return -1; >> + >> + pdl = pdl->val.dataseq; >> + if (pdl->dtd != SDP_SEQ8) >> + return -1; >> + >> + p0 = pdl->val.dataseq; >> + >> + if (get_prot_desc_entry(p0, L2CAP_UUID, psm) < 0) >> + return -1; >> + >> + p1 = p0->next; >> + if (get_prot_desc_entry(p1, MCAP_DATA_UUID, NULL) < 0) >> + return -1; >> + >> + return 0; >> +} >> + >> +static int get_dcpsm(sdp_list_t *recs, uint16_t *dcpsm) >> +{ >> + sdp_list_t *l; >> + >> + for (l = recs; l; l = l->next) { >> + sdp_record_t *rec = l->data; >> + >> + if (!get_add_prot_desc_list(rec, dcpsm)) >> + return 0; >> + } >> + >> + return -1; >> +} >> + >> +static void mcap_mdl_connected_cb(struct mcap_mdl *mdl, void *data) >> +{ >> + DBG("Not Implemeneted"); >> +} >> + >> +static void mcap_mdl_closed_cb(struct mcap_mdl *mdl, void *data) >> +{ >> + DBG("Not Implemeneted"); >> +} >> + >> +static void mcap_mdl_deleted_cb(struct mcap_mdl *mdl, void *data) >> +{ >> + DBG("Not Implemeneted"); >> +} >> + >> +static void mcap_mdl_aborted_cb(struct mcap_mdl *mdl, void *data) >> +{ >> + DBG("Not Implemeneted"); >> +} >> + >> +static void mcap_mdl_conn_req_cb(struct mcap_mcl *mcl, uint8_t mdepid, >> + uint16_t mdlid, uint8_t *conf, void *data) >> +{ >> + DBG("Not Implemeneted"); >> +} >> + >> +static void mcap_mdl_reconn_req_cb(struct mcap_mdl *mdl, void *data) >> +{ >> + DBG("Not Implemeneted"); >> +} >> + >> +static void create_mcl_cb(struct mcap_mcl *mcl, GError *err, gpointer data) >> +{ >> + struct health_channel *channel = data; >> + gboolean ret; >> + GError *gerr = NULL; >> + >> + DBG(""); >> + >> + if (err) { >> + error("error creating MCL : %s", err->message); >> + goto fail; >> + } >> + >> + if (!channel->dev->mcl) >> + channel->dev->mcl = mcap_mcl_ref(mcl); >> + >> + channel->dev->mcl_conn = TRUE; > bool, use 'true'. Ok. > >> + DBG("MCL connected"); >> + >> + ret = mcap_mcl_set_cb(channel->dev->mcl, channel, &gerr, >> + MCAP_MDL_CB_CONNECTED, mcap_mdl_connected_cb, >> + MCAP_MDL_CB_CLOSED, mcap_mdl_closed_cb, >> + MCAP_MDL_CB_DELETED, mcap_mdl_deleted_cb, >> + MCAP_MDL_CB_ABORTED, mcap_mdl_aborted_cb, >> + MCAP_MDL_CB_REMOTE_CONN_REQ, mcap_mdl_conn_req_cb, >> + MCAP_MDL_CB_REMOTE_RECONN_REQ, mcap_mdl_reconn_req_cb, >> + MCAP_MDL_CB_INVALID); >> + if (!ret) { >> + error("error setting mdl callbacks on mcl"); > gerr should be checked and freed here. ok. > >> + goto fail; >> + } >> + >> + /* TODO : create mdl */ >> + return; >> + >> +fail: >> + destroy_channel(channel); >> + >> + if (err) >> + g_error_free(err); > Are you sure this is correct? Wouldn't this error will be free by glib > when call returns? mcap library is not designed in that way. It sets error on error condition and returns. Not performing any cleanup operations like you mentioned. >> +} >> + >> +static void search_cb(sdp_list_t *recs, int err, gpointer data) >> +{ >> + struct health_channel *channel = data; >> + GError *gerr = NULL; >> + >> + DBG(""); >> + >> + if (err < 0 || !recs) { >> + error("Error getting remote SDP records"); >> + goto fail; >> + } >> + >> + if (get_ccpsm(recs, &channel->dev->ccpsm) < 0) { >> + error("Can't get remote PSM for control channel"); >> + goto fail; >> + } >> + >> + if (get_dcpsm(recs, &channel->dev->dcpsm) < 0) { >> + error("Can't get remote PSM for data channel"); >> + goto fail; >> + } >> + >> + if (!mcap_create_mcl(mcap, &channel->dev->dst, channel->dev->ccpsm, >> + create_mcl_cb, channel, NULL, &gerr)) { >> + error("error creating mcl %s", gerr->message); >> + goto fail; >> + } >> + >> + /* TODO: send channel state CONNECTING */ >> + return; >> + >> +fail: >> + /* TODO: send channel state DESTROYED*/ >> + >> + queue_remove(channel->dev->channels, channel); >> + free_health_channel(channel); >> + >> + if (gerr) >> + g_error_free(gerr); > I'd free error before jumping to fail: label. Ok. > >> +} >> + >> +static int connect_mcl(struct health_channel *channel) >> +{ >> + uuid_t uuid; >> + >> + DBG(""); >> + >> + bt_string2uuid(&uuid, HDP_UUID); >> + >> + return bt_search_service(&adapter_addr, &channel->dev->dst, &uuid, >> + search_cb, channel, NULL, 0); >> +} >> + >> +static struct health_device *create_device(uint16_t app_id, >> + const uint8_t addr[]) >> +{ >> + struct health_device *dev; >> + >> + dev = new0(struct health_device, 1); >> + if (!dev) >> + return NULL; >> + >> + android2bdaddr(&addr, &dev->dst); >> + dev->app_id = app_id; >> + >> + return dev; >> +} >> + >> +static struct health_channel *create_channel(uint16_t app_id, >> + uint8_t mdep_index) >> +{ >> + struct health_app *app; >> + struct mdep_cfg *mdep; >> + struct health_channel *channel; >> + uint8_t index; >> + static unsigned int channel_id = 1; >> + >> + app = queue_find(apps, app_by_app_id, INT_TO_PTR(app_id)); >> + if (!app) >> + return NULL; >> + >> + index = mdep_index + 1; >> + mdep = queue_find(app->mdeps, mdep_by_mdep_id, INT_TO_PTR(index)); >> + if (!mdep) >> + return NULL; >> + >> + channel = new0(struct health_channel, 1); >> + if (!channel) >> + return NULL; >> + >> + channel->mdep_id = mdep_index; >> + channel->type = mdep->channel_type; >> + channel->id = channel_id++; >> + >> + return channel; >> +} >> + >> static void bt_health_connect_channel(const void *buf, uint16_t len) >> { >> - DBG("Not implemented"); >> + const struct hal_cmd_health_connect_channel *cmd = buf; >> + struct hal_rsp_health_connect_channel rsp; >> + struct health_app *app; >> + struct health_device *dev = NULL; >> + struct health_channel *channel = NULL; >> + >> + DBG(""); >> >> + app = queue_find(apps, app_by_app_id, INT_TO_PTR(cmd->app_id)); >> + if (!app) >> + goto fail; >> + >> + dev = create_device(cmd->app_id, cmd->bdaddr); >> + if (!dev) >> + goto fail; > Should we check if device was already created ie if it is already > connected? Yeap, we should check. That is lot more stuff. Have to check if device is already exists, if exists if it has mcl, if mcl is connected, then first data channel is reliable or not. It is just an initial implementation, thing will done step by step. >> + >> + channel = create_channel(cmd->app_id, cmd->mdep_index); >> + if (!channel) >> + goto fail; >> + >> + channel->dev = dev; >> + >> + if (!app->devices) { >> + app->devices = queue_new(); >> + if (!app->devices) >> + goto fail; >> + } >> + >> + if (!queue_push_tail(app->devices, dev)) >> + goto fail; >> + >> + if (!dev->channels) { >> + dev->channels = queue_new(); >> + if (!dev->channels) >> + goto fail; >> + } >> + >> + if (!queue_push_tail(dev->channels, channel)) { >> + queue_remove(app->devices, dev); >> + goto fail; >> + } >> + >> + if (connect_mcl(channel) < 0) { >> + error("error retrieving HDP SDP record"); >> + queue_remove(app->devices, dev); >> + goto fail; >> + } >> + >> + rsp.channel_id = channel->id; >> + ipc_send_rsp_full(hal_ipc, HAL_SERVICE_ID_HEALTH, >> + HAL_OP_HEALTH_CONNECT_CHANNEL, >> + sizeof(rsp), &rsp, -1); >> + return; >> + >> +fail: >> + free_health_channel(channel); >> + free_health_device(dev); >> ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_HEALTH, >> - HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_UNSUPPORTED); >> + HAL_OP_HEALTH_CONNECT_CHANNEL, HAL_STATUS_FAILED); >> } >> >> static void bt_health_destroy_channel(const void *buf, uint16_t len) >> Thanks, Ravi.