Hi Luiz,
On Wed, Oct 29, 2014 at 2:15 AM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Arman,
>
> On Wed, Oct 29, 2014 at 12:20 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>> Hi Luiz,
>>
>> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
>> <luiz.dentz@xxxxxxxxx> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>>>
>>> ---
>>> src/shared/gatt-db.c | 18 +++++++++++++++++-
>>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>> index f43cac3..b5b6c72 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -891,7 +891,23 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
>>> uint8_t opcode, bdaddr_t *bdaddr,
>>> gatt_db_attribute_read_t func, void *user_data)
>>> {
>>> - return false;
>>> + if (!attrib || !func)
>>> + return false;
>>> +
>>> + if (attrib->read_func) {
>>> + attrib->read_func(attrib->handle, offset, opcode, bdaddr,
>>> + attrib->user_data);
>>> + return true;
>>> + }
>>> +
>>> + /* Check length boundary if value is stored in the db */
>>> + if (offset >= attrib->value_len)
>>> + return false;
>>
>> Thanks for moving this after the callback check. As for valid
>> boundaries, I've actually seen devices that allow sending an offset
>> that is the same as the length of the value. They just return an empty
>> value PDU back. So maybe return NULL for the value if offset ==
>> attrib->value_len? What I had in mind was:
>
> But then this would be valid for any offset past value_len doesn't it
> or this is specific to determine the side of the value, in the other
> hand there does exist an error for invalid offset which I thought
> would be preferable, well if we can distinct this error which
> currently we do not because of the bool return.
>
So for your first point, the offset should be valid for the side of
the value only but invalid beyond that. E.g. the CSR uEnergy device I
have does the following:
offset == value_len - 1 ---> result is 1 byte (the last byte)
offset == value_len ---> result is 0 bytes
offset > value_len ---> result is ERROR_INVALID_OFFSET
I'm not exactly sure if the spec dictates that it be this way but this
is how I've seen several devices behave.
As for the bool return problem, maybe we can report the error in the
callback instead? When the read is being handled by read_func, we will
use the "err" argument gatt_db_attribute_read_t to report ATT errors
in the future, so maybe we should do the same here. If the offset is
invalid, we call func(attrib, BT_ATT_ERROR_INVALID_OFFSET, NULL, 0,
user_data); and return true.
>> if (offset > attrib->value_len)
>> return false;
>>
>> func(attrib, 0, (offset == attrib->value_len) ? NULL :
>> &attrib->value[offset], ...);
>>
>> return true;
>>
>> Then we would guard against the invalid access but the large offset
>> would still be a valid. I guess it really comes down to what we
>> consider to be a "valid" offset but I've seen implementations that do
>> this. Up to you.
>>
>>> +
>>> + func(attrib, 0, &attrib->value[offset], attrib->value_len - offset,
>>> + user_data);
>>> +
>>> + return true;
>>> }
>>>
>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> --
>>> 1.9.3
>>>
>>> --
>>> 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
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
--
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
