Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1418910367-19792-1-git-send-email-lukasz.rymanowski@tieto.com> <1418910367-19792-4-git-send-email-lukasz.rymanowski@tieto.com> Date: Thu, 18 Dec 2014 17:11:30 +0100 Message-ID: Subject: Re: [PATCH v2 3/3] attrib/gattrib: Add tracking all the internal request id From: Lukasz Rymanowski To: Michael Janssen Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Michael, On 18 December 2014 at 16:55, Michael Janssen wrote: > Hi Lukasz: > > On Thu, Dec 18, 2014 at 5:46 AM, Lukasz Rymanowski > wrote: >> With this patch gattrib track all the pending request id's to bt_att. >> When doing g_attrib_cancel_all, only those own by gattrib will be >> canceled. >> --- >> attrib/gattrib.c | 95 ++++++++++++++++++++++++-------------------------------- >> 1 file changed, 41 insertions(+), 54 deletions(-) >> >> diff --git a/attrib/gattrib.c b/attrib/gattrib.c >> index f986a77..5709d84 100644 >> --- a/attrib/gattrib.c >> +++ b/attrib/gattrib.c >> @@ -54,9 +54,13 @@ struct _GAttrib { >> struct queue *track_ids; >> }; >> >> +struct id_pair { >> + unsigned int org_id; >> + unsigned int pend_id; >> +}; >> >> struct attrib_callbacks { >> - unsigned int id; >> + struct id_pair *id; >> GAttribResultFunc result_func; >> GAttribNotifyFunc notify_func; >> GDestroyNotify destroy_func; >> @@ -65,20 +69,6 @@ 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_UINT(user_data); >> - >> - return (p->pend_id == pending); >> -} >> - >> static bool find_with_org_id(const void *data, const void *user_data) >> { >> const struct id_pair *p = data; >> @@ -87,37 +77,22 @@ static bool find_with_org_id(const void *data, const void *user_data) >> 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, >> - UINT_TO_PTR(pending_id)); >> - >> - free(p); >> -} >> - >> -static void store_id(GAttrib *attrib, unsigned int org_id, >> +static struct id_pair *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, >> - UINT_TO_PTR(org_id)); >> - if (t) { >> - t->pend_id = pend_id; >> - return; >> - } >> - >> t = new0(struct id_pair, 1); >> if (!t) >> - return; >> + return NULL; >> >> t->org_id = org_id; >> t->pend_id = pend_id; >> >> - if (!queue_push_tail(attrib->track_ids, t)) >> - free(t); >> + if (queue_push_tail(attrib->track_ids, t)) >> + return t; >> + >> + return NULL; >> } >> >> GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu) >> @@ -185,6 +160,9 @@ static void attrib_callbacks_destroy(void *data) >> if (cb->destroy_func) >> cb->destroy_func(cb->user_data); >> >> + if (queue_remove(cb->parent->track_ids, cb->id)) >> + free(cb->id); >> + >> free(data); >> } >> >> @@ -291,8 +269,6 @@ 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); >> } >> >> @@ -328,6 +304,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, const guint8 *pdu, guint16 len, >> struct attrib_callbacks *cb = NULL; >> bt_att_response_func_t response_cb = NULL; >> bt_att_destroy_func_t destroy_cb = NULL; >> + unsigned int pend_id; >> >> if (!attrib) >> return 0; >> @@ -349,32 +326,36 @@ guint g_attrib_send(GAttrib *attrib, guint id, const guint8 *pdu, guint16 len, >> >> } >> >> - cb->id = bt_att_send(attrib->att, pdu[0], (void *) pdu + 1, len - 1, >> + pend_id = bt_att_send(attrib->att, pdu[0], (void *) pdu + 1, len - 1, >> response_cb, cb, destroy_cb); >> >> - if (id == 0) >> - return cb->id; >> + /* >> + * We store here pair as it is easier to handle it in response and in >> + * case where user request us to use specific id request - see below. >> + */ >> + if (id == 0) { >> + cb->id = store_id(attrib, pend_id, pend_id); >> + return pend_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); >> + cb->id = store_id(attrib, id, pend_id); >> return id; >> } >> >> gboolean g_attrib_cancel(GAttrib *attrib, guint id) >> { >> struct id_pair *p; >> - unsigned int pend_id; >> >> if (!attrib) >> return FALSE; >> >> /* >> - * Let's try to find actual pending request id on the tracking id queue. >> - * If there is no such it means it is not tracked id and we can cancel >> - * it. >> + * If request belongs to gattrib and is not yet done it has to be on >> + * the tracking id queue >> * >> * FIXME: It can happen that on the queue there is id_pair with >> * given id which was provided by the user. In the same time it might >> @@ -386,14 +367,18 @@ gboolean g_attrib_cancel(GAttrib *attrib, guint id) >> */ >> p = queue_remove_if(attrib->track_ids, find_with_org_id, >> UINT_TO_PTR(id)); >> - if (p) { >> - pend_id = p->pend_id; >> - free(p); >> - } else { >> - pend_id = id; >> - } >> + if (!p) >> + return FALSE; >> >> - return bt_att_cancel(attrib->att, pend_id); >> + return bt_att_cancel(attrib->att, p->pend_id); >> +} >> + >> +static void cancel_request(void *data, void *user_data) >> +{ >> + struct id_pair *p = data; >> + GAttrib *attrib = user_data; >> + >> + bt_att_cancel(attrib->att, p->pend_id); >> } >> >> gboolean g_attrib_cancel_all(GAttrib *attrib) >> @@ -401,9 +386,11 @@ gboolean g_attrib_cancel_all(GAttrib *attrib) >> if (!attrib) >> return FALSE; >> >> + /* Cancel only request which belongs to gattrib */ >> + queue_foreach(attrib->track_ids, cancel_request, attrib); >> queue_remove_all(attrib->track_ids, NULL, NULL, free); >> >> - return bt_att_cancel_all(attrib->att); >> + return TRUE; >> } >> >> guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle, >> -- >> 1.8.4 > > I think you can eliminate a lot of memory management, the additional > queue and more by putting the pend_id and org_id both into > attrib_callbacks and then keeping all the pending requests, regardless > of whether they have a callback function, in the attrib->callbacks > queue. Yes, maybe I would, but in this shape it is more clear and I would stick to it if you agree. Especially when user call g_attrib_cancel or cancel all it makes more sens to look into id_tracks than callbacks->xx \Lukasz > > The request can be cancelled in attrib_callbacks_destroy then. > > -- > Michael Janssen