Hi Lukasz, Michael,
On Thu, Dec 18, 2014 at 2:11 PM, Lukasz Rymanowski
<lukasz.rymanowski@xxxxxxxxx> wrote:
> 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
Yep, lets start with this since it is a temporary fix I don't think we
should stay too long on this, Im currently fixing a couple of things
since this breaks make check and will push this in a moment.
--
Luiz Augusto von Dentz
--
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
