From: Luiz Augusto von Dentz <[email protected]>
This is the new API to reduce the lookups in gatt_db and make it
a little bit more convenient for batch operations, so the general idea
is to be able to get a hold of it via gatt_db_get_attribute but also
replace the handles in the queues with proper attributes so the server
code don't have to lookup again when reading/writing, checking
permissions, or any other operation that can be done directly.
---
v2: Address problems gatt_db_attribute_read and gatt_db_attribute_write
pointed out by Arman.
src/shared/gatt-db.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
src/shared/gatt-db.h | 33 +++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 65c5759..19784d1 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -802,3 +802,52 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
return true;
}
+
+struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
+ uint16_t handle)
+{
+ return NULL;
+}
+
+uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
+{
+ return 0;
+}
+
+uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
+{
+ return 0;
+}
+
+const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
+{
+ return NULL;
+}
+
+bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
+ bt_uuid_t *uuid)
+{
+ return false;
+}
+
+bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
+ uint32_t *permissions)
+{
+ return false;
+}
+
+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;
+}
+
+bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
+ const uint8_t *value, size_t len,
+ uint8_t opcode, bdaddr_t *bdaddr,
+ gatt_db_attribute_write_t func,
+ void *user_data)
+{
+ return false;
+}
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 8d18434..0c16e88 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -96,3 +96,36 @@ bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
uint32_t *permissions);
+
+struct gatt_db_attribute;
+
+struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
+ uint16_t handle);
+
+uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib);
+uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib);
+
+const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib);
+
+bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
+ bt_uuid_t *uuid);
+
+bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
+ uint32_t *permissions);
+
+typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
+ int err, uint8_t *value, size_t length,
+ void *user_data);
+
+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);
+
+typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
+ int err, void *user_data);
+
+bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
+ const uint8_t *value, size_t len,
+ uint8_t opcode, bdaddr_t *bdaddr,
+ gatt_db_attribute_write_t func,
+ void *user_data);
--
1.9.3
Hi Arman,
On Wed, Oct 29, 2014 at 4:59 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Wed, Oct 29, 2014 at 2:15 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Arman,
>>
>> On Wed, Oct 29, 2014 at 12:20 AM, Arman Uguray <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> ---
>>>> 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.
Fair enough, I will fix it in v3.
--
Luiz Augusto von Dentz
Hi Luiz,
On Wed, Oct 29, 2014 at 8:54 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Wed, Oct 29, 2014 at 5:09 PM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> This is the new API to reduce the lookups in gatt_db and make it
>>> a little bit more convenient for batch operations, so the general idea
>>> is to be able to get a hold of it via gatt_db_get_attribute but also
>>> replace the handles in the queues with proper attributes so the server
>>> code don't have to lookup again when reading/writing, checking
>>> permissions, or any other operation that can be done directly.
>>> ---
>>> v2: Address problems gatt_db_attribute_read and gatt_db_attribute_write
>>> pointed out by Arman.
>>>
>>> src/shared/gatt-db.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> src/shared/gatt-db.h | 33 +++++++++++++++++++++++++++++++++
>>> 2 files changed, 82 insertions(+)
>>>
>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>> index 65c5759..19784d1 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -802,3 +802,52 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
>>> return true;
>>>
>>> }
>>> +
>>> +struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
>>> + uint16_t handle)
>>> +{
>>> + return NULL;
>>> +}
>>> +
>>> +uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
>>> +{
>>> + return NULL;
>>> +}
>>> +
>>> +bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
>>> + bt_uuid_t *uuid)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> +bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>>> + uint32_t *permissions)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> +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;
>>> +}
>>> +
>>> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> + const uint8_t *value, size_t len,
>>> + uint8_t opcode, bdaddr_t *bdaddr,
>>> + gatt_db_attribute_write_t func,
>>> + void *user_data)
>>> +{
>>> + return false;
>>> +}
>>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>>> index 8d18434..0c16e88 100644
>>> --- a/src/shared/gatt-db.h
>>> +++ b/src/shared/gatt-db.h
>>> @@ -96,3 +96,36 @@ bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
>>>
>>> bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
>>> uint32_t *permissions);
>>> +
>>> +struct gatt_db_attribute;
>>> +
>>> +struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
>>> + uint16_t handle);
>>> +
>>> +uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib);
>>> +uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib);
>>> +
>>> +const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib);
>>> +
>>> +bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
>>> + bt_uuid_t *uuid);
>>> +
>>> +bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>>> + uint32_t *permissions);
>>> +
>>> +typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
>>> + int err, uint8_t *value, size_t length,
>>> + void *user_data);
>>
>> Should the "err" argument here and in gatt_db_attribute_write_it be
>> uint8_t instead of int? The pre-defined ATT protocol errors as well as
>> profile/application defined errors are reported in one byte in an
>> error response. What would we need a larger or a negative value for?
>
> Usually we use negative errno to report errors and then have a central
> point converting them to native protocol, but perhaps we have to give
> the possibility to provide the ATT errors directly here, which is kind
> of a stack violation if you ask me, but Android does seems to be doing
> it by expecting the application to respond with proper error code, so
> perhaps we do both and treat positive returns as ATT error code and
> negative as errno.
>
Sounds good to me.
>>> +
>>> +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);
>>> +
>>> +typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>>> + int err, void *user_data);
>>> +
>>> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> + const uint8_t *value, size_t len,
>>> + uint8_t opcode, bdaddr_t *bdaddr,
>>> + gatt_db_attribute_write_t func,
>>> + void *user_data);
>>> --
>>> 1.9.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
Hi Arman,
On Wed, Oct 29, 2014 at 5:09 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> This is the new API to reduce the lookups in gatt_db and make it
>> a little bit more convenient for batch operations, so the general idea
>> is to be able to get a hold of it via gatt_db_get_attribute but also
>> replace the handles in the queues with proper attributes so the server
>> code don't have to lookup again when reading/writing, checking
>> permissions, or any other operation that can be done directly.
>> ---
>> v2: Address problems gatt_db_attribute_read and gatt_db_attribute_write
>> pointed out by Arman.
>>
>> src/shared/gatt-db.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>> src/shared/gatt-db.h | 33 +++++++++++++++++++++++++++++++++
>> 2 files changed, 82 insertions(+)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 65c5759..19784d1 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -802,3 +802,52 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
>> return true;
>>
>> }
>> +
>> +struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
>> + uint16_t handle)
>> +{
>> + return NULL;
>> +}
>> +
>> +uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
>> +{
>> + return 0;
>> +}
>> +
>> +uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
>> +{
>> + return 0;
>> +}
>> +
>> +const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
>> +{
>> + return NULL;
>> +}
>> +
>> +bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
>> + bt_uuid_t *uuid)
>> +{
>> + return false;
>> +}
>> +
>> +bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>> + uint32_t *permissions)
>> +{
>> + return false;
>> +}
>> +
>> +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;
>> +}
>> +
>> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> + const uint8_t *value, size_t len,
>> + uint8_t opcode, bdaddr_t *bdaddr,
>> + gatt_db_attribute_write_t func,
>> + void *user_data)
>> +{
>> + return false;
>> +}
>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>> index 8d18434..0c16e88 100644
>> --- a/src/shared/gatt-db.h
>> +++ b/src/shared/gatt-db.h
>> @@ -96,3 +96,36 @@ bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
>>
>> bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
>> uint32_t *permissions);
>> +
>> +struct gatt_db_attribute;
>> +
>> +struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
>> + uint16_t handle);
>> +
>> +uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib);
>> +uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib);
>> +
>> +const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib);
>> +
>> +bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
>> + bt_uuid_t *uuid);
>> +
>> +bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>> + uint32_t *permissions);
>> +
>> +typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
>> + int err, uint8_t *value, size_t length,
>> + void *user_data);
>
> Should the "err" argument here and in gatt_db_attribute_write_it be
> uint8_t instead of int? The pre-defined ATT protocol errors as well as
> profile/application defined errors are reported in one byte in an
> error response. What would we need a larger or a negative value for?
Usually we use negative errno to report errors and then have a central
point converting them to native protocol, but perhaps we have to give
the possibility to provide the ATT errors directly here, which is kind
of a stack violation if you ask me, but Android does seems to be doing
it by expecting the application to respond with proper error code, so
perhaps we do both and treat positive returns as ATT error code and
negative as errno.
>> +
>> +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);
>> +
>> +typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>> + int err, void *user_data);
>> +
>> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> + const uint8_t *value, size_t len,
>> + uint8_t opcode, bdaddr_t *bdaddr,
>> + gatt_db_attribute_write_t func,
>> + void *user_data);
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Luiz,
On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This is the new API to reduce the lookups in gatt_db and make it
> a little bit more convenient for batch operations, so the general idea
> is to be able to get a hold of it via gatt_db_get_attribute but also
> replace the handles in the queues with proper attributes so the server
> code don't have to lookup again when reading/writing, checking
> permissions, or any other operation that can be done directly.
> ---
> v2: Address problems gatt_db_attribute_read and gatt_db_attribute_write
> pointed out by Arman.
>
> src/shared/gatt-db.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/gatt-db.h | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 82 insertions(+)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 65c5759..19784d1 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -802,3 +802,52 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
> return true;
>
> }
> +
> +struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
> + uint16_t handle)
> +{
> + return NULL;
> +}
> +
> +uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
> +{
> + return 0;
> +}
> +
> +uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
> +{
> + return 0;
> +}
> +
> +const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
> +{
> + return NULL;
> +}
> +
> +bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
> + bt_uuid_t *uuid)
> +{
> + return false;
> +}
> +
> +bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
> + uint32_t *permissions)
> +{
> + return false;
> +}
> +
> +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;
> +}
> +
> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> + const uint8_t *value, size_t len,
> + uint8_t opcode, bdaddr_t *bdaddr,
> + gatt_db_attribute_write_t func,
> + void *user_data)
> +{
> + return false;
> +}
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 8d18434..0c16e88 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -96,3 +96,36 @@ bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
>
> bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
> uint32_t *permissions);
> +
> +struct gatt_db_attribute;
> +
> +struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
> + uint16_t handle);
> +
> +uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib);
> +uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib);
> +
> +const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib);
> +
> +bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
> + bt_uuid_t *uuid);
> +
> +bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
> + uint32_t *permissions);
> +
> +typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
> + int err, uint8_t *value, size_t length,
> + void *user_data);
Should the "err" argument here and in gatt_db_attribute_write_it be
uint8_t instead of int? The pre-defined ATT protocol errors as well as
profile/application defined errors are reported in one byte in an
error response. What would we need a larger or a negative value for?
> +
> +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);
> +
> +typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
> + int err, void *user_data);
> +
> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> + const uint8_t *value, size_t len,
> + uint8_t opcode, bdaddr_t *bdaddr,
> + gatt_db_attribute_write_t func,
> + void *user_data);
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
Hi Luiz,
On Wed, Oct 29, 2014 at 7:41 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Wed, Oct 29, 2014 at 12:30 AM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> ---
>>> src/shared/gatt-db.c | 32 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>> index b5b6c72..5a912f8 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -900,7 +900,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
>>> return true;
>>> }
>>>
>>> - /* Check length boundary if value is stored in the db */
>>> + /* Check boundary if value is stored in the db */
>>> if (offset >= attrib->value_len)
>>> return false;
>>>
>>> @@ -916,5 +916,33 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> gatt_db_attribute_write_t func,
>>> void *user_data)
>>> {
>>> - return false;
>>> + if (!attrib)
>>> + return false;
>>> +
>>> + if (attrib->write_func) {
>>> + attrib->write_func(attrib->handle, offset, value, len, opcode,
>>> + bdaddr, attrib->user_data);
>>> + return true;
>>> + }
>>> +
>>> + /* For values stored in db allocate on demand */
>>> + if (!attrib->value) {
>>> + attrib->value = malloc0(len + offset);
>>> + if (!attrib->value)
>>> + return false;
>>> + attrib->value_len = len + offset;
>>> + } else if (offset >= attrib->value_len ||
>>> + len > (unsigned) (attrib->value_len - offset)) {
>>> + attrib->value = realloc(attrib->value, len + offset);
>>
>> I wouldn't immediately modify attrib->value here. Use a temporary, so
>> that if realloc fails, the write will fail but the attribute value
>> will remain unmodified.
>
> Well realloc would still affect the pointer anyway, and I guess if it
> fails it would return the original pointer and nothing would be change
> so the return is only really useful if the original pointer is NULL
> then it acts as malloc.
>
I see, sounds good then.
>>> + if (!attrib->value)
>>> + return 0;
>>
>> return false, to be consistent with the previous block.
>
> Will fix this.
>
>>> + attrib->value_len = len + offset;
>>> + }
>>> +
>>> + memcpy(&attrib->value[offset], value, len);
>>
>> I'm still not sure about allowing writes if attrib->write_func is
>> NULL. My thinking is, if the write_func is not set, then this
>> attribute is not writable so we always return false. This is
>> automatically the case for all declaration attributes since they
>> wouldn't have write permission. All other writes will then always be
>> delegated to the upper layer. This would keep things a bit simpler I
>> think, especially since we wouldn't want some accidental usage of
>> gatt_db_attribute_write on a declaration handle to break the database.
>
> The db has no idea of permissions bits to begin with so it cannot
> check consistency between permissions and callbacks, and I do remember
> commenting that this allows to store values in the db itself, which is
> very convenient for plugins, this is also valid for read, any plugin
> should be able to read values stored in the db, so at in terms of API
> it is consistent to allow writes without a callback.
>
Fair enough. In this case it looks like we are making the values be
variable length as the db's default behavior, which is fine with me.
We should probably later start documenting these things some place
though.
>>> +
>>> + if (func)
>>> + func(attrib, 0, user_data);
>>> +
>>> + return true;
>>> }
>>> --
>>> 1.9.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
Hi Luiz,
On Wed, Oct 29, 2014 at 2:15 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Wed, Oct 29, 2014 at 12:20 AM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> ---
>>> 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 [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
Hi Luiz,
On Wed, Oct 29, 2014 at 1:50 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Wed, Oct 29, 2014 at 12:12 AM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> ---
>>> src/shared/gatt-db.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>> index 9a3f864..f43cac3 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -879,7 +879,12 @@ bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
>>> bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>>> uint32_t *permissions)
>>> {
>>> - return false;
>>> + if (!attrib || !permissions)
>>> + return false;
>>
>> So, based on my understanding, android/gatt treats an attribute
>> permission value of "0" as "this is a declaration attribute since we
>> didn't explicitly set it". Hence, returning 0 here will break things
>> in the android code once it starts using the gatt_db_attribute_*
>> functions.
>
> The check here is for the permissions pointer passed to the function
> to receive the value, if it is NULL it is probably a misuse of the
> API.
>
Oops, I misread the code there, so there's nothing wrong here.
>> That said, I don't like this logic one bit in the first place. Instead
>> of having "0" mean "anything goes" and having a special define for "no
>> permissions" (such as GATT_PERM_NONE from android/gatt and
>> BT_GATT_PERM from shared/att-types), we should just treat 0 to mean no
>> permissions. I.e. we should define BT_GATT_PERM_NONE as 0, and have
>> gatt_db_add_service, _add_characteristic, etc. explicitly set at least
>> the BT_GATT_PERM_READ permission on the declaration attributes.
>
> I guess you are confusing this code with some ideas I have in the past
> about doing the permissions check in the db itself, this is no longer
> the case since Vinicius pointed out this wouldn't be possible with the
> current design of the db and I remember you also agreeing that the
> permissions should be checked in the server or somewhere else, not
> sure why you got the impression this would do anything with the
> permissions, it just read what permissions have been attached to the
> attribute nothing else.
>
Sorry I misread the check above, so there is nothing wrong with this
function as it is. As for permission checks, there's no confusion, I
still think bt_gatt_server should perform those. My issue in general
(and this is not specifically related to this patch) is having a
permission value of "0" mean "no permission set, this must be a
declaration". I think eventually we should explicitly set the
permissions of declaration attributes to BT_GATT_PERM READ inside
functions like gatt_db_add_service.
>> In short we should unify the permission story for shared/gatt and
>> android/gatt sooner then later.
>
> That I guess is bt_gatt_server business since that is per connection
> it should check what permissions the db has stored and perform the
> necessary checks, so the defines and everything else related to
> permissions so probably be in bt_gatt_server.
>
You're right, the reason that I mentioned this here is because storing
the correct permission values is still gatt_db's job, even if
bt_gatt_server performs the checks while handling requests. There is
nothing wrong with this patch though.
>>> +
>>> + *permissions = attrib->permissions;
>>> +
>>> + return true;
>>> }
>>>
>>> bool gatt_db_attribute_read(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 [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
Hi Arman,
On Wed, Oct 29, 2014 at 12:30 AM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> ---
>> src/shared/gatt-db.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index b5b6c72..5a912f8 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -900,7 +900,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
>> return true;
>> }
>>
>> - /* Check length boundary if value is stored in the db */
>> + /* Check boundary if value is stored in the db */
>> if (offset >= attrib->value_len)
>> return false;
>>
>> @@ -916,5 +916,33 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> gatt_db_attribute_write_t func,
>> void *user_data)
>> {
>> - return false;
>> + if (!attrib)
>> + return false;
>> +
>> + if (attrib->write_func) {
>> + attrib->write_func(attrib->handle, offset, value, len, opcode,
>> + bdaddr, attrib->user_data);
>> + return true;
>> + }
>> +
>> + /* For values stored in db allocate on demand */
>> + if (!attrib->value) {
>> + attrib->value = malloc0(len + offset);
>> + if (!attrib->value)
>> + return false;
>> + attrib->value_len = len + offset;
>> + } else if (offset >= attrib->value_len ||
>> + len > (unsigned) (attrib->value_len - offset)) {
>> + attrib->value = realloc(attrib->value, len + offset);
>
> I wouldn't immediately modify attrib->value here. Use a temporary, so
> that if realloc fails, the write will fail but the attribute value
> will remain unmodified.
Well realloc would still affect the pointer anyway, and I guess if it
fails it would return the original pointer and nothing would be change
so the return is only really useful if the original pointer is NULL
then it acts as malloc.
>> + if (!attrib->value)
>> + return 0;
>
> return false, to be consistent with the previous block.
Will fix this.
>> + attrib->value_len = len + offset;
>> + }
>> +
>> + memcpy(&attrib->value[offset], value, len);
>
> I'm still not sure about allowing writes if attrib->write_func is
> NULL. My thinking is, if the write_func is not set, then this
> attribute is not writable so we always return false. This is
> automatically the case for all declaration attributes since they
> wouldn't have write permission. All other writes will then always be
> delegated to the upper layer. This would keep things a bit simpler I
> think, especially since we wouldn't want some accidental usage of
> gatt_db_attribute_write on a declaration handle to break the database.
The db has no idea of permissions bits to begin with so it cannot
check consistency between permissions and callbacks, and I do remember
commenting that this allows to store values in the db itself, which is
very convenient for plugins, this is also valid for read, any plugin
should be able to read values stored in the db, so at in terms of API
it is consistent to allow writes without a callback.
>> +
>> + if (func)
>> + func(attrib, 0, user_data);
>> +
>> + return true;
>> }
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Arman,
On Wed, Oct 29, 2014 at 12:20 AM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> ---
>> 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.
> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Arman,
On Wed, Oct 29, 2014 at 12:12 AM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> ---
>> src/shared/gatt-db.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 9a3f864..f43cac3 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -879,7 +879,12 @@ bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
>> bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>> uint32_t *permissions)
>> {
>> - return false;
>> + if (!attrib || !permissions)
>> + return false;
>
> So, based on my understanding, android/gatt treats an attribute
> permission value of "0" as "this is a declaration attribute since we
> didn't explicitly set it". Hence, returning 0 here will break things
> in the android code once it starts using the gatt_db_attribute_*
> functions.
The check here is for the permissions pointer passed to the function
to receive the value, if it is NULL it is probably a misuse of the
API.
> That said, I don't like this logic one bit in the first place. Instead
> of having "0" mean "anything goes" and having a special define for "no
> permissions" (such as GATT_PERM_NONE from android/gatt and
> BT_GATT_PERM from shared/att-types), we should just treat 0 to mean no
> permissions. I.e. we should define BT_GATT_PERM_NONE as 0, and have
> gatt_db_add_service, _add_characteristic, etc. explicitly set at least
> the BT_GATT_PERM_READ permission on the declaration attributes.
I guess you are confusing this code with some ideas I have in the past
about doing the permissions check in the db itself, this is no longer
the case since Vinicius pointed out this wouldn't be possible with the
current design of the db and I remember you also agreeing that the
permissions should be checked in the server or somewhere else, not
sure why you got the impression this would do anything with the
permissions, it just read what permissions have been attached to the
attribute nothing else.
> In short we should unify the permission story for shared/gatt and
> android/gatt sooner then later.
That I guess is bt_gatt_server business since that is per connection
it should check what permissions the db has stored and perform the
necessary checks, so the defines and everything else related to
permissions so probably be in bt_gatt_server.
>> +
>> + *permissions = attrib->permissions;
>> +
>> + return true;
>> }
>>
>> bool gatt_db_attribute_read(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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Arman,
On Tue, Oct 28, 2014 at 11:59 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> ---
>> src/shared/gatt-db.c | 20 +++++++++++++++++++-
>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 19784d1..bfc7bfa 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -806,7 +806,25 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
>> struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
>> uint16_t handle)
>> {
>> - return NULL;
>> + struct gatt_db_service *service;
>> + uint16_t service_handle;
>> +
>> + if (!db || !handle)
>> + return NULL;
>> +
>> + service = queue_find(db->services, find_service_for_handle,
>> + INT_TO_PTR(handle));
>
> Shouldn't this be UINT_TO_PTR?
Yep, apparently it was broken in the original code as well, I will fix them.
>
>> + if (!service)
>> + return NULL;
>> +
>> + service_handle = service->attributes[0]->handle;
>> +
>> + /*
>> + * We can safely get attribute from attributes array with offset,
>> + * because find_service_for_handle() check if given handle is
>> + * in service range.
>> + */
>> + return service->attributes[handle - service_handle];
>> }
>>
>> uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Luiz,
On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> ---
> src/shared/gatt-db.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index b5b6c72..5a912f8 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -900,7 +900,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> return true;
> }
>
> - /* Check length boundary if value is stored in the db */
> + /* Check boundary if value is stored in the db */
> if (offset >= attrib->value_len)
> return false;
>
> @@ -916,5 +916,33 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> gatt_db_attribute_write_t func,
> void *user_data)
> {
> - return false;
> + if (!attrib)
> + return false;
> +
> + if (attrib->write_func) {
> + attrib->write_func(attrib->handle, offset, value, len, opcode,
> + bdaddr, attrib->user_data);
> + return true;
> + }
> +
> + /* For values stored in db allocate on demand */
> + if (!attrib->value) {
> + attrib->value = malloc0(len + offset);
> + if (!attrib->value)
> + return false;
> + attrib->value_len = len + offset;
> + } else if (offset >= attrib->value_len ||
> + len > (unsigned) (attrib->value_len - offset)) {
> + attrib->value = realloc(attrib->value, len + offset);
I wouldn't immediately modify attrib->value here. Use a temporary, so
that if realloc fails, the write will fail but the attribute value
will remain unmodified.
> + if (!attrib->value)
> + return 0;
return false, to be consistent with the previous block.
> + attrib->value_len = len + offset;
> + }
> +
> + memcpy(&attrib->value[offset], value, len);
I'm still not sure about allowing writes if attrib->write_func is
NULL. My thinking is, if the write_func is not set, then this
attribute is not writable so we always return false. This is
automatically the case for all declaration attributes since they
wouldn't have write permission. All other writes will then always be
delegated to the upper layer. This would keep things a bit simpler I
think, especially since we wouldn't want some accidental usage of
gatt_db_attribute_write on a declaration handle to break the database.
> +
> + if (func)
> + func(attrib, 0, user_data);
> +
> + return true;
> }
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
Hi Luiz,
On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> ---
> 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:
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
Hi Luiz,
On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> ---
> src/shared/gatt-db.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 9a3f864..f43cac3 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -879,7 +879,12 @@ bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
> bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
> uint32_t *permissions)
> {
> - return false;
> + if (!attrib || !permissions)
> + return false;
So, based on my understanding, android/gatt treats an attribute
permission value of "0" as "this is a declaration attribute since we
didn't explicitly set it". Hence, returning 0 here will break things
in the android code once it starts using the gatt_db_attribute_*
functions.
That said, I don't like this logic one bit in the first place. Instead
of having "0" mean "anything goes" and having a special define for "no
permissions" (such as GATT_PERM_NONE from android/gatt and
BT_GATT_PERM from shared/att-types), we should just treat 0 to mean no
permissions. I.e. we should define BT_GATT_PERM_NONE as 0, and have
gatt_db_add_service, _add_characteristic, etc. explicitly set at least
the BT_GATT_PERM_READ permission on the declaration attributes.
In short we should unify the permission story for shared/gatt and
android/gatt sooner then later.
> +
> + *permissions = attrib->permissions;
> +
> + return true;
> }
>
> bool gatt_db_attribute_read(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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
Hi Luiz,
On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> ---
> src/shared/gatt-db.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 19784d1..bfc7bfa 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -806,7 +806,25 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
> struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
> uint16_t handle)
> {
> - return NULL;
> + struct gatt_db_service *service;
> + uint16_t service_handle;
> +
> + if (!db || !handle)
> + return NULL;
> +
> + service = queue_find(db->services, find_service_for_handle,
> + INT_TO_PTR(handle));
Shouldn't this be UINT_TO_PTR?
> + if (!service)
> + return NULL;
> +
> + service_handle = service->attributes[0]->handle;
> +
> + /*
> + * We can safely get attribute from attributes array with offset,
> + * because find_service_for_handle() check if given handle is
> + * in service range.
> + */
> + return service->attributes[handle - service_handle];
> }
>
> uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 9a3f864..f43cac3 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -879,7 +879,12 @@ bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
uint32_t *permissions)
{
- return false;
+ if (!attrib || !permissions)
+ return false;
+
+ *permissions = attrib->permissions;
+
+ return true;
}
bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
--
1.9.3
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 41 +++++++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index bbd8c9c..9a3f864 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -46,6 +46,7 @@ struct gatt_db {
};
struct gatt_db_attribute {
+ struct gatt_db_service *service;
uint16_t handle;
bt_uuid_t uuid;
uint32_t permissions;
@@ -70,7 +71,8 @@ static bool match_service_by_handle(const void *data, const void *user_data)
return service->attributes[0]->handle == PTR_TO_INT(user_data);
}
-static struct gatt_db_attribute *new_attribute(const bt_uuid_t *type,
+static struct gatt_db_attribute *new_attribute(struct gatt_db_service *service,
+ const bt_uuid_t *type,
const uint8_t *val,
uint16_t len)
{
@@ -80,6 +82,7 @@ static struct gatt_db_attribute *new_attribute(const bt_uuid_t *type,
if (!attribute)
return NULL;
+ attribute->service = service;
attribute->uuid = *type;
attribute->value_len = len;
if (len) {
@@ -187,7 +190,7 @@ uint16_t gatt_db_add_service(struct gatt_db *db, const bt_uuid_t *uuid,
len = uuid_to_le(uuid, value);
- service->attributes[0] = new_attribute(type, value, len);
+ service->attributes[0] = new_attribute(service, type, value, len);
if (!service->attributes[0]) {
gatt_db_service_destroy(service);
return 0;
@@ -297,14 +300,14 @@ uint16_t gatt_db_add_characteristic(struct gatt_db *db, uint16_t handle,
len += sizeof(uint16_t);
len += uuid_to_le(uuid, &value[3]);
- service->attributes[i] = new_attribute(&characteristic_uuid, value,
- len);
+ service->attributes[i] = new_attribute(service, &characteristic_uuid,
+ value, len);
if (!service->attributes[i])
return 0;
update_attribute_handle(service, i++);
- service->attributes[i] = new_attribute(uuid, NULL, 0);
+ service->attributes[i] = new_attribute(service, uuid, NULL, 0);
if (!service->attributes[i]) {
free(service->attributes[i - 1]);
return 0;
@@ -335,7 +338,7 @@ uint16_t gatt_db_add_char_descriptor(struct gatt_db *db, uint16_t handle,
if (!i)
return 0;
- service->attributes[i] = new_attribute(uuid, NULL, 0);
+ service->attributes[i] = new_attribute(service, uuid, NULL, 0);
if (!service->attributes[i])
return 0;
@@ -385,8 +388,9 @@ uint16_t gatt_db_add_included_service(struct gatt_db *db, uint16_t handle,
if (!index)
return 0;
- service->attributes[index] = new_attribute(&included_service_uuid,
- value, len);
+ service->attributes[index] = new_attribute(service,
+ &included_service_uuid,
+ value, len);
if (!service->attributes[index])
return 0;
@@ -848,6 +852,27 @@ const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
bt_uuid_t *uuid)
{
+ if (!attrib)
+ return false;
+
+ if (attrib->value_len == sizeof(uint16_t)) {
+ uint16_t value;
+
+ value = get_le16(attrib->value);
+ bt_uuid16_create(uuid, value);
+
+ return true;
+ }
+
+ if (attrib->value_len == sizeof(uint128_t)) {
+ uint128_t value;
+
+ bswap_128(attrib->value, &value);
+ bt_uuid128_create(uuid, value);
+
+ return true;
+ }
+
return false;
}
--
1.9.3
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index b5b6c72..5a912f8 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -900,7 +900,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
return true;
}
- /* Check length boundary if value is stored in the db */
+ /* Check boundary if value is stored in the db */
if (offset >= attrib->value_len)
return false;
@@ -916,5 +916,33 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
gatt_db_attribute_write_t func,
void *user_data)
{
- return false;
+ if (!attrib)
+ return false;
+
+ if (attrib->write_func) {
+ attrib->write_func(attrib->handle, offset, value, len, opcode,
+ bdaddr, attrib->user_data);
+ return true;
+ }
+
+ /* For values stored in db allocate on demand */
+ if (!attrib->value) {
+ attrib->value = malloc0(len + offset);
+ if (!attrib->value)
+ return false;
+ attrib->value_len = len + offset;
+ } else if (offset >= attrib->value_len ||
+ len > (unsigned) (attrib->value_len - offset)) {
+ attrib->value = realloc(attrib->value, len + offset);
+ if (!attrib->value)
+ return 0;
+ attrib->value_len = len + offset;
+ }
+
+ memcpy(&attrib->value[offset], value, len);
+
+ if (func)
+ func(attrib, 0, user_data);
+
+ return true;
}
--
1.9.3
From: Luiz Augusto von Dentz <[email protected]>
---
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;
+
+ 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
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 99fe849..e371f1b 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -841,7 +841,10 @@ uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
{
- return 0;
+ if (!attrib)
+ return 0;
+
+ return attrib->handle + attrib->service->num_handles - 1;
}
const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
--
1.9.3
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 5a912f8..99fe849 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -833,7 +833,10 @@ struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
{
- return 0;
+ if (!attrib)
+ return 0;
+
+ return attrib->handle;
}
uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
--
1.9.3
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index bfc7bfa..bbd8c9c 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -839,7 +839,10 @@ uint16_t gatt_db_attribute_get_end_handle(struct gatt_db_attribute *attrib)
const bt_uuid_t *gatt_db_attribute_get_type(struct gatt_db_attribute *attrib)
{
- return NULL;
+ if (!attrib)
+ return NULL;
+
+ return &attrib->uuid;
}
bool gatt_db_attribute_get_service_uuid(struct gatt_db_attribute *attrib,
--
1.9.3
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 19784d1..bfc7bfa 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -806,7 +806,25 @@ bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
uint16_t handle)
{
- return NULL;
+ struct gatt_db_service *service;
+ uint16_t service_handle;
+
+ if (!db || !handle)
+ return NULL;
+
+ service = queue_find(db->services, find_service_for_handle,
+ INT_TO_PTR(handle));
+ if (!service)
+ return NULL;
+
+ service_handle = service->attributes[0]->handle;
+
+ /*
+ * We can safely get attribute from attributes array with offset,
+ * because find_service_for_handle() check if given handle is
+ * in service range.
+ */
+ return service->attributes[handle - service_handle];
}
uint16_t gatt_db_attribute_get_start_handle(struct gatt_db_attribute *attrib)
--
1.9.3