Hi Michael,
On 18 December 2014 at 16:55, Michael Janssen <jamuraa@xxxxxxxxxx> wrote:
> Hi Lukasz:
>
> On Thu, Dec 18, 2014 at 5:46 AM, Lukasz Rymanowski
> <lukasz.rymanowski@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
