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.
---
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 b3f95d2..75fdff2 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -793,3 +793,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 Luiz,
On Tue, Oct 28, 2014 at 9:11 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Mon, Oct 27, 2014 at 8:57 PM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Mon, Oct 27, 2014 at 9:04 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.
>>> ---
>>> 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 b3f95d2..75fdff2 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -793,3 +793,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);
>>
>> So we currently have gatt_db_read_t and gatt_db_write_t, which are
>> stored with the attribute, and gatt_db_attribute_read_t and
>> gatt_db_attribute_write_t which are used to report the result of a
>> read or write. I think this is a bit confusing. Should we call these
>> something like gatt_db_read_complete_t or gatt_db_read_result_func_t
>> perhaps?
>
> Confusing in which way? We should probably use gatt_db_attribute
> prefix anyway and it looked too big to use complete so I just went
> with the shortest term I could find.
>
What I found confusing was that both types of callbacks are named
*_read_t and *_write_t yet they achieve different things. It's OK if
you want to leave them as is, I just prefer names to be a bit more
descriptive in the absence of explicit documentation :)
>> Also, won't we also have to change gatt_db_read_t and gatt_db_write_t
>> signatures to accept the result functions? If the read/write is being
>> handled by a callback, then they should report the result back using
>> the new callbacks. This way the code can be uniform regardless of
>> whether the result was obtained directly from the database or was
>> delegated to a callback (or at least this is what I was trying to do
>> in the patch set I sent out last week).
>
> Yep, that is the second part of these changes, but I would like to
> first get gatt_db_attribute API done because changing gatt_db_read_t
> and gatt_db_write_t will cause quite a lot of other changes which make
> it harder to review the details if everything is in a single
> patch-set.
>
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 Mon, Oct 27, 2014 at 8:57 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
> On Mon, Oct 27, 2014 at 9:04 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.
>> ---
>> 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 b3f95d2..75fdff2 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -793,3 +793,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);
>
> So we currently have gatt_db_read_t and gatt_db_write_t, which are
> stored with the attribute, and gatt_db_attribute_read_t and
> gatt_db_attribute_write_t which are used to report the result of a
> read or write. I think this is a bit confusing. Should we call these
> something like gatt_db_read_complete_t or gatt_db_read_result_func_t
> perhaps?
Confusing in which way? We should probably use gatt_db_attribute
prefix anyway and it looked too big to use complete so I just went
with the shortest term I could find.
> Also, won't we also have to change gatt_db_read_t and gatt_db_write_t
> signatures to accept the result functions? If the read/write is being
> handled by a callback, then they should report the result back using
> the new callbacks. This way the code can be uniform regardless of
> whether the result was obtained directly from the database or was
> delegated to a callback (or at least this is what I was trying to do
> in the patch set I sent out last week).
Yep, that is the second part of these changes, but I would like to
first get gatt_db_attribute API done because changing gatt_db_read_t
and gatt_db_write_t will cause quite a lot of other changes which make
it harder to review the details if everything is in a single
patch-set.
>> +
>> +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 Arman,
On Mon, Oct 27, 2014 at 8:46 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>>
>> ---
>> src/shared/gatt-db.c | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 9ead5e3..acd2e9a 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -882,7 +882,22 @@ 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 (offset > attrib->value_len)
>> + return false;
>> +
>> + if (attrib->read_func) {
>> + attrib->read_func(attrib->handle, offset, opcode, bdaddr,
>> + attrib->user_data);
>> + return true;
>> + }
>> +
>> + func(attrib, 0, &attrib->value[offset], attrib->value_len - offset,
>> + user_data);
>
> Wouldn't this potentially cause an invalid access if value_len ==
> offset? I would just do a check here and pass NULL to func if that's
> the case.
Yep, thanks to point it out, I actually pushed the boundary check
after the callback check if the value is not stored in the db itself.
>> +
>> + return true;
>> }
>>
>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> --
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Arman,
On Mon, Oct 27, 2014 at 8:43 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>
>>
>> ---
>> src/shared/gatt-db.c | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index acd2e9a..4ff7358 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -906,5 +906,20 @@ 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 (offset > attrib->value_len ||
>> + len > (unsigned) (attrib->value_len - offset))
>> + return false;
>
> Won't this incorrectly fail if the attribute value has a variable
> length? Quoted from Core v4.1 Vol 3 ATT: 3.4.5.1 Write Request: "If
> the attribute value has a variable length, then the attribute value
> shall be truncated or lengthened to match the length of the Attribute
> Value parameter". So we can't really make assumptions on value_len
> here.
True, the these check cannot be done here except if we store the value
and have some means to identify it use a variable length but I guess
we can leave this for later once we have a better understanding how
this API works in practice.
> Also, if the attribute value is being implemented by the higher layer,
> then won't the value_len field of the attribute structure be invalid
> in general? Unless we always cache the full value in the attribute
> structure based on the last time gatt-db invoked the read callback but
> then this would still incorrectly fail if we never read the attribute
> value beforehand.
I guess the idea here would be to store locally if no write callback
was provided, but you are right regarding value_len it cannot be used
like that, at least not without having some way to check that it is
valid.
> I think we can assume that all attributes that are created without a
> write callback are not writable. I think we should always delegate
> writes to a callback and just return false here if the callback was
> never set (this would mean that the attribute is not writable). The
> callback can perform the necessary offset and length checks as needed
> by the service it's implementing, so we wouldn't end up making any
> assumptions there.
Well this would make it read and write not consistent with each other
since on for read the callback is optional, btw I though about using
gatt_db_attribute_write exactly to fill values locally stored in the
db so read without callback would use it and if Im not mistaken we
perform the permissions check before calling write on Android so I
guess bt_gatt_server can do the same, we just need to make sure the
permissions used are the same so we can check them in bt_gatt_server
before reaches the callback with the possibility to bypass these
checks by not providing any permissions.
>> +
>> + if (attrib->write_func) {
>> + attrib->write_func(attrib->handle, offset, value, len, opcode,
>> + bdaddr, attrib->user_data);
>> + return true;
>> + }
>> +
>> + memcpy(&attrib->value[offset], value, len);
>> +
>> + return true;
>> }
>> --
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Luiz,
On Mon, Oct 27, 2014 at 9:04 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.
> ---
> 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 b3f95d2..75fdff2 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -793,3 +793,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);
So we currently have gatt_db_read_t and gatt_db_write_t, which are
stored with the attribute, and gatt_db_attribute_read_t and
gatt_db_attribute_write_t which are used to report the result of a
read or write. I think this is a bit confusing. Should we call these
something like gatt_db_read_complete_t or gatt_db_read_result_func_t
perhaps?
Also, won't we also have to change gatt_db_read_t and gatt_db_write_t
signatures to accept the result functions? If the read/write is being
handled by a callback, then they should report the result back using
the new callbacks. This way the code can be uniform regardless of
whether the result was obtained directly from the database or was
delegated to a callback (or at least this is what I was trying to do
in the patch set I sent out last week).
> +
> +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,
>
> ---
> src/shared/gatt-db.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 9ead5e3..acd2e9a 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -882,7 +882,22 @@ 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 (offset > attrib->value_len)
> + return false;
> +
> + if (attrib->read_func) {
> + attrib->read_func(attrib->handle, offset, opcode, bdaddr,
> + attrib->user_data);
> + return true;
> + }
> +
> + func(attrib, 0, &attrib->value[offset], attrib->value_len - offset,
> + user_data);
Wouldn't this potentially cause an invalid access if value_len ==
offset? I would just do a check here and pass NULL to func if that's
the case.
> +
> + return true;
> }
>
> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> --
Cheers,
Arman
Hi Luiz,
>
> ---
> src/shared/gatt-db.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index acd2e9a..4ff7358 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -906,5 +906,20 @@ 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 (offset > attrib->value_len ||
> + len > (unsigned) (attrib->value_len - offset))
> + return false;
Won't this incorrectly fail if the attribute value has a variable
length? Quoted from Core v4.1 Vol 3 ATT: 3.4.5.1 Write Request: "If
the attribute value has a variable length, then the attribute value
shall be truncated or lengthened to match the length of the Attribute
Value parameter". So we can't really make assumptions on value_len
here.
Also, if the attribute value is being implemented by the higher layer,
then won't the value_len field of the attribute structure be invalid
in general? Unless we always cache the full value in the attribute
structure based on the last time gatt-db invoked the read callback but
then this would still incorrectly fail if we never read the attribute
value beforehand.
I think we can assume that all attributes that are created without a
write callback are not writable. I think we should always delegate
writes to a callback and just return false here if the callback was
never set (this would mean that the attribute is not writable). The
callback can perform the necessary offset and length checks as needed
by the service it's implementing, so we wouldn't end up making any
assumptions there.
> +
> + if (attrib->write_func) {
> + attrib->write_func(attrib->handle, offset, value, len, opcode,
> + bdaddr, attrib->user_data);
> + return true;
> + }
> +
> + memcpy(&attrib->value[offset], value, len);
> +
> + return true;
> }
> --
Cheers,
Arman
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 938cb67..5df6b10 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;
@@ -839,6 +843,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 | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 4ff7358..2d812f4 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -824,7 +824,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 2d812f4..29b9481 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -832,7 +832,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 | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 5df6b10..9ead5e3 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -870,7 +870,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 | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 9ead5e3..acd2e9a 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -882,7 +882,22 @@ 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 (offset > attrib->value_len)
+ return false;
+
+ if (attrib->read_func) {
+ attrib->read_func(attrib->handle, offset, opcode, bdaddr,
+ attrib->user_data);
+ return true;
+ }
+
+ 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 | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 75fdff2..c3fc2a7 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -797,7 +797,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
From: Luiz Augusto von Dentz <[email protected]>
---
src/shared/gatt-db.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index acd2e9a..4ff7358 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -906,5 +906,20 @@ 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 (offset > attrib->value_len ||
+ len > (unsigned) (attrib->value_len - offset))
+ return false;
+
+ if (attrib->write_func) {
+ attrib->write_func(attrib->handle, offset, value, len, opcode,
+ bdaddr, attrib->user_data);
+ return true;
+ }
+
+ memcpy(&attrib->value[offset], value, len);
+
+ return true;
}
--
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 c3fc2a7..938cb67 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -830,7 +830,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