Re: [PATCH 1/1] Support for reading long Characteristic Values.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux