Return-Path: MIME-Version: 1.0 In-Reply-To: <0B0F7F54-5DA6-4744-8694-1E798B5B7427@holtmann.org> References: <1418829975-17375-1-git-send-email-lukasz.rymanowski@tieto.com> <1418829975-17375-2-git-send-email-lukasz.rymanowski@tieto.com> <0B0F7F54-5DA6-4744-8694-1E798B5B7427@holtmann.org> Date: Wed, 17 Dec 2014 09:57:54 -0800 Message-ID: Subject: Re: [PATCH 1/2] attrib/gattrib: Add track for internal request id From: Michael Janssen To: Marcel Holtmann Cc: Lukasz Rymanowski , "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 List-ID: Hi Lukasz & Marcel, On Wed, Dec 17, 2014 at 7:44 AM, Marcel Holtmann wrote: > Hi Lukasz, > >> If user provides req_id to the g_attrib_send, he assume that req_id >> should be used for transaction. >> With this patch, gattrib keeps track on user requested req_id and >> actual pending req_id which allow to e.g. cancel correct transaction >> when user request it. >> --- >> attrib/gattrib.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 98 insertions(+), 2 deletions(-) >> >> diff --git a/attrib/gattrib.c b/attrib/gattrib.c >> index ab43f84..c75866b 100644 >> --- a/attrib/gattrib.c >> +++ b/attrib/gattrib.c >> @@ -51,10 +51,12 @@ struct _GAttrib { >> struct queue *callbacks; >> uint8_t *buf; >> int buflen; >> + struct queue *track_ids; >> }; >> >> >> struct attrib_callbacks { >> + unsigned int id; >> GAttribResultFunc result_func; >> GAttribNotifyFunc notify_func; >> GDestroyNotify destroy_func; >> @@ -63,6 +65,60 @@ struct attrib_callbacks { >> uint16_t notify_handle; >> }; >> >> +struct id_pair { >> + unsigned int org_id; >> + unsigned int pend_id; >> +}; >> + >> + >> +static bool find_with_pend_id(const void *data, const void *user_data) >> +{ >> + const struct id_pair *p = data; >> + unsigned int pending = PTR_TO_INT(user_data); > > PTR_TO_UINT. > >> + >> + return (p->pend_id == pending); >> +} >> + >> +static bool find_with_org_id(const void *data, const void *user_data) >> +{ >> + const struct id_pair *p = data; >> + unsigned int orig_id = PTR_TO_INT(user_data); > > PTR_TO_UINT. > >> + >> + return (p->org_id == orig_id); >> +} >> + >> +static void remove_stored_ids(GAttrib *attrib, unsigned int pending_id) >> +{ >> + struct id_pair *p; >> + >> + p = queue_remove_if(attrib->track_ids, find_with_pend_id, >> + INT_TO_PTR(pending_id)); > > UINT_TO_PTR. > >> + >> + free(p); >> +} >> + >> +static void store_id(GAttrib *attrib, unsigned int org_id, >> + unsigned int pend_id) >> +{ >> + struct id_pair *t; >> + >> + t = queue_find(attrib->track_ids, find_with_org_id, INT_TO_PTR(org_id)); > > UINT_TO_PTR. > >> + if (t) { >> + t->pend_id = pend_id; >> + return; >> + } >> + >> + t = new0(struct id_pair, 1); >> + if (!t) >> + return; >> + >> + t->org_id = org_id; >> + t->pend_id = pend_id; >> + >> + if (!queue_push_tail(attrib->track_ids, t)) >> + free(t); >> +} >> + >> GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu) >> { >> gint fd; >> @@ -95,6 +151,10 @@ GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu) >> if (!attr->callbacks) >> goto fail; >> >> + attr->track_ids = queue_new(); >> + if (!attr->track_ids) >> + goto fail; >> + >> return g_attrib_ref(attr); >> >> fail: >> @@ -153,6 +213,7 @@ void g_attrib_unref(GAttrib *attrib) >> bt_att_unref(attrib->att); >> >> queue_destroy(attrib->callbacks, attrib_callbacks_destroy); >> + queue_destroy(attrib->track_ids, free); >> >> free(attrib->buf); >> >> @@ -229,6 +290,8 @@ static void attrib_callback_result(uint8_t opcode, const void *pdu, >> if (cb->result_func) >> cb->result_func(status, buf, length + 1, cb->user_data); >> >> + remove_stored_ids(cb->parent, cb->id); >> + >> free(buf); >> } >> >> @@ -282,18 +345,49 @@ guint g_attrib_send(GAttrib *attrib, guint id, const guint8 *pdu, guint16 len, >> queue_push_head(attrib->callbacks, cb); >> response_cb = attrib_callback_result; >> destroy_cb = attrib_callbacks_remove; >> + >> } >> >> - return bt_att_send(attrib->att, pdu[0], (void *)pdu + 1, len - 1, >> + cb->id = bt_att_send(attrib->att, pdu[0], (void *) pdu + 1, len - 1, >> response_cb, cb, destroy_cb); >> + >> + if (id == 0) >> + return cb->id; >> + >> + /* >> + * If user what us to use given id, lets keep track on that so we give >> + * user a possibility to cancel ongoing request >> + */ >> + store_id(attrib, id, cb->id); >> + return id; >> +} >> + >> +static unsigned int get_id(GAttrib *attrib, guint id) >> +{ >> + struct id_pair *p; >> + unsigned int result = id; >> + >> + p = queue_remove_if(attrib->track_ids, find_with_org_id, >> + INT_TO_PTR(id)); > > UINT_TO_PTR. > >> + if (!p) >> + return id; >> + >> + result = p->pend_id; >> + free(p); >> + >> + return result; >> } >> >> gboolean g_attrib_cancel(GAttrib *attrib, guint id) >> { >> + unsigned int pend_id; >> + >> if (!attrib) >> return FALSE; >> >> - return bt_att_cancel(attrib->att, id); >> + pend_id = get_id(attrib, id); > > If you only have a single her for get_id, why not just include it in g_attrib_cancel. It is not that the wrap is by any means complex. > > Actually the get_id code is broken since it returns id and not error out if it is not found. Meaning you will try to cancel something else. And that might be a valid id from some user bt_att user. > >> + >> + return bt_att_cancel(attrib->att, pend_id); >> } >> >> gboolean g_attrib_cancel_all(GAttrib *attrib) >> @@ -301,6 +395,8 @@ gboolean g_attrib_cancel_all(GAttrib *attrib) >> if (!attrib) >> return FALSE; >> >> + queue_remove_all(attrib->track_ids, NULL, NULL, free); >> + >> return bt_att_cancel_all(attrib->att); > > And I think you need to replace this bt_att_cancel_all with just canceling the ids in the queue. Since otherwise you have the same problem as before. It could be just luck that g_attrib_cancel_all has only been used in cases where it does not matter. If this is meant to only cancel requests that initiated from g_attrib_send(), then you also need to track all ids returned from bt_att_send, not just the ones that are requested to have a specific tracking id. Now that the internal bt_att is retrievable, this is likely a good idea. -- Michael Janssen