Hi Arman,
On Wed, Oct 29, 2014 at 12:12 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
> Hi Luiz,
>
> On Tue, Oct 28, 2014 at 9:18 AM, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>>
>> ---
>> src/shared/gatt-db.c | 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 majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
