Hi Vinicius,
On 12/13/2010 12:33 PM, Vinicius Costa Gomes wrote:
On 11:56 Mon 13 Dec, Brian Gix wrote:
[trimmed]
>
>>>> +
>>>> + if (ret_val != 0)
>>>> + return;
>>>> +
>>>> + status = ATT_ECODE_INSUFF_RESOURCES;
>>>> +
>>>> +done:
>>>> + long_read->func(status, res_pdu, res_len, long_read->user_data);
>>>
>>> If g_attrib_send() fails, load_read->result_data may be leaking here.
Is there no guarantee that the GAttribResultFunc parameter will be
invoked if there is a non-Zero return value from g_attrib_send?
If there is paths that could result in a non-response (abnormal link
termination?) then you are correct, and I will need to rethink clean-up.
The case I was referring was when g_attrib_send() cannot allocate memory for
the command.
I don't think this is an issue, because if g_attrib_send() cannot
allocate memory, then it will return a 0, and fall through to the
completion and clean-up. However, I like the idea of doing all clean-up
in the destructor anyway, which resolves the memory leak potential.
[...]
Also:
If we intend to use the ID of the (original) command to cancel the GATT
procedure, I propose the following:
Compound GATT procedure such as the Read Long procedure that I am
implementing, should save the original ID returned by g_attrib_send in
the nested user_data structure such as I have done. When subsequent ATT
commands are sent as part of the same GATT procedure, the new command
shall be assigned the same ID as the original. This could be done with
a function in gattrib.c with the prototype:
g_attrib_set_id(GAttrib *attrib, guint old_id, guint new_id);
This would allow the upper layer to cancel the entire GATT procedure,
even if it is partially completed.
This same methodology could then be applied to long writes, and other
compound GATT procedures.
What do you think?
Sounds good, I just don't know about messing with the commands id's,
maybe having some thing like command groups.
For example, adding these funtions:
guint g_attrib_send_full(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
guint16 len, guint group, GAttribResultFunc func,
gpointer user_data, GDestroyNotify notify);
gboolean g_attrib_cancel_group(GAttrib *attrib, guint group);
How do this look?
My biggest objection would be that to go this route, I think we would
want to extend this "command group" to all GATT procedures, and not just
the ones known or suspected to be Compound. I don't think that
canceling a "Characteristic Value Read" procedure should be any
different (or use a different parameter type) than canceling "Find
Primary Service", for example.
Do you think everything in gatt.c should use these "command groups"?
And if so, by what mechanism would the "group number" remain unique?
It doesn't seem to me to be such a horrible overloading of the id. I do
see that when a cancel is called, that it is important that only one
command with that ID is in the queue. However, this reclaimation/reuse
of the old ID would only occur after the prior instance is in the
process of being destroyed. The danger I suppose would be the misuse of
this renumbering capability by an entity other than gatt.c.
I'm sorry this is getting long winded BUT:
This also highlights the inherent danger of allowing multiple Requests
to be queued up. If are to allow a Read-Value, Write-Value, Read-Value
sequence of commands to be enqueued onto the attrib->queue, it will be
impossible to correctly implement any of the compound Read or Write
commands, because the subsequent "Read-Blob" would be added to the end
of the queue, and the read could be split around a write. GATT/ATT was
always intended to be a single outstanding request (and procedure) at a
time kind of protocol, and having a FIFO attrib->queue subverts that intent.
To get compound GATT procedures to work properly (while maintaining the
Attrib queuing mechanism) we may need to restructure gattrib.c such that
we can add the subsequent commands to the *head* of the queue, and not
call wake_up_sender in the received_data() function until after the
cmd->func() has been invoked, and given the opportunity to "continue the
current procedure".
Cheers,
--
Brian Gix
bgix@xxxxxxxxxxxxxx
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
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
