* Re: [PATCH BlueZ] shared/gatt-db: Expose gatt_db_attribute
From: Arman Uguray @ 2014-10-24 16:56 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: BlueZ development
In-Reply-To: <1414158494-12020-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Fri, Oct 24, 2014 at 6:48 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This is just a draft 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.h | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 8d18434..ab7469a 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -96,3 +96,25 @@ 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 *attrib,
> + uint16_t handle, uint16_t offset);
> +
What's the offset parameter here for? Why not just get the attribute by handle?
> +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);
> +
> +bool gatt_db_attribute_read(struct gatt_db_attribute *attrib,
> + uint8_t opcode, bdaddr_t *bdaddr,
> + void *callback, void *user_data);
> +
> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib,
> + const uint8_t *value, size_t len,
> + uint8_t opcode, bdaddr_t *bdaddr,
> + void *callback, void *user_data);
> --
Are these functions meant to replace the existing gatt_db_read, etc?
We might as well just replace all of those with functions that accept
a struct gatt_db_attribute. So that all functions would be preceded by
a call to gatt_db_get_attribute, followed by one of these functions.
Unless you want to keep the other functions as helpers that accomplish
the same thing.
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
Arman
^ permalink raw reply
* Re: [PATCH BlueZ 2/8] shared/gatt-db: Add complete callback to gatt_db_read.
From: Arman Uguray @ 2014-10-24 17:02 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Arman Uguray, linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZ+rxN6GrQTi1Fh5NYdRb9FXgf-w5cQrb+LEBdVZpSzjWg@mail.gmail.com>
Hi Luiz,
On Fri, Oct 24, 2014 at 2:31 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Tue, Oct 21, 2014 at 12:00 AM, Arman Uguray <armansito@chromium.org> wrote:
>> This patch introduces a completion callback parameter to gatt_db_read,
>> which is meant to be used by a gatt_db_read_t implementation to signal
>> the end of an asynchronous read operation performed in the upper layer.
>> ---
>> android/gatt.c | 21 ++++++++++++++++++---
>> src/shared/gatt-db.c | 7 +++++--
>> src/shared/gatt-db.h | 9 ++++++++-
>> src/shared/gatt-server.c | 4 ++--
>> 4 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index ea20941..e2aa686 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -4694,7 +4694,8 @@ static void read_requested_attributes(void *data, void *user_data)
>> resp_data->offset,
>> process_data->opcode,
>> &process_data->device->bdaddr,
>> - &value, &value_len))
>> + &value, &value_len,
>> + NULL, NULL))
>> error = ATT_ECODE_UNLIKELY;
>>
>> /* We have value here already if no callback will be called */
>> @@ -4744,7 +4745,10 @@ static struct pending_trans_data *conn_add_transact(struct app_connection *conn,
>> }
>>
>> static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>> - bdaddr_t *bdaddr, void *user_data)
>> + bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> + void *user_data)
>> {
>> struct pending_trans_data *transaction;
>> struct hal_ev_gatt_server_request_read ev;
>> @@ -6266,7 +6270,10 @@ static struct gap_srvc_handles gap_srvc_data;
>> #define PERIPHERAL_PRIVACY_DISABLE 0x00
>>
>> static void gap_read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>> - bdaddr_t *bdaddr, void *user_data)
>> + bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> + void *user_data)
>> {
>> struct pending_request *entry;
>> struct gatt_device *dev;
>> @@ -6373,6 +6380,8 @@ static void register_gap_service(void)
>>
>> static void device_info_read_cb(uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> void *user_data)
>> {
>> struct pending_request *entry;
>> @@ -6406,6 +6415,8 @@ done:
>>
>> static void device_info_read_system_id_cb(uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> void *user_data)
>> {
>> struct pending_request *entry;
>> @@ -6438,6 +6449,8 @@ done:
>>
>> static void device_info_read_pnp_id_cb(uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> void *user_data)
>> {
>> struct pending_request *entry;
>> @@ -6602,6 +6615,8 @@ static void gatt_srvc_change_write_cb(uint16_t handle, uint16_t offset,
>>
>> static void gatt_srvc_change_read_cb(uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> void *user_data)
>> {
>> struct pending_request *entry;
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index b3f95d2..c342e32 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -638,7 +638,9 @@ static bool find_service_for_handle(const void *data, const void *user_data)
>>
>> bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> - uint8_t **value, int *length)
>> + uint8_t **value, int *length,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data)
>> {
>> struct gatt_db_service *service;
>> uint16_t service_handle;
>> @@ -665,7 +667,8 @@ bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
>> if (a->read_func) {
>> *value = NULL;
>> *length = -1;
>> - a->read_func(handle, offset, att_opcode, bdaddr, a->user_data);
>> + a->read_func(handle, offset, att_opcode, bdaddr, complete_func,
>> + complete_data, a->user_data);
>> } else {
>> if (offset > a->value_len)
>> return false;
>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>> index 8d18434..1c8739e 100644
>> --- a/src/shared/gatt-db.h
>> +++ b/src/shared/gatt-db.h
>> @@ -30,8 +30,13 @@ uint16_t gatt_db_add_service(struct gatt_db *db, const bt_uuid_t *uuid,
>> bool primary, uint16_t num_handles);
>> bool gatt_db_remove_service(struct gatt_db *db, uint16_t handle);
>>
>> +typedef void (*gatt_db_read_complete_t)(uint16_t handle, uint16_t att_ecode,
>> + const uint8_t *value, size_t len,
>> + void *complete_data);
>> typedef void (*gatt_db_read_t) (uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data,
>> void *user_data);
>>
>> typedef void (*gatt_db_write_t) (uint16_t handle, uint16_t offset,
>> @@ -81,7 +86,9 @@ void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
>>
>> bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
>> uint8_t att_opcode, bdaddr_t *bdaddr,
>> - uint8_t **value, int *length);
>> + uint8_t **value, int *length,
>> + gatt_db_read_complete_t complete_func,
>> + void *complete_data);
>
> Lets remove value and length here and make it always return via
> callback, we can probably use a idle callback if there is a problem to
> return the values immediately.
>
I agree with this, since this'll get rid of a lot duplicate code, and
we can probably get away with not using an idle callback initially.
One problem though is that the android code makes use of these
functions and I don't have an Android build or devices set up to test
this. So would you mind taking on this task (or someone that has an
android build), change the gatt_db_read signature and make sure all
the android code works with passing unit tests, and then I'll rebase
my gatt-server patches on top of it?
Or if we don't want to hold back on my current patch set, you can
merge them and then do the refactor. Up to you.
>> bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
>> const uint8_t *value, size_t len,
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 657b564..3233b21 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -114,8 +114,8 @@ static bool encode_read_by_grp_type_rsp(struct gatt_db *db, struct queue *q,
>> */
>> if (!gatt_db_read(db, start_handle, 0,
>> BT_ATT_OP_READ_BY_GRP_TYPE_REQ,
>> - NULL, &value,
>> - &value_len) || value_len < 0)
>> + NULL, &value, &value_len,
>> + NULL, NULL) || value_len < 0)
>> return false;
>>
>> /*
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
^ permalink raw reply
* Re: [PATCH BlueZ 6/8] shared/gatt-server: Implement "Read By Type" request.
From: Arman Uguray @ 2014-10-24 17:12 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZLkhzGgdZk2YSxbhdKHXeX2f_2OZi6sXvcbj75P8NgRtw@mail.gmail.com>
Hi Luiz,
On Fri, Oct 24, 2014 at 2:25 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Tue, Oct 21, 2014 at 12:00 AM, Arman Uguray <armansito@chromium.org> wrote:
>> This patch implements the ATT protocol "Read By Type" request for
>> shared/gatt-server. Logic is implemented that allows asynchronous
>> reading of non-standard attribute values via the registered read and
>> read completion callbacks.
>> ---
>> src/shared/gatt-server.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 270 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 3233b21..6c5ea2b 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -38,6 +38,16 @@
>> #define MIN(a, b) ((a) < (b) ? (a) : (b))
>> #endif
>>
>> +struct async_read_op {
>> + struct bt_gatt_server *server;
>> + uint8_t opcode;
>> + bool done;
>> + uint8_t *pdu;
>> + size_t pdu_len;
>> + int value_len;
>> + struct queue *db_data;
>> +};
>> +
>> struct bt_gatt_server {
>> struct gatt_db *db;
>> struct bt_att *att;
>> @@ -46,6 +56,9 @@ struct bt_gatt_server {
>>
>> unsigned int mtu_id;
>> unsigned int read_by_grp_type_id;
>> + unsigned int read_by_type_id;
>> +
>> + struct async_read_op *pending_read_op;
>>
>> bt_gatt_server_debug_func_t debug_callback;
>> bt_gatt_server_destroy_func_t debug_destroy;
>> @@ -59,8 +72,12 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>>
>> bt_att_unregister(server->att, server->mtu_id);
>> bt_att_unregister(server->att, server->read_by_grp_type_id);
>> + bt_att_unregister(server->att, server->read_by_type_id);
>> bt_att_unref(server->att);
>>
>> + if (server->pending_read_op)
>> + server->pending_read_op->server = NULL;
>> +
>> free(server);
>> }
>>
>> @@ -124,21 +141,21 @@ static bool encode_read_by_grp_type_rsp(struct gatt_db *db, struct queue *q,
>> * value is seen.
>> */
>> if (iter == 0) {
>> - data_val_len = value_len;
>> + data_val_len = MIN(MIN(mtu - 6, 251), value_len);
>> pdu[0] = data_val_len + 4;
>> iter++;
>> } else if (value_len != data_val_len)
>> break;
>>
>> /* Stop if this unit would surpass the MTU */
>> - if (iter + data_val_len + 4 > mtu)
>> + if (iter + data_val_len + 4 > mtu - 1)
>> break;
>>
>> end_handle = gatt_db_get_end_handle(db, start_handle);
>>
>> put_le16(start_handle, pdu + iter);
>> put_le16(end_handle, pdu + iter + 2);
>> - memcpy(pdu + iter + 4, value, value_len);
>> + memcpy(pdu + iter + 4, value, data_val_len);
>>
>> iter += data_val_len + 4;
>> }
>> @@ -235,6 +252,248 @@ done:
>> NULL, NULL, NULL);
>> }
>>
>> +static void async_read_op_destroy(struct async_read_op *op)
>> +{
>> + if (op->server)
>> + op->server->pending_read_op = NULL;
>> +
>> + queue_destroy(op->db_data, NULL);
>> + free(op->pdu);
>> + free(op);
>> +}
>> +
>> +static void process_read_by_type(struct async_read_op *op);
>> +
>> +static void read_by_type_read_complete_cb(uint16_t handle, uint16_t att_ecode,
>> + const uint8_t *value, size_t len,
>> + void *complete_data)
>> +{
>> + struct async_read_op *op = complete_data;
>> + struct bt_gatt_server *server = op->server;
>> + uint16_t mtu;
>> +
>> + if (!server) {
>> + async_read_op_destroy(op);
>> + return;
>> + }
>> +
>> + mtu = bt_att_get_mtu(server->att);
>> +
>> + /* Terminate the operation if there was an error */
>> + if (att_ecode) {
>> + uint8_t pdu[4];
>> +
>> + encode_error_rsp(BT_ATT_OP_READ_BY_TYPE_REQ, handle, att_ecode,
>> + pdu);
>> + bt_att_send(server->att, BT_ATT_OP_ERROR_RSP, pdu, 4, NULL,
>> + NULL, NULL);
>> + async_read_op_destroy(op);
>> + return;
>> + }
>> +
>> + if (op->pdu_len == 0) {
>> + op->value_len = MIN(MIN(mtu - 4, 253), len);
>> + op->pdu[0] = op->value_len + 2;
>> + op->pdu_len++;
>> + } else if (len != op->value_len) {
>> + op->done = true;
>> + goto done;
>> + }
>> +
>> + /* Stop if this would surpass the MTU */
>> + if (op->pdu_len + op->value_len + 2 > mtu - 1) {
>> + op->done = true;
>> + goto done;
>> + }
>> +
>> + /* Encode the current value */
>> + put_le16(handle, op->pdu + op->pdu_len);
>> + memcpy(op->pdu + op->pdu_len + 2, value, op->value_len);
>> +
>> + op->pdu_len += op->value_len + 2;
>> +
>> + if (op->pdu_len == mtu - 1)
>> + op->done = true;
>
> The code above is duplicated and it is actually causing some warnings
> comparing unsigned with signed (usually cause by '-' operation) which
> I started fixing myself but stop once I realize this was the result of
> gatt_db_read having 2 modes to read, sync and async, which I don't
> think is a good idea.
>
Yes, please see my response to your other comment in this patch set.
>> +done:
>> + process_read_by_type(op);
>> +}
>> +
>> +static void process_read_by_type(struct async_read_op *op)
>> +{
>> + struct bt_gatt_server *server = op->server;
>> + uint16_t mtu = bt_att_get_mtu(server->att);
>> + uint8_t rsp_opcode;
>> + uint8_t rsp_len;
>> + uint8_t ecode;
>> + uint16_t ehandle;
>> + uint16_t start_handle;
>> + uint8_t *value;
>> + int value_len;
>> + uint32_t perm;
>> +
>> + if (op->done) {
>> + rsp_opcode = BT_ATT_OP_READ_BY_TYPE_RSP;
>> + rsp_len = op->pdu_len;
>> + goto done;
>> + }
>> +
>> + while (queue_peek_head(op->db_data)) {
>> + start_handle = PTR_TO_UINT(queue_pop_head(op->db_data));
>> + value = NULL;
>> + value_len = 0;
>> +
>> + if (!gatt_db_get_attribute_permissions(server->db, start_handle,
>> + &perm)) {
>> + ecode = BT_ATT_ERROR_UNLIKELY;
>> + goto error;
>> + }
>> +
>> + /*
>> + * Check for the READ access permission. Encryption,
>> + * authentication, and authorization permissions need to be
>> + * checked by the read handler, since bt_att is agnostic to
>> + * connection type and doesn't have security information on it.
>> + */
>> + if (perm && !(perm & BT_ATT_PERM_READ)) {
>> + ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
>> + goto error;
>> + }
>
> I would suggest moving this check to gatt_db_read which should return
> -EPERM or the actual code if it is too hard to have errno mapping for
> each error code.
>
gatt_db currently has this nice property that the permissions field is
just a uint32_t. This means that while shared/gatt-server can use the
new permission macros I added to shared/att-types, the android code
can still use the Android platform definitions. Of course, once
android starts using shared/gatt-server it'll have to use the new
macros but for now we can have gatt-db support both types by keeping
the permission checks outside of gatt-db and this would make the whole
transition easier. Makes sense?
>> + if (!gatt_db_read(server->db, start_handle, 0, op->opcode, NULL,
>> + &value, &value_len,
>> + read_by_type_read_complete_cb, op)) {
>> + ecode = BT_ATT_ERROR_UNLIKELY;
>> + goto error;
>> + }
>> +
>> + /* The read has been deferred to the upper layer if |value_len|
>> + * is less than 0.
>> + */
>> + if (value_len < 0)
>> + return;
>> +
>> + if (op->pdu_len == 0) {
>> + op->value_len = MIN(MIN(mtu - 4, 253), value_len);
>> + op->pdu[0] = op->value_len + 2;
>> + op->pdu_len++;
>> + } else if (value_len != op->value_len)
>> + break;
>> +
>> + /* Stop if this would surpass the MTU */
>> + if (op->pdu_len + op->value_len + 2 > mtu - 1)
>> + break;
>> +
>> + /* Encode the current value */
>> + put_le16(start_handle, op->pdu + op->pdu_len);
>> + memcpy(op->pdu + op->pdu_len + 2, value, op->value_len);
>> +
>> + op->pdu_len += op->value_len + 2;
>> +
>> + if (op->pdu_len == mtu - 1)
>> + break;
>> + }
>> +
>> + rsp_opcode = BT_ATT_OP_READ_BY_TYPE_RSP;
>> + rsp_len = op->pdu_len;
>> +
>> + goto done;
>> +
>> +error:
>> + rsp_opcode = BT_ATT_OP_ERROR_RSP;
>> + rsp_len = 4;
>> + encode_error_rsp(BT_ATT_OP_READ_BY_TYPE_REQ, ehandle, ecode, op->pdu);
>> +
>> +done:
>> + bt_att_send(server->att, rsp_opcode, op->pdu, rsp_len, NULL,
>> + NULL, NULL);
>> + async_read_op_destroy(op);
>> +}
>> +
>> +static void read_by_type_cb(uint8_t opcode, const void *pdu,
>> + uint16_t length, void *user_data)
>> +{
>> + struct bt_gatt_server *server = user_data;
>> + uint16_t start, end;
>> + bt_uuid_t type;
>> + uint8_t rsp_pdu[4];
>> + uint16_t ehandle = 0;
>> + uint8_t ecode;
>> + struct queue *q = NULL;
>> + struct async_read_op *op;
>> +
>> + if (length != 6 && length != 20) {
>> + ecode = BT_ATT_ERROR_INVALID_PDU;
>> + goto error;
>> + }
>> +
>> + q = queue_new();
>> + if (!q) {
>> + ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;
>> + goto error;
>> + }
>> +
>> + start = get_le16(pdu);
>> + end = get_le16(pdu + 2);
>> + get_uuid_le(pdu + 4, length - 4, &type);
>> +
>> + util_debug(server->debug_callback, server->debug_data,
>> + "Read By Type - start: 0x%04x end: 0x%04x",
>> + start, end);
>> +
>> + if (!start || !end) {
>> + ecode = BT_ATT_ERROR_INVALID_HANDLE;
>> + goto error;
>> + }
>> +
>> + ehandle = start;
>> +
>> + if (start > end) {
>> + ecode = BT_ATT_ERROR_INVALID_HANDLE;
>> + goto error;
>> + }
>> +
>> + gatt_db_read_by_type(server->db, start, end, type, q);
>> +
>> + if (queue_isempty(q)) {
>> + ecode = BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND;
>> + goto error;
>> + }
>> +
>> + if (server->pending_read_op) {
>> + ecode = BT_ATT_ERROR_UNLIKELY;
>> + goto error;
>> + }
>> +
>> + op = new0(struct async_read_op, 1);
>> + if (!op) {
>> + ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;
>> + goto error;
>> + }
>> +
>> + op->pdu = malloc(bt_att_get_mtu(server->att));
>> + if (!op->pdu) {
>> + free(op);
>> + ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES;
>> + goto error;
>> + }
>> +
>> + op->opcode = opcode;
>> + op->server = server;
>> + op->db_data = q;
>> + server->pending_read_op = op;
>> +
>> + process_read_by_type(op);
>> +
>> + return;
>> +
>> +error:
>> + encode_error_rsp(opcode, ehandle, ecode, rsp_pdu);
>> + queue_destroy(q, NULL);
>> + bt_att_send(server->att, BT_ATT_OP_ERROR_RSP, rsp_pdu, 4,
>> + NULL, NULL, NULL);
>> +}
>> +
>> static void exchange_mtu_cb(uint8_t opcode, const void *pdu,
>> uint16_t length, void *user_data)
>> {
>> @@ -283,6 +542,14 @@ static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
>> if (!server->read_by_grp_type_id)
>> return false;
>>
>> + /* Read By Type */
>> + server->read_by_type_id = bt_att_register(server->att,
>> + BT_ATT_OP_READ_BY_TYPE_REQ,
>> + read_by_type_cb,
>> + server, NULL);
>> + if (!server->read_by_type_id)
>> + return false;
>> +
>> return true;
>> }
>>
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
^ permalink raw reply
* Re: GATT + DBus API Questions
From: Arman Uguray @ 2014-10-24 17:16 UTC (permalink / raw)
To: Ryan Du Bois; +Cc: BlueZ development
In-Reply-To: <68B9F3E4-5167-413F-9907-7733B8B404B2@kamama.com>
Hi Ryan,
On Thu, Oct 23, 2014 at 5:21 PM, Ryan Du Bois <rdub@kamama.com> wrote:
> Hey Arman,
>
> On Oct 23, 2014, at 5:00 PM, Arman Uguray <armansito@chromium.org> wrote:
>
>> Hi Ryan,
>>
>> On Thu, Oct 23, 2014 at 4:41 PM, Ryan Du Bois <rdub@kamama.com> wrote:
>>> Hey There,
>>>
>>> I've been trying to create a GATT service via the DBus BlueZ API (org.bluez.GattManager1 + org.bluez.GattService1/GattCharacteristic1). I'm currently using dbus-python to do this.
>>>
>>> What I have been able to accomplish thus far (with much exploring of the source code, and additional DBG() messages added to illuminate where I had been going wrong) is the following:
>>> - Created proper DBus Proxy objects for GattService1 & GattCharacteristic1
>>> - Each object vends the org.freedesktop.DBus.Properties interface, to allow BlueZ to retrieve the UUID and other properties on each instance.
>>> - Created a proper ObjectManager proxy object to allow BlueZ to retrieve the aforementioned GattService1 and GattCharacteristic1 objects.
>>> - Called GattManager1's RegisterService() method with appropriate arguments.
>>> - Observed that BlueZ's GattManager1 instance calls my ObjectManager's GetManagedObjects() method, and receives the appropriate object paths/handles/properties.
>>> - Observed that BlueZ accomplishes all the internal machinery necessary to register the GATT Service in src/gatt-dbus.c (namely, I get the "Added GATT service /com/kamama/blargh..." printout on the console, when running in debug mode).
>>>
>>> What I'm not able to see is the GATT service being listed when I explore the GATT services on my device via a BTLE exploration tool (I'm using LightBlue on Mac OS X to explore this device).
>>>
>>> Looking closer at the code, it looks to me like src/gatt-dbus.c is not hooked up to attrib/gatt-service.c, which is probably why my GATT Service and Characteristic are not showing up when exploring the device.
>>>
>>> Am I missing something, or is this not yet implemented?
>>>
>>> Thanks!
>>> --
>>> +Ryan Du Bois
>>> rdub@kamama.com
>>>
>>
>> As far as I know, gatt-dbus isn't actually wired up to attrib-server
>> as you said. So it populates an internal linked list of attributes but
>> then doesn't actually do anything over the listening socket it
>> creates.
>>
>> The thing is, we're currently in the process of rewriting all of
>> bluez's internal GATT/ATT tools and we'll probably entirely rewrite
>> src/gatt-dbus as well. An experimental implementation should appear
>> for both GATT client and server roles in the upcoming months, but
>> until then there's no good way to host GATT services over D-Bus.
>>
>> Thanks,
>> Arman
>
> Okay, that correlates with what I'm seeing. Most examples I've seen online
> have been implemented in C, calling gatt_service_add().
>
> In the time between now and a working DBus GATT API, would that
> approach be the proper method to host a GATT service with BlueZ?
> I'm on 5.21 at the moment, but could easily upgrade to something else
> if it's better for GATT services.
>
Yes, until there's a working GATT DBus API, you can follow the C examples.
> Thanks!
> --
> +Ryan Du Bois
> rdub@kamama.com
>
Cheers,
Arman
^ permalink raw reply
* Re: Attribute security permissions and CCC descriptors in shared/gatt-server.
From: Arman Uguray @ 2014-10-24 19:26 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: BlueZ development, Marcin Kraglak, Marcel Holtmann
In-Reply-To: <CABBYNZLE_xxoaPXZ=9LY2AwT-Pa_mqEYRD8v4_TgMTdo77j-Qw@mail.gmail.com>
Hi Luiz,
On Thu, Oct 23, 2014 at 12:51 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Wed, Oct 22, 2014 at 11:12 PM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi all,
>>
>> I have some unresolved questions about two aspects of
>> shared/gatt-server and I thought it might be better to discuss them
>> before moving on with the related parts of the implementation.
>>
>> *** 1. Who will be responsible for Attribute Permission checks? ***
>>
>> These mainly come in play when the remote end wants to read or write
>> the value of a characteristic/descriptor. We currently have read &
>> write callbacks that get assigned by the upper-layer to a
>> characteristic/descriptor definition in shared/gatt-db. Since these
>> callbacks already handle the read/write it initially made sense to me
>> that this is where encryption/authentication/authorization permission
>> checks can be performed. I then realized that there are cases where we
>> may want to perform this check before we invoke the read/write
>> callback. For instance, the Prepare Write request requires attribute
>> permission checks to be performed, however (in the current design) we
>> wouldn't actually call the write callback until a subsequent Execute
>> Write request is received.
>
> I believe this should be handle by the db, perhaps with a dedicated
> prepare function that should probably call a callback so the
> implementation can really tell when a write is about to happen. The
> weird thing here is that the gatt_db_add_characteristic take
> permissions but never really use it for anything except in
> gatt_db_get_attribute_permissions, I thought the idea would be to do
> the permission checks with it or Im missing something.
>
My impression was that gatt-db simple acts as a place to store the
permission data. It currently doesn't define a standard format for
storing those permissions and android uses the Android platform API's
permission enum values directly. I'm now defining standard values for
these, though again, gatt-db still would only be able to check for the
access permissions and not encryption/authentication/authorization
permissions, which should really be checked by bt_gatt_server since
it's already associated with the relevant bt_att.
>> The main problem here is the fact that shared/att is designed to be
>> transport agnostic: it doesn't know whether the underlying connection
>> is AF_BLUETOOTH or something else, so it can't make assumptions on the
>> nature of the connection to obtain security level information. We
>> could add some sort of delegate callback to this end, perhaps
>> something like:
>
> Then we should probably have some functions to set and get the
> security level, but Im sure it needs to be a callback as it does not
> looks like it gonna change without us noticing it, then we can
> automatically check with security level against the permissions
> provided by the profile when registering the characteristic.
>
Do you mean that you're sure it needs to be a callback or you're NOT sure? :)
>> static uint8_t my_sec_handler(void *user_data)
>> {
>> ....
>> return sec;
>> }
>> ...
>> bt_att_register_security_handler(att, my_sec_handler);
>>
>> And then bt_gatt_server can obtain the current security level like this:
>>
>> uint8_t sec = bt_att_get_security_level(att);
>>
>> where bt_att_get_security_level is implemented as:
>>
>> static uint8_t bt_att_get_security_level(struct bt_att *att)
>> {
>> if (!att || !att->security_callback)
>> return 0;
>>
>> return att->security_callback(att->security_data);
>> }
>>
>>
>> I'm mostly thinking out loud here; would something like this make sense?
>>
>>
>> *** 2. Is shared/gatt-server responsible for keeping track of Client
>> Characteristic Configuration descriptor states for each bonded client?
>> ***
>>
>> Currently, the descriptor read/write operations need to be handled by
>> the upper layer via the read/write callbacks anyway, since the
>> side-effects of a writing to a particular CCC descriptor need to be
>> implemented by the related profile. In the bonding case, the
>> read/write callbacks could determine what value to return and how to
>> cache the value on a per-device basis without shared/gatt-db having a
>> notion of "per-client CCC descriptors".
>
> Well making gatt_db have the notion of CCC would make things a little
> bit simpler for storing and reloading but it would probably force us
> to rethink how the db access the values, perhaps the db could cache
> CCC values per client so it could detect changes and send
> notifications automatically whenever the profiles tell it values has
> changed or when a write succeeds.
>
Things start getting tricky here since automatically sending
notifications to the correct clients requires gatt-db to send these
through the correct bt_gatt_server/bt_att structures, which means then
it needs to somehow keep track of or access these. I think this goes
beyond what gatt-db was built for which is to just act as an attribute
cache.
Regardless, all profiles will already have to separately implement the
side effects of writing to a CCC descriptor and sending out the
notifications/indications when needed. That said, leaving the
management of which clients enabled notifications and which didn't
entirely to profiles will lead to a lot of duplicate code among
profile implementations.
One thing we could do is have bt_gatt_server keep track of CCC
descriptors, similar to how bt_gatt_client takes care of writing to a
remote CCC. Since bt_gatt_server is already a per-connection entity,
calling bt_gatt_server_send_notification could check to see if the
given handle matches a characteristic with a CCC descriptor and send
the notification only if that CCC descriptor was written earlier
through a write request through its internal bt_att.
Also, think of how all of this code will fit together in the future.
Take bluetoothd for example. It makes sense to me that a gatt-db will
be initialized by src/adapter, while there will be a separate instance
of bt_gatt_server for each src/device. So if a profile wants to notify
all connected GATT clients of a characteristic value with a single
function call, it'll probably have to call some API function through
src/adapter. So either there will need to be a higher-level entity
that owns gatt-db and has access to all bt_gatt_server instances, or
all bt_gatt_server instances will need to be registered with gatt-db
somehow. So, we will have to think of an interesting GATT-specific
client & server role profile API.
We should probably keep things all simple for now and revisit this in
the future. So I'm not going to focus on managing per-client CCC
descriptors for now and leave the notion of CCC up to the higher
levels of the stack until we have enough solid code to actually
implement something.
>> Does this make sense? If so, is there a point of keeping the "bdaddr_t
>> *bdaddr" arguments of gatt_db_read_t, gatt_db_write_t, gatt_db_read,
>> and gatt_db_write? Note that I'm currently passing NULL to all of
>> these calls for this parameter in shared/gatt-server, since bt_att
>> doesn't have a notion of a BD_ADDR.
>
> I guess this was inspired by Android, I would agree that if you
> register a characteristic and set the permission properly then it does
> not matter who is attempting to read/write as long as they are fulfill
> the requirements it should be okay but perhaps Android have pushed the
> notion of CCC up in the stack which force it to expose the remote
> address. Anyway for unit test I will have to address it since that
> will be using a socket pair, perhaps passing NULL is fine if we have
> CCC caching but in case of Android I don't think we can do much about
> it.
>
Well, in the Android case, could the device address be somehow
propagated as part of user_data?
>
> --
> Luiz Augusto von Dentz
Cheers,
Arman
^ permalink raw reply
* Re: [PATCHv5 00/14] Included service discovery
From: Arman Uguray @ 2014-10-24 19:32 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Szymon Janc, Marcin Kraglak,
linux-bluetooth@vger.kernel.org development
In-Reply-To: <CABBYNZKYSfpWCvK=3_VaKTz8nC-dWonkXesCibiuX1DPv+e3vA@mail.gmail.com>
Hi Luiz,
On Thu, Oct 23, 2014 at 12:55 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Szymon,
>
> On Wed, Oct 22, 2014 at 9:39 PM, Szymon Janc <szymon.janc@gmail.com> wrote:
>> Hi,
>>
>> On Wednesday 22 October 2014 08:35:05 Arman Uguray wrote:
>>> Hi Luiz,
>>>
>>> On Wed, Oct 22, 2014 at 7:54 AM, Luiz Augusto von Dentz
>>>
>>> <luiz.dentz@gmail.com> wrote:
>>> > Hi Marcin,
>>> >
>>> > On Wed, Oct 22, 2014 at 9:25 AM, Marcin Kraglak
>>> >
>>> > <marcin.kraglak@tieto.com> wrote:
>>> >> On 16 October 2014 12:17, Marcin Kraglak <marcin.kraglak@tieto.com>
>> wrote:
>>> >>> v3:
>>> >>> In this version after primary service discovery,
>>> >>> secondary services are discovered. Next included
>>> >>> services are resolved. With this approach we
>>> >>> don't have recursively search for included service,
>>> >>> like it was TODO in previous proposal.
>>> >>> There is also small coding style fix suggested by Arman.
>>> >>>
>>> >>> v4:
>>> >>> If no secondary services found, continue include services search (fixed
>>> >>> in gatt-client.c).
>>> >>> Fixed wrong debug logs (primary->secondary).
>>> >>> Fixed searching descriptors
>>> >>>
>>> >>> v5:
>>> >>> Ignore Unsupported Group Type Error in response to secondary service
>>> >>> discovery and continue included services discovery.
>>> >>>
>>> >>> Marcin Kraglak (14):
>>> >>> shared/gatt: Add discover_secondary_services()
>>> >>> shared/gatt: Add initial implementation of discover_included_services
>>> >>> shared/gatt: Discover included services 128 bit UUIDS
>>> >>> shared/gatt: Add extra check in characteristic iterator
>>> >>> shared/gatt: Add included service iterator
>>> >>> shared/gatt: Remove not needed function parameter
>>> >>> shared/gatt: Distinguish Primary from Secondary services
>>> >>> tools/btgatt-client: Print type of service
>>> >>> shared/gatt: Discover secondary services
>>> >>> shared/gatt: Discover included services
>>> >>> shared/gatt: Add gatt-client include service iterator
>>> >>> tools/btgatt-client: Print found include services
>>> >>> shared/gatt: Fix searching descriptors
>>> >>> shared/gatt: Add function bt_gatt_result_included_count()
>>> >>>
>>> >>> src/shared/gatt-client.c | 263 +++++++++++++++++++++++++++--
>>> >>> src/shared/gatt-client.h | 18 ++
>>> >>> src/shared/gatt-helpers.c | 418
>>> >>> +++++++++++++++++++++++++++++++++++++++++++---
>>> >>> src/shared/gatt-helpers.h | 10 +-
>>> >>> tools/btgatt-client.c | 17 +-
>>> >>> 5 files changed, 690 insertions(+), 36 deletions(-)
>>> >>>
>>> >>> --
>>> >>> 1.9.3
>>> >
>>> > Patches looks fine to me, but I would like Arman to ack before applying.
>>>
>>> I'm fine with the patches as they are, though I saw that Szymon has
>>> left comments on some of them (I'm guessing he's a doing a pass of the
>>> patch set now). I'm good with it as long as his comments are
>>> addressed.
>>
>> I really think we should have more comments and less magic numbers there.
>> Otherwise this code will be hard to maintain in long term.
>
> Yep, we could actually start creating packed structs for PDU as it was
> done for AVRCP so we could do sizeof etc, it makes the code much
> easier to understand and maintain.
>
I agree, though I'd be happier if we added unit tests first. We can
then introduce packed structs and convert all code, though the unit
tests would at least give us confidence that we didn't accidentally
break any of the PDU encoding.
Cheers,
Arman
^ permalink raw reply
* [PATCH BlueZ v2 0/7] Unit tests for GAttrib
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
Since we want to replace GAttrib with a bt_att shim for a smooth transition
to bt_att profiles, this patch cleans up the API and adds unit tests for
GAttrib functions.
This adds tests for all but the register / unregister functions, which
I will send in a separate patch since they are slightly more complicated due
to catchalls.
v1 -> v2:
* Add g_attrib_register tests
* Cleanup of unit/test-gattrib.c
* Bugfix for GATTRIB_ALL_REQS
Michael Janssen (7):
Remove unused g_attrib_set_debug function
attrib: Remove MTU-probing code
attrib: Add mtu argument to g_attrib_new
attrib: remove g_attrib_is_encrypted
Add unit tests for gattrib
attrib: Add unit tests for g_attrib_register
attrib: fix GATTRIB_ALL_REQS behavior
.gitignore | 1 +
Makefile.am | 7 +
attrib/gattrib.c | 61 +++----
attrib/gattrib.h | 7 +-
attrib/gatttool.c | 17 +-
attrib/interactive.c | 17 +-
src/attrib-server.c | 9 +-
src/device.c | 11 +-
unit/test-gattrib.c | 505 +++++++++++++++++++++++++++++++++++++++++++++++++++
9 files changed, 588 insertions(+), 47 deletions(-)
create mode 100644 unit/test-gattrib.c
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply
* [PATCH BlueZ v2 1/7] Remove unused g_attrib_set_debug function
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
This function is not used, and also not implemented.
---
attrib/gattrib.c | 5 -----
attrib/gattrib.h | 3 ---
2 files changed, 8 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 912dffb..71d1cef 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -651,11 +651,6 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
return ret;
}
-gboolean g_attrib_set_debug(GAttrib *attrib,
- GAttribDebugFunc func, gpointer user_data)
-{
- return TRUE;
-}
uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len)
{
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 3fe92c7..18df2ad 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -58,9 +58,6 @@ guint g_attrib_send(GAttrib *attrib, guint id, const guint8 *pdu, guint16 len,
gboolean g_attrib_cancel(GAttrib *attrib, guint id);
gboolean g_attrib_cancel_all(GAttrib *attrib);
-gboolean g_attrib_set_debug(GAttrib *attrib,
- GAttribDebugFunc func, gpointer user_data);
-
guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,
GAttribNotifyFunc func, gpointer user_data,
GDestroyNotify notify);
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ v2 2/7] attrib: Remove MTU-probing code
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
Probing for the MTU using bt_io is problematic for
testing because you cannot impersonate AF_BLUETOOTH sockets
with a socketpair.
---
attrib/gattrib.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 71d1cef..fa41ade 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -476,27 +476,17 @@ done:
GAttrib *g_attrib_new(GIOChannel *io)
{
struct _GAttrib *attrib;
- uint16_t imtu;
uint16_t att_mtu;
uint16_t cid;
- GError *gerr = NULL;
g_io_channel_set_encoding(io, NULL, NULL);
g_io_channel_set_buffered(io, FALSE);
- bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu,
- BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID);
- if (gerr) {
- error("%s", gerr->message);
- g_error_free(gerr);
- return NULL;
- }
-
attrib = g_try_new0(struct _GAttrib, 1);
if (attrib == NULL)
return NULL;
- att_mtu = (cid == ATT_CID) ? ATT_DEFAULT_LE_MTU : imtu;
+ att_mtu = ATT_DEFAULT_LE_MTU;
attrib->buf = g_malloc0(att_mtu);
attrib->buflen = att_mtu;
@@ -651,7 +641,6 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
return ret;
}
-
uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len)
{
if (len == NULL)
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ v2 3/7] attrib: Add mtu argument to g_attrib_new
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
Instead of using the default MTU, use one passed
in by the user, and detect it from the channel when
it is created.
---
attrib/gattrib.c | 10 +++-------
attrib/gattrib.h | 2 +-
attrib/gatttool.c | 17 ++++++++++++++++-
attrib/interactive.c | 17 ++++++++++++++++-
src/device.c | 11 ++++++++++-
5 files changed, 46 insertions(+), 11 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index fa41ade..a6511a2 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -473,11 +473,9 @@ done:
return TRUE;
}
-GAttrib *g_attrib_new(GIOChannel *io)
+GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu)
{
struct _GAttrib *attrib;
- uint16_t att_mtu;
- uint16_t cid;
g_io_channel_set_encoding(io, NULL, NULL);
g_io_channel_set_buffered(io, FALSE);
@@ -486,10 +484,8 @@ GAttrib *g_attrib_new(GIOChannel *io)
if (attrib == NULL)
return NULL;
- att_mtu = ATT_DEFAULT_LE_MTU;
-
- attrib->buf = g_malloc0(att_mtu);
- attrib->buflen = att_mtu;
+ attrib->buf = g_malloc0(mtu);
+ attrib->buflen = mtu;
attrib->io = g_io_channel_ref(io);
attrib->requests = g_queue_new();
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 18df2ad..99b8b37 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -42,7 +42,7 @@ typedef void (*GAttribDebugFunc)(const char *str, gpointer user_data);
typedef void (*GAttribNotifyFunc)(const guint8 *pdu, guint16 len,
gpointer user_data);
-GAttrib *g_attrib_new(GIOChannel *io);
+GAttrib *g_attrib_new(GIOChannel *io, guint16 mtu);
GAttrib *g_attrib_ref(GAttrib *attrib);
void g_attrib_unref(GAttrib *attrib);
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index db5da62..8f92d62 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -123,6 +123,9 @@ static gboolean listen_start(gpointer user_data)
static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
{
GAttrib *attrib;
+ uint16_t mtu;
+ uint16_t cid;
+ GError *gerr = NULL;
if (err) {
g_printerr("%s\n", err->message);
@@ -130,7 +133,19 @@ static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
g_main_loop_quit(event_loop);
}
- attrib = g_attrib_new(io);
+ bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &mtu,
+ BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID);
+
+ if (gerr) {
+ g_printerr("Can't detect MTU, using default: %s", gerr->message);
+ g_error_free(gerr);
+ mtu = ATT_DEFAULT_LE_MTU;
+ }
+
+ if (cid == ATT_CID)
+ mtu = ATT_DEFAULT_LE_MTU;
+
+ attrib = g_attrib_new(io, mtu);
if (opt_listen)
g_idle_add(listen_start, attrib);
diff --git a/attrib/interactive.c b/attrib/interactive.c
index 08f39f7..7911ba5 100644
--- a/attrib/interactive.c
+++ b/attrib/interactive.c
@@ -150,13 +150,28 @@ static void events_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
static void connect_cb(GIOChannel *io, GError *err, gpointer user_data)
{
+ uint16_t mtu;
+ uint16_t cid;
+
if (err) {
set_state(STATE_DISCONNECTED);
error("%s\n", err->message);
return;
}
- attrib = g_attrib_new(iochannel);
+ bt_io_get(io, &err, BT_IO_OPT_IMTU, &mtu,
+ BT_IO_OPT_CID, &cid, BT_IO_OPT_INVALID);
+
+ if (err) {
+ g_printerr("Can't detect MTU, using default: %s", err->message);
+ g_error_free(err);
+ mtu = ATT_DEFAULT_LE_MTU;
+ }
+
+ if (cid == ATT_CID)
+ mtu = ATT_DEFAULT_LE_MTU;
+
+ attrib = g_attrib_new(iochannel, mtu);
g_attrib_register(attrib, ATT_OP_HANDLE_NOTIFY, GATTRIB_ALL_HANDLES,
events_handler, attrib, NULL);
g_attrib_register(attrib, ATT_OP_HANDLE_IND, GATTRIB_ALL_HANDLES,
diff --git a/src/device.c b/src/device.c
index 875a5c5..0925951 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3627,11 +3627,17 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
GError *gerr = NULL;
GAttrib *attrib;
BtIOSecLevel sec_level;
+ uint16_t mtu;
+ uint16_t cid;
bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
+ BT_IO_OPT_IMTU, &mtu,
+ BT_IO_OPT_CID, &cid,
BT_IO_OPT_INVALID);
+
if (gerr) {
error("bt_io_get: %s", gerr->message);
+ mtu = ATT_DEFAULT_LE_MTU;
g_error_free(gerr);
return false;
}
@@ -3649,7 +3655,10 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
}
}
- attrib = g_attrib_new(io);
+ if (cid == ATT_CID)
+ mtu = ATT_DEFAULT_LE_MTU;
+
+ attrib = g_attrib_new(io, mtu);
if (!attrib) {
error("Unable to create new GAttrib instance");
return false;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ v2 4/7] attrib: remove g_attrib_is_encrypted
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
This function is only used in one place and encryption is the
responsibility of the channel, not the attribute.
---
attrib/gattrib.c | 12 ------------
attrib/gattrib.h | 2 --
src/attrib-server.c | 9 +++++++--
3 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index a6511a2..a65d1ca 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -690,18 +690,6 @@ static int event_cmp_by_id(gconstpointer a, gconstpointer b)
return evt->id - id;
}
-gboolean g_attrib_is_encrypted(GAttrib *attrib)
-{
- BtIOSecLevel sec_level;
-
- if (!bt_io_get(attrib->io, NULL,
- BT_IO_OPT_SEC_LEVEL, &sec_level,
- BT_IO_OPT_INVALID))
- return FALSE;
-
- return sec_level > BT_IO_SEC_LOW;
-}
-
gboolean g_attrib_unregister(GAttrib *attrib, guint id)
{
struct event *evt;
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 99b8b37..1557b99 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -62,8 +62,6 @@ guint g_attrib_register(GAttrib *attrib, guint8 opcode, guint16 handle,
GAttribNotifyFunc func, gpointer user_data,
GDestroyNotify notify);
-gboolean g_attrib_is_encrypted(GAttrib *attrib);
-
uint8_t *g_attrib_get_buffer(GAttrib *attrib, size_t *len);
gboolean g_attrib_set_mtu(GAttrib *attrib, int mtu);
diff --git a/src/attrib-server.c b/src/attrib-server.c
index e65fff2..1ccfc65 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -375,12 +375,17 @@ static struct attribute *attrib_db_add_new(struct gatt_server *server,
static uint8_t att_check_reqs(struct gatt_channel *channel, uint8_t opcode,
int reqs)
{
+ BtIOSecLevel sec_level;
+ GIOChannel *io = g_attrib_get_channel(channel->attrib);
+
/* FIXME: currently, it is assumed an encrypted link is enough for
* authentication. This will allow to enable the SMP negotiation once
* it is on upstream kernel. High security level should be mapped
* to authentication and medium to encryption permission. */
- if (!channel->encrypted)
- channel->encrypted = g_attrib_is_encrypted(channel->attrib);
+ if (!channel->encrypted &&
+ bt_io_get(io, NULL, BT_IO_OPT_SEC_LEVEL, &sec_level,
+ BT_IO_OPT_INVALID))
+ channel->encrypted = sec_level > BT_IO_SEC_LOW;
if (reqs == ATT_AUTHENTICATION && !channel->encrypted)
return ATT_ECODE_AUTHENTICATION;
else if (reqs == ATT_AUTHORIZATION)
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ v2 5/7] Add unit tests for gattrib
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
This will ensure that the API behavior of gattrib is preserved.
---
.gitignore | 1 +
Makefile.am | 7 ++
unit/test-gattrib.c | 350 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 358 insertions(+)
create mode 100644 unit/test-gattrib.c
diff --git a/.gitignore b/.gitignore
index 97daa9f..164cc97 100644
--- a/.gitignore
+++ b/.gitignore
@@ -121,6 +121,7 @@ unit/test-avdtp
unit/test-avctp
unit/test-avrcp
unit/test-gatt
+unit/test-gattrib
unit/test-*.log
unit/test-*.trs
diff --git a/Makefile.am b/Makefile.am
index 2dfea28..3fddb80 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -377,6 +377,13 @@ unit_test_gatt_SOURCES = unit/test-gatt.c
unit_test_gatt_LDADD = src/libshared-glib.la \
lib/libbluetooth-internal.la @GLIB_LIBS@
+unit_tests += unit/test-gattrib
+
+unit_test_gattrib_SOURCES = unit/test-gattrib.c attrib/gattrib.c $(btio_sources) src/log.h src/log.c
+unit_test_gattrib_LDADD = lib/libbluetooth-internal.la \
+ src/libshared-glib.la \
+ @GLIB_LIBS@ @DBUS_LIBS@ -ldl -lrt
+
if MAINTAINER_MODE
noinst_PROGRAMS += $(unit_tests)
endif
diff --git a/unit/test-gattrib.c b/unit/test-gattrib.c
new file mode 100644
index 0000000..23a5485
--- /dev/null
+++ b/unit/test-gattrib.c
@@ -0,0 +1,350 @@
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <inttypes.h>
+#include <string.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+
+#include <glib.h>
+
+#include "src/shared/util.h"
+#include "lib/uuid.h"
+#include "attrib/att.h"
+#include "attrib/gattrib.h"
+#include "src/log.h"
+
+#define DEFAULT_MTU 23
+
+#define data(args...) ((const unsigned char[]) { args })
+
+struct context {
+ GMainLoop *main_loop;
+ GIOChannel *att_io;
+ GIOChannel *server_io;
+ GAttrib *att;
+};
+
+static void setup_context(struct context *cxt, gconstpointer data)
+{
+ int err, sv[2];
+
+ cxt->main_loop = g_main_loop_new(NULL, FALSE);
+ g_assert(cxt->main_loop != NULL);
+
+ err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
+ g_assert(err == 0);
+
+ cxt->att_io = g_io_channel_unix_new(sv[0]);
+ g_assert(cxt->att_io != NULL);
+
+ g_io_channel_set_close_on_unref(cxt->att_io, TRUE);
+
+ cxt->server_io = g_io_channel_unix_new(sv[1]);
+ g_assert(cxt->server_io != NULL);
+
+ g_io_channel_set_close_on_unref(cxt->server_io, TRUE);
+ g_io_channel_set_encoding(cxt->server_io, NULL, NULL);
+ g_io_channel_set_buffered(cxt->server_io, FALSE);
+
+ cxt->att = g_attrib_new(cxt->att_io, DEFAULT_MTU);
+ g_assert(cxt->att != NULL);
+}
+
+static void teardown_context(struct context *cxt, gconstpointer data)
+{
+ if (cxt->att)
+ g_attrib_unref(cxt->att);
+
+ g_io_channel_unref(cxt->server_io);
+
+ g_io_channel_unref(cxt->att_io);
+
+ g_main_loop_unref(cxt->main_loop);
+}
+
+
+static void test_debug(const char *str, void *user_data)
+{
+ const char *prefix = user_data;
+
+ g_print("%s%s\n", prefix, str);
+}
+
+static void destroy_canary_increment(gpointer data)
+{
+ int *canary = data;
+ (*canary)++;
+}
+
+static void test_refcount(struct context *cxt, gconstpointer unused)
+{
+ GAttrib *extra_ref;
+ int destroy_canary;
+
+ g_attrib_set_destroy_function(cxt->att, destroy_canary_increment,
+ &destroy_canary);
+
+ extra_ref = g_attrib_ref(cxt->att);
+
+ g_assert(extra_ref == cxt->att);
+
+ g_assert(destroy_canary == 0);
+
+ g_attrib_unref(extra_ref);
+
+ g_assert(destroy_canary == 0);
+
+ g_attrib_unref(cxt->att);
+
+ g_assert(destroy_canary == 1);
+
+ /* Avoid a double-free from the teardown function */
+ cxt->att = NULL;
+}
+
+static void test_get_channel(struct context *cxt, gconstpointer unused)
+{
+ GIOChannel *chan;
+
+ chan = g_attrib_get_channel(cxt->att);
+
+ g_assert(chan == cxt->att_io);
+}
+
+struct challenge_response {
+ const uint8_t *expect;
+ const size_t expect_len;
+ const uint8_t *respond;
+ const size_t respond_len;
+ bool received;
+};
+
+static gboolean test_client(GIOChannel *channel, GIOCondition cond,
+ gpointer data)
+{
+ struct challenge_response *cr = data;
+ int fd;
+ uint8_t buf[256];
+ ssize_t len;
+
+ if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
+ return FALSE;
+ }
+
+ fd = g_io_channel_unix_get_fd(channel);
+
+ len = read(fd, buf, sizeof(buf));
+
+ g_assert(len > 0);
+ g_assert_cmpint(len, ==, cr->expect_len);
+
+ if (g_test_verbose())
+ util_hexdump('>', buf, len, test_debug, "test_client: ");
+
+ cr->received = true;
+
+ if (cr->respond != NULL) {
+ if (g_test_verbose())
+ util_hexdump('<', cr->respond, cr->respond_len,
+ test_debug, "test_client: ");
+ len = write(fd, cr->respond, cr->respond_len);
+
+ g_assert_cmpint(len, ==, cr->respond_len);
+ }
+
+ return TRUE;
+}
+
+struct result_data {
+ guint8 status;
+ const guint8 *pdu;
+ guint16 len;
+ bool done;
+};
+
+static void result_canary(guint8 status, const guint8 *pdu, guint16 len,
+ gpointer data)
+{
+ struct result_data *result = data;
+ result->status = status;
+ result->pdu = pdu;
+ result->len = len;
+
+ if (g_test_verbose())
+ util_hexdump('<', pdu, len, test_debug, "result_canary: ");
+
+ result->done = true;
+}
+
+static void test_send(struct context *cxt, gconstpointer unused)
+{
+ int cmp;
+ struct result_data results;
+ struct challenge_response data = {
+ .expect = data(0x02, 0x00, 0x02),
+ .expect_len = 3,
+ .respond = data(0x01, 0x02, 0x03, 0x04),
+ .respond_len = 4,
+ .received = false,
+ };
+
+ g_io_add_watch(cxt->server_io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ test_client, &data);
+
+ results.done = false;
+
+ g_attrib_send(cxt->att, 0, data.expect, data.expect_len, result_canary,
+ (gpointer) &results, NULL);
+
+ /* Spin the main loop until we are ready. */
+ do {
+ g_main_context_iteration(NULL, TRUE);
+ } while (!results.done);
+
+ g_assert_cmpint(results.len, ==, data.respond_len);
+
+ cmp = memcmp(results.pdu, data.respond, results.len);
+
+ g_assert(cmp == 0);
+}
+
+static void test_cancel(struct context *cxt, gconstpointer unused)
+{
+ guint event_id;
+ gboolean canceled;
+ struct result_data results;
+ struct challenge_response data = {
+ .expect = data(0x02, 0x00, 0x02),
+ .expect_len = 3,
+ .respond = data(0x01, 0x02, 0x03, 0x04),
+ .respond_len = 4,
+ .received = false,
+ };
+
+ g_io_add_watch(cxt->server_io,
+ G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ test_client, &data);
+
+ results.done = false;
+ event_id = g_attrib_send(cxt->att, 0, data.expect, data.expect_len,
+ result_canary, &results, NULL);
+
+ /* Spin the main loop until we receive the first message */
+ do {
+ g_main_context_iteration(NULL, FALSE);
+ } while (!data.received);
+
+ canceled = g_attrib_cancel(cxt->att, event_id);
+ g_assert(canceled);
+
+ /*
+ * Spin the main loop until all events are processed,
+ * no result should be called because it was cancelled */
+ do {
+ g_main_context_iteration(NULL, TRUE);
+ } while (g_main_context_pending(NULL));
+
+ g_assert(!results.done);
+
+ results.done = false;
+ data.received = false;
+ event_id = g_attrib_send(cxt->att, 0, data.expect, data.expect_len,
+ result_canary, &results, NULL);
+
+ canceled = g_attrib_cancel(cxt->att, event_id);
+ g_assert(canceled);
+
+ /*
+ * Spin the main loop until all events are processed,
+ * the message should never be delivered.
+ */
+ do {
+ g_main_context_iteration(NULL, TRUE);
+ } while (g_main_context_pending(NULL));
+
+ g_assert(!data.received);
+ g_assert(!results.done);
+
+ /* Invalid ID */
+ canceled = g_attrib_cancel(cxt->att, 42);
+ g_assert(!canceled);
+}
+
+static void test_register(struct context *cxt, gconstpointer user_data)
+{
+ /* TODO */
+}
+
+static void test_buffers(struct context *cxt, gconstpointer unused)
+{
+ size_t buflen;
+ uint8_t *buf;
+ gboolean success;
+
+ buf = g_attrib_get_buffer(cxt->att, &buflen);
+ g_assert(buf != 0);
+ g_assert_cmpint(buflen, ==, DEFAULT_MTU);
+
+ success = g_attrib_set_mtu(cxt->att, 5);
+ g_assert(!success);
+
+ success = g_attrib_set_mtu(cxt->att, 255);
+ g_assert(success);
+
+ buf = g_attrib_get_buffer(cxt->att, &buflen);
+ g_assert(buf != 0);
+ g_assert_cmpint(buflen, ==, 255);
+}
+
+int main(int argc, char *argv[])
+{
+ g_test_init(&argc, &argv, NULL);
+
+ __btd_log_init("*", 0);
+
+ /*
+ * Test the GAttrib API behavior
+ */
+ g_test_add("/gattrib/refcount", struct context, NULL, setup_context,
+ test_refcount, teardown_context);
+ g_test_add("/gattrib/get_channel", struct context, NULL, setup_context,
+ test_get_channel, teardown_context);
+ g_test_add("/gattrib/send", struct context, NULL, setup_context,
+ test_send, teardown_context);
+ g_test_add("/gattrib/cancel", struct context, NULL, setup_context,
+ test_cancel, teardown_context);
+ g_test_add("/gattrib/register", struct context, NULL, setup_context,
+ test_register, teardown_context);
+ g_test_add("/gattrib/buffers", struct context, NULL, setup_context,
+ test_buffers, teardown_context);
+
+ return g_test_run();
+}
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ v2 6/7] attrib: Add unit tests for g_attrib_register
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
---
unit/test-gattrib.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 156 insertions(+), 1 deletion(-)
diff --git a/unit/test-gattrib.c b/unit/test-gattrib.c
index 23a5485..61db4a1 100644
--- a/unit/test-gattrib.c
+++ b/unit/test-gattrib.c
@@ -45,6 +45,23 @@
#define data(args...) ((const unsigned char[]) { args })
+struct test_pdu {
+ bool valid;
+ bool sent;
+ bool received;
+ const uint8_t *data;
+ size_t size;
+};
+
+#define pdu(args...) \
+ { \
+ .valid = true, \
+ .sent = false, \
+ .received = false, \
+ .data = data(args), \
+ .size = sizeof(data(args)), \
+ }
+
struct context {
GMainLoop *main_loop;
GIOChannel *att_io;
@@ -298,9 +315,147 @@ static void test_cancel(struct context *cxt, gconstpointer unused)
g_assert(!canceled);
}
+static void send_test_pdus(gpointer context, struct test_pdu *pdus)
+{
+ struct context *cxt = context;
+ size_t len;
+ int fd;
+ struct test_pdu *cur_pdu;
+
+ fd = g_io_channel_unix_get_fd(cxt->server_io);
+
+ for (cur_pdu = pdus; cur_pdu->valid; cur_pdu++)
+ cur_pdu->sent = false;
+
+ for (cur_pdu = pdus; cur_pdu->valid; cur_pdu++) {
+ if (g_test_verbose())
+ util_hexdump('>', cur_pdu->data, cur_pdu->size,
+ test_debug, "send_test_pdus: ");
+ len = write(fd, cur_pdu->data, cur_pdu->size);
+ g_assert_cmpint(len, ==, cur_pdu->size);
+ do {
+ g_main_context_iteration(NULL, TRUE);
+ } while (g_main_context_pending(NULL));
+ cur_pdu->sent = true;
+ }
+}
+
+#define PDU_MTU_RESP pdu(ATT_OP_MTU_RESP, 0x17)
+#define PDU_FIND_INFO_REQ pdu(ATT_OP_FIND_INFO_REQ, 0x01, 0x00, 0xFF, 0xFF)
+#define PDU_IND_NODATA pdu(ATT_OP_HANDLE_IND, 0x01, 0x00)
+#define PDU_INVALID_IND pdu(ATT_OP_HANDLE_IND, 0x14)
+#define PDU_IND_DATA pdu(ATT_OP_HANDLE_IND, 0x14, 0x00, 0x01)
+
+static void notify_canary_expect(const guint8 *pdu, guint16 len, gpointer data)
+{
+ struct test_pdu *expected = data;
+ int cmp;
+
+ if (g_test_verbose())
+ util_hexdump('<', pdu, len, test_debug,
+ "notify_canary_expect: ");
+
+ while (expected->valid && expected->received)
+ expected++;
+
+ g_assert(expected->valid);
+
+ if (g_test_verbose())
+ util_hexdump('?', expected->data, expected->size, test_debug,
+ "notify_canary_expect: ");
+
+ g_assert_cmpint(expected->size, ==, len);
+
+ cmp = memcmp(pdu, expected->data, expected->size);
+
+ g_assert(cmp == 0);
+
+ expected->received = true;
+}
+
static void test_register(struct context *cxt, gconstpointer user_data)
{
- /* TODO */
+ guint reg_id;
+ gboolean success;
+ struct test_pdu pdus[] = {
+ /* Unmatched by any (GATTRIB_ALL_EVENTS) */
+ PDU_MTU_RESP,
+ /*
+ * Unmatched PDU opcode
+ * Unmatched handle (GATTRIB_ALL_REQS) */
+ PDU_FIND_INFO_REQ,
+ /*
+ * Matched PDU opcode
+ * Unmatched handle (GATTRIB_ALL_HANDLES) */
+ PDU_IND_NODATA,
+ /*
+ * Matched PDU opcode
+ * Invalid length? */
+ PDU_INVALID_IND,
+ /*
+ * Matched PDU opcode
+ * Matched handle */
+ PDU_IND_DATA,
+ { },
+ };
+ struct test_pdu req_pdus[] = { PDU_FIND_INFO_REQ, { } };
+ struct test_pdu all_ind_pdus[] = {
+ PDU_IND_NODATA,
+ PDU_INVALID_IND,
+ PDU_IND_DATA,
+ { },
+ };
+ struct test_pdu followed_ind_pdus[] = { PDU_IND_DATA, { } };
+
+ /*
+ * Without registering anything, should be able to ignore everything but
+ * an unexpected response. */
+ send_test_pdus(cxt, pdus + 1);
+
+ reg_id = g_attrib_register(cxt->att, GATTRIB_ALL_EVENTS,
+ GATTRIB_ALL_HANDLES, notify_canary_expect,
+ pdus, NULL);
+
+ send_test_pdus(cxt, pdus);
+
+ g_attrib_unregister(cxt->att, reg_id);
+
+ if (g_test_verbose())
+ g_print("ALL_REQS, ALL_HANDLES\r\n");
+
+ reg_id = g_attrib_register(cxt->att, GATTRIB_ALL_REQS,
+ GATTRIB_ALL_HANDLES, notify_canary_expect,
+ req_pdus, NULL);
+
+ send_test_pdus(cxt, pdus);
+
+ g_attrib_unregister(cxt->att, reg_id);
+
+ if (g_test_verbose())
+ g_print("IND, ALL_HANDLES\r\n");
+
+ reg_id = g_attrib_register(cxt->att, ATT_OP_HANDLE_IND,
+ GATTRIB_ALL_HANDLES, notify_canary_expect,
+ all_ind_pdus, NULL);
+
+ send_test_pdus(cxt, pdus);
+
+ g_attrib_unregister(cxt->att, reg_id);
+
+ if (g_test_verbose())
+ g_print("IND, 0x0014\r\n");
+
+ reg_id = g_attrib_register(cxt->att, ATT_OP_HANDLE_IND, 0x0014,
+ notify_canary_expect, followed_ind_pdus,
+ NULL);
+
+ send_test_pdus(cxt, pdus);
+
+ g_attrib_unregister(cxt->att, reg_id);
+
+ success = g_attrib_unregister(cxt->att, reg_id);
+
+ g_assert(!success);
}
static void test_buffers(struct context *cxt, gconstpointer unused)
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH BlueZ v2 7/7] attrib: fix GATTRIB_ALL_REQS behavior
From: Michael Janssen @ 2014-10-24 20:36 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Michael Janssen
In-Reply-To: <1414182983-23959-1-git-send-email-jamuraa@chromium.org>
g_attrib_register(..., GATTRIB_ALL_REQS, ...) behavior would previously
include indications, this fixes it to only include requests and
commands.
---
attrib/gattrib.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index a65d1ca..f678435 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -147,6 +147,27 @@ static bool is_response(guint8 opcode)
return false;
}
+static bool is_request(guint8 opcode)
+{
+ switch (opcode) {
+ case ATT_OP_MTU_REQ:
+ case ATT_OP_FIND_INFO_REQ:
+ case ATT_OP_FIND_BY_TYPE_REQ:
+ case ATT_OP_READ_BY_TYPE_REQ:
+ case ATT_OP_READ_REQ:
+ case ATT_OP_READ_BLOB_REQ:
+ case ATT_OP_READ_MULTI_REQ:
+ case ATT_OP_READ_BY_GROUP_REQ:
+ case ATT_OP_WRITE_REQ:
+ case ATT_OP_WRITE_CMD:
+ case ATT_OP_PREP_WRITE_REQ:
+ case ATT_OP_EXEC_WRITE_REQ:
+ return true;
+ }
+
+ return false;
+}
+
GAttrib *g_attrib_ref(GAttrib *attrib)
{
int refs;
@@ -373,7 +394,7 @@ static bool match_event(struct event *evt, const uint8_t *pdu, gsize len)
if (evt->expected == GATTRIB_ALL_EVENTS)
return true;
- if (!is_response(pdu[0]) && evt->expected == GATTRIB_ALL_REQS)
+ if (is_request(pdu[0]) && evt->expected == GATTRIB_ALL_REQS)
return true;
if (evt->expected == pdu[0] && evt->handle == GATTRIB_ALL_HANDLES)
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH V2 net-next] Bluetooth: fix shadow warning in hci_disconnect()
From: Fabian Frederick @ 2014-10-25 8:48 UTC (permalink / raw)
To: linux-kernel
Cc: Fabian Frederick, Marcel Holtmann, Gustavo Padovan, Johan Hedberg,
David S. Miller, linux-bluetooth, netdev
use clkoff_cp for hci_cp_read_clock_offset instead of cp
(already defined above).
Suggested-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
V2: use clkoff_cp instead of cpr (suggested by Marcel Holtmann)
net/bluetooth/hci_conn.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..62f4ac6 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -141,10 +141,11 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason)
*/
if (conn->type == ACL_LINK && conn->role == HCI_ROLE_MASTER) {
struct hci_dev *hdev = conn->hdev;
- struct hci_cp_read_clock_offset cp;
+ struct hci_cp_read_clock_offset clkoff_cp;
- cp.handle = cpu_to_le16(conn->handle);
- hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(cp), &cp);
+ clkoff_cp.handle = cpu_to_le16(conn->handle);
+ hci_send_cmd(hdev, HCI_OP_READ_CLOCK_OFFSET, sizeof(clkoff_cp),
+ &clkoff_cp);
}
conn->state = BT_DISCONN;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v5 0/2] core: Add plugin-support for Manufacturer Specific Data EIR
From: Johan Hedberg @ 2014-10-25 14:33 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413816661-19290-1-git-send-email-fons@spotify.com>
Hi Alfonso,
On Mon, Oct 20, 2014, Alfonso Acosta wrote:
> v5:
>
> * rebased
> * fixed warning
> * fixed android compilation error
>
> v4:
>
> * Support for multiple MSD fields in the same EIR data block.
> * Moved parsing to a separate function: eir_parse_msd()
>
> v3:
>
> * Corrections suggested by Johan on the handling of eir.h
> * Parser sanity check on the size of the MSD payload.
> * Minor typo correction on commit message.
>
> Alfonso Acosta (2):
> core: Add Manufacturer Specific Data EIR field
> core: Add subscription API for Manufacturer Specific Data
>
> src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> src/adapter.h | 10 ++++++++++
> src/eir.c | 22 ++++++++++++++++++++++
> src/eir.h | 10 ++++++++++
> 4 files changed, 87 insertions(+)
Both of these patches have been applied. Note that I made a small change
to the first one to avoid a dependency on hci.h (by having a dedicated
define in eir.h).
Johan
^ permalink raw reply
* Re: [PATCH V2 net-next] Bluetooth: fix shadow warning in hci_disconnect()
From: Marcel Holtmann @ 2014-10-25 16:57 UTC (permalink / raw)
To: Fabian Frederick
Cc: linux-kernel, Gustavo F. Padovan, Johan Hedberg, David S. Miller,
linux-bluetooth, netdev
In-Reply-To: <1414226938-7478-1-git-send-email-fabf@skynet.be>
Hi Fabian,
> use clkoff_cp for hci_cp_read_clock_offset instead of cp
> (already defined above).
>
> Suggested-by: Marcel Holtmann <marcel@holtmann.org>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
> V2: use clkoff_cp instead of cpr (suggested by Marcel Holtmann)
>
> net/bluetooth/hci_conn.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH 0/3] Bluetooth: Add kernel-side SMP self-tests
From: johan.hedberg @ 2014-10-25 19:15 UTC (permalink / raw)
To: linux-bluetooth
Hi,
This patch set adds some simple self-tests for the SMP crypto functions.
The tests are disabled by default and need to be enabled with a new
config option in order to be compiled in.
Johan
----------------------------------------------------------------
Johan Hedberg (3):
Bluetooth: Pass only crypto context to SMP crypto functions
Bluetooth: Add skeleton for SMP self-tests
Bluetooth: Add self-tests for SMP crypto functions
net/bluetooth/Kconfig | 6 ++
net/bluetooth/smp.c | 150 ++++++++++++++++++++++++++++++++++++++++------
2 files changed, 138 insertions(+), 18 deletions(-)
^ permalink raw reply
* [PATCH 1/3] Bluetooth: Pass only crypto context to SMP crypto functions
From: johan.hedberg @ 2014-10-25 19:15 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1414264539-19068-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
In order to make unit testing possible we need to make the SMP crypto
functions only take the crypto context instead of the full SMP context
(the latter would require having hci_dev, hci_conn, l2cap_chan,
l2cap_conn, etc around). The drawback is that we no-longer get the
involved hdev in the debug logs, but this is really the only way to make
simple unit tests for the code.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index f09b6b65cf6b..fea3782989f4 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -191,16 +191,13 @@ int smp_generate_rpa(struct hci_dev *hdev, u8 irk[16], bdaddr_t *rpa)
return 0;
}
-static int smp_c1(struct smp_chan *smp, u8 k[16], u8 r[16], u8 preq[7],
- u8 pres[7], u8 _iat, bdaddr_t *ia, u8 _rat, bdaddr_t *ra,
- u8 res[16])
+static int smp_c1(struct crypto_blkcipher *tfm_aes, u8 k[16], u8 r[16],
+ u8 preq[7], u8 pres[7], u8 _iat, bdaddr_t *ia, u8 _rat,
+ bdaddr_t *ra, u8 res[16])
{
- struct hci_dev *hdev = smp->conn->hcon->hdev;
u8 p1[16], p2[16];
int err;
- BT_DBG("%s", hdev->name);
-
memset(p1, 0, 16);
/* p1 = pres || preq || _rat || _iat */
@@ -218,7 +215,7 @@ static int smp_c1(struct smp_chan *smp, u8 k[16], u8 r[16], u8 preq[7],
u128_xor((u128 *) res, (u128 *) r, (u128 *) p1);
/* res = e(k, res) */
- err = smp_e(smp->tfm_aes, k, res);
+ err = smp_e(tfm_aes, k, res);
if (err) {
BT_ERR("Encrypt data error");
return err;
@@ -228,26 +225,23 @@ static int smp_c1(struct smp_chan *smp, u8 k[16], u8 r[16], u8 preq[7],
u128_xor((u128 *) res, (u128 *) res, (u128 *) p2);
/* res = e(k, res) */
- err = smp_e(smp->tfm_aes, k, res);
+ err = smp_e(tfm_aes, k, res);
if (err)
BT_ERR("Encrypt data error");
return err;
}
-static int smp_s1(struct smp_chan *smp, u8 k[16], u8 r1[16], u8 r2[16],
- u8 _r[16])
+static int smp_s1(struct crypto_blkcipher *tfm_aes, u8 k[16], u8 r1[16],
+ u8 r2[16], u8 _r[16])
{
- struct hci_dev *hdev = smp->conn->hcon->hdev;
int err;
- BT_DBG("%s", hdev->name);
-
/* Just least significant octets from r1 and r2 are considered */
memcpy(_r, r2, 8);
memcpy(_r + 8, r1, 8);
- err = smp_e(smp->tfm_aes, k, _r);
+ err = smp_e(tfm_aes, k, _r);
if (err)
BT_ERR("Encrypt data error");
@@ -547,7 +541,7 @@ static u8 smp_confirm(struct smp_chan *smp)
BT_DBG("conn %p", conn);
- ret = smp_c1(smp, smp->tk, smp->prnd, smp->preq, smp->prsp,
+ ret = smp_c1(smp->tfm_aes, smp->tk, smp->prnd, smp->preq, smp->prsp,
conn->hcon->init_addr_type, &conn->hcon->init_addr,
conn->hcon->resp_addr_type, &conn->hcon->resp_addr,
cp.confirm_val);
@@ -578,7 +572,7 @@ static u8 smp_random(struct smp_chan *smp)
BT_DBG("conn %p %s", conn, conn->hcon->out ? "master" : "slave");
- ret = smp_c1(smp, smp->tk, smp->rrnd, smp->preq, smp->prsp,
+ ret = smp_c1(smp->tfm_aes, smp->tk, smp->rrnd, smp->preq, smp->prsp,
hcon->init_addr_type, &hcon->init_addr,
hcon->resp_addr_type, &hcon->resp_addr, confirm);
if (ret)
@@ -594,7 +588,7 @@ static u8 smp_random(struct smp_chan *smp)
__le64 rand = 0;
__le16 ediv = 0;
- smp_s1(smp, smp->tk, smp->rrnd, smp->prnd, stk);
+ smp_s1(smp->tfm_aes, smp->tk, smp->rrnd, smp->prnd, stk);
memset(stk + smp->enc_key_size, 0,
SMP_MAX_ENC_KEY_SIZE - smp->enc_key_size);
@@ -613,7 +607,7 @@ static u8 smp_random(struct smp_chan *smp)
smp_send_cmd(conn, SMP_CMD_PAIRING_RANDOM, sizeof(smp->prnd),
smp->prnd);
- smp_s1(smp, smp->tk, smp->prnd, smp->rrnd, stk);
+ smp_s1(smp->tfm_aes, smp->tk, smp->prnd, smp->rrnd, stk);
memset(stk + smp->enc_key_size, 0,
SMP_MAX_ENC_KEY_SIZE - smp->enc_key_size);
--
1.9.3
^ permalink raw reply related
* [PATCH 2/3] Bluetooth: Add skeleton for SMP self-tests
From: johan.hedberg @ 2014-10-25 19:15 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1414264539-19068-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
This patch adds a basic skeleton for SMP self-tests. The tests are put
behind a new configuration option since running them will slow down the
boot process. For now there are no actual tests defined but those will
come in a subsequent patch.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/Kconfig | 6 ++++++
net/bluetooth/smp.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 600fb29288f4..2675b4106b00 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -45,6 +45,12 @@ config BT_6LOWPAN
help
IPv6 compression over Bluetooth Low Energy.
+config BT_SELFTEST
+ bool "Run self-tests on boot"
+ depends on BT && DEBUG_KERNEL
+ help
+ Run self-tests during boot. Currently limited to SMP.
+
source "net/bluetooth/rfcomm/Kconfig"
source "net/bluetooth/bnep/Kconfig"
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index fea3782989f4..9821dc938e2c 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1743,3 +1743,36 @@ void smp_unregister(struct hci_dev *hdev)
hdev->smp_data = NULL;
l2cap_chan_put(chan);
}
+
+#ifdef CONFIG_BT_SELFTEST
+
+static int __init run_selftests(struct crypto_blkcipher *tfm_aes)
+{
+ return 0;
+}
+
+static int __init test_smp(void)
+{
+ struct crypto_blkcipher *tfm_aes;
+ int err;
+
+ tfm_aes = crypto_alloc_blkcipher("ecb(aes)", 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(tfm_aes)) {
+ BT_ERR("Unable to create ECB crypto context");
+ return PTR_ERR(tfm_aes);
+ }
+
+ err = run_selftests(tfm_aes);
+ if (err < 0)
+ BT_ERR("Self tests failed");
+ else
+ BT_INFO("Self-tests passed");
+
+ crypto_free_blkcipher(tfm_aes);
+
+ return err;
+}
+
+module_init(test_smp);
+
+#endif /* CONFIG_BT_SELFTEST */
--
1.9.3
^ permalink raw reply related
* [PATCH 3/3] Bluetooth: Add self-tests for SMP crypto functions
From: johan.hedberg @ 2014-10-25 19:15 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1414264539-19068-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
This patch adds self-tests for the c1 and s1 crypto functions used for
SMP pairing. The data used is the sample data from the core
specification.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 87 insertions(+)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 9821dc938e2c..983d1e0793f6 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1746,8 +1746,95 @@ void smp_unregister(struct hci_dev *hdev)
#ifdef CONFIG_BT_SELFTEST
+static int __init test_ah(struct crypto_blkcipher *tfm_aes)
+{
+ u8 irk[16] = { 0x9b, 0x7d, 0x39, 0x0a, 0xa6, 0x10, 0x10, 0x34,
+ 0x05, 0xad, 0xc8, 0x57, 0xa3, 0x34, 0x02, 0xec };
+ u8 r[3] = { 0x94, 0x81, 0x70 };
+ u8 exp[3] = { 0xaa, 0xfb, 0x0d };
+ u8 res[3];
+ int err;
+
+ err = smp_ah(tfm_aes, irk, r, res);
+ if (err)
+ return err;
+
+ if (memcmp(res, exp, 3) != 0)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int __init test_c1(struct crypto_blkcipher *tfm_aes)
+{
+ u8 k[16] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+ u8 r[16] = { 0xe0, 0x2e, 0x70, 0xc6, 0x4e, 0x27, 0x88, 0x63,
+ 0x0e, 0x6f, 0xad, 0x56, 0x21, 0xd5, 0x83, 0x57 };
+ u8 preq[7] = { 0x01, 0x01, 0x00, 0x00, 0x10, 0x07, 0x07 };
+ u8 pres[7] = { 0x02, 0x03, 0x00, 0x00, 0x08, 0x00, 0x05 };
+ u8 _iat = 0x01;
+ u8 _rat = 0x00;
+ bdaddr_t ra = { { 0xb6, 0xb5, 0xb4, 0xb3, 0xb2, 0xb1 } };
+ bdaddr_t ia = { { 0xa6, 0xa5, 0xa4, 0xa3, 0xa2, 0xa1 } };
+ u8 exp[16] = { 0x86, 0x3b, 0xf1, 0xbe, 0xc5, 0x4d, 0xa7, 0xd2,
+ 0xea, 0x88, 0x89, 0x87, 0xef, 0x3f, 0x1e, 0x1e };
+ u8 res[16];
+ int err;
+
+ err = smp_c1(tfm_aes, k, r, preq, pres, _iat, &ia, _rat, &ra, res);
+ if (err)
+ return err;
+
+ if (memcmp(res, exp, 16) != 0)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int __init test_s1(struct crypto_blkcipher *tfm_aes)
+{
+ u8 k[16] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+ u8 r1[16] = { 0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11 };
+ u8 r2[16] = { 0x00, 0xff, 0xee, 0xdd, 0xcc, 0xbb, 0xaa, 0x99 };
+ u8 exp[16] = { 0x62, 0xa0, 0x6d, 0x79, 0xae, 0x16, 0x42, 0x5b,
+ 0x9b, 0xf4, 0xb0, 0xe8, 0xf0, 0xe1, 0x1f, 0x9a };
+ u8 res[16];
+ int err;
+
+ err = smp_s1(tfm_aes, k, r1, r2, res);
+ if (err)
+ return err;
+
+ if (memcmp(res, exp, 16) != 0)
+ return -EINVAL;
+
+ return 0;
+}
+
static int __init run_selftests(struct crypto_blkcipher *tfm_aes)
{
+ int err;
+
+ err = test_ah(tfm_aes);
+ if (err) {
+ BT_ERR("smp_ah test failed");
+ return err;
+ }
+
+ err = test_c1(tfm_aes);
+ if (err) {
+ BT_ERR("smp_c1 test failed");
+ return err;
+ }
+
+ err = test_s1(tfm_aes);
+ if (err) {
+ BT_ERR("smp_s1 test failed");
+ return err;
+ }
+
return 0;
}
--
1.9.3
^ permalink raw reply related
* Re: [PATCH 0/3] Bluetooth: Add kernel-side SMP self-tests
From: Marcel Holtmann @ 2014-10-25 19:39 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1414264539-19068-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> This patch set adds some simple self-tests for the SMP crypto functions.
> The tests are disabled by default and need to be enabled with a new
> config option in order to be compiled in.
>
> Johan
>
> ----------------------------------------------------------------
> Johan Hedberg (3):
> Bluetooth: Pass only crypto context to SMP crypto functions
> Bluetooth: Add skeleton for SMP self-tests
> Bluetooth: Add self-tests for SMP crypto functions
>
> net/bluetooth/Kconfig | 6 ++
> net/bluetooth/smp.c | 150 ++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 138 insertions(+), 18 deletions(-)
all 3 patches have been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-26 14:22 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1413817285-23388-2-git-send-email-fons@spotify.com>
Hi,
I don't know what's the customary time to wait until reminding
maintainers about patches but, this one is a couple of weeks old (one
week since last revision).
Thanks,
On Mon, Oct 20, 2014 at 5:01 PM, Alfonso Acosta <fons@spotify.com> wrote:
> This patch adds btd_adapter_recreate_bonding() which rebonds a device,
> i.e. it performs an unbonding operation inmediately followed by a
> bonding operation.
>
> Although there is no internal use for btd_adapter_recreate_bonding()
> yet, it is useful for plugins dealing with devices which require
> renegotiaing their keys. For instance, when dealing with devices
> without persistent storage which lose their keys on reset.
>
> Certain caveats have been taken into account when rebonding with a LE
> device:
>
> * If the device to rebond is already connected, the rebonding is done
> without disconnecting to avoid the extra latency of reestablishing
> the connection.
>
> * If no connection exists, we connect before unbonding anyways to
> avoid losing the device's autoconnection and connection parameters,
> which are removed by the kernel when unpairing if no connection
> exists.
>
> * Not closing the connection requires defering attribute discovery
> until the rebonding is done. Otherwise, the security level could be
> elavated with the old LTK, which may be invalid since we are
> rebonding. When rebonding, attribute discovery is suspended and the
> ATT socket is saved to allow resuming it once bonding is finished.
> ---
> src/adapter.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/adapter.h | 7 ++++++-
> src/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/device.h | 4 ++++
> 4 files changed, 119 insertions(+), 7 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 1a7d4eb..d67c507 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5195,14 +5195,15 @@ int btd_adapter_read_clock(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
> }
>
> int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> - const bdaddr_t *bdaddr, uint8_t bdaddr_type)
> + const bdaddr_t *bdaddr, uint8_t bdaddr_type,
> + uint8_t disconnect)
> {
> struct mgmt_cp_unpair_device cp;
>
> memset(&cp, 0, sizeof(cp));
> bacpy(&cp.addr.bdaddr, bdaddr);
> cp.addr.type = bdaddr_type;
> - cp.disconnect = 1;
> + cp.disconnect = disconnect;
>
> if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE,
> adapter->dev_id, sizeof(cp), &cp,
> @@ -5212,6 +5213,53 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> return -EIO;
> }
>
> +int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
> + const bdaddr_t *bdaddr,
> + uint8_t bdaddr_type,
> + uint8_t io_cap)
> +{
> + struct btd_device *device;
> + int err;
> +
> + device = btd_adapter_get_device(adapter, bdaddr, bdaddr_type);
> +
> + if (!device || !device_is_bonded(device, bdaddr_type))
> + return -EINVAL;
> +
> + device_set_rebonding(device, bdaddr_type, true);
> +
> + /* Make sure the device is connected before unbonding to avoid
> + * losing the device's autoconnection and connection
> + * parameters, which are removed by the kernel when unpairing
> + * if no connection exists. We would had anyways implicitly
> + * connected when bonding later on, so we might as well just
> + * do it explicitly now.
> + */
> + if (bdaddr_type != BDADDR_BREDR && !btd_device_is_connected(device)) {
> + err = device_connect_le(device);
> + if (err < 0)
> + goto failed;
> + }
> +
> + /* Unbond without disconnecting to avoid the connection
> + * re-establishment latency
> + */
> + err = btd_adapter_remove_bonding(adapter, bdaddr, bdaddr_type, 0);
> + if (err < 0)
> + goto failed;
> +
> + err = adapter_create_bonding(adapter, bdaddr, bdaddr_type, io_cap);
> + if (err < 0)
> + goto failed;
> +
> + return 0;
> +
> +failed:
> + error("failed: %s", strerror(-err));
> + device_set_rebonding(device, bdaddr_type, false);
> + return err;
> +}
> +
> static void pincode_reply_complete(uint8_t status, uint16_t length,
> const void *param, void *user_data)
> {
> @@ -5594,8 +5642,11 @@ static void bonding_complete(struct btd_adapter *adapter,
> else
> device = btd_adapter_find_device(adapter, bdaddr, addr_type);
>
> - if (device != NULL)
> + if (device != NULL) {
> device_bonding_complete(device, addr_type, status);
> + if (device_is_rebonding(device, addr_type))
> + device_rebonding_complete(device, addr_type);
> + }
>
> resume_discovery(adapter);
>
> diff --git a/src/adapter.h b/src/adapter.h
> index 6801fee..bd00d3e 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -158,7 +158,12 @@ int btd_adapter_disconnect_device(struct btd_adapter *adapter,
> uint8_t bdaddr_type);
>
> int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> - const bdaddr_t *bdaddr, uint8_t bdaddr_type);
> + const bdaddr_t *bdaddr, uint8_t bdaddr_type,
> + uint8_t disconnect);
> +int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
> + const bdaddr_t *bdaddr,
> + uint8_t bdaddr_type,
> + uint8_t io_cap);
>
> int btd_adapter_pincode_reply(struct btd_adapter *adapter,
> const bdaddr_t *bdaddr,
> diff --git a/src/device.c b/src/device.c
> index 875a5c5..4aab349 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -158,6 +158,7 @@ struct att_callbacks {
> struct bearer_state {
> bool paired;
> bool bonded;
> + bool rebonding;
> bool connected;
> bool svc_resolved;
> };
> @@ -221,6 +222,8 @@ struct btd_device {
> int8_t rssi;
>
> GIOChannel *att_io;
> + GIOChannel *att_rebond_io;
> +
> guint cleanup_id;
> guint store_id;
> };
> @@ -522,6 +525,9 @@ static void device_free(gpointer user_data)
>
> attio_cleanup(device);
>
> + if (device->att_rebond_io)
> + g_io_channel_unref(device->att_rebond_io);
> +
> if (device->tmp_records)
> sdp_list_free(device->tmp_records,
> (sdp_free_func_t) sdp_record_free);
> @@ -1739,6 +1745,23 @@ long device_bonding_last_duration(struct btd_device *device)
> return bonding->last_attempt_duration_ms;
> }
>
> +bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type)
> +{
> + struct bearer_state *state = get_state(device, bdaddr_type);
> +
> + return state->rebonding;
> +}
> +
> +void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
> + bool rebonding)
> +{
> + struct bearer_state *state = get_state(device, bdaddr_type);
> +
> + state->rebonding = rebonding;
> +
> + DBG("rebonding = %d", rebonding);
> +}
> +
> static void create_bond_req_exit(DBusConnection *conn, void *user_data)
> {
> struct btd_device *device = user_data;
> @@ -2029,7 +2052,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
>
> if (state->paired && !state->bonded)
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - bdaddr_type);
> + bdaddr_type, 1);
>
> if (device->bredr_state.connected || device->le_state.connected)
> return;
> @@ -2639,13 +2662,13 @@ static void device_remove_stored(struct btd_device *device)
> if (device->bredr_state.bonded) {
> device->bredr_state.bonded = false;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - BDADDR_BREDR);
> + BDADDR_BREDR, 1);
> }
>
> if (device->le_state.bonded) {
> device->le_state.bonded = false;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - device->bdaddr_type);
> + device->bdaddr_type, 1);
> }
>
> device->bredr_state.paired = false;
> @@ -3628,6 +3651,18 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
> GAttrib *attrib;
> BtIOSecLevel sec_level;
>
> + DBG("");
> +
> + if (dev->le_state.rebonding) {
> + DBG("postponing due to rebonding");
> + /* Keep the att socket, and defer attaching the attributes
> + * until rebonding is done.
> + */
> + if (!dev->att_rebond_io)
> + dev->att_rebond_io = g_io_channel_ref(io);
> + return false;
> + }
> +
> bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
> BT_IO_OPT_INVALID);
> if (gerr) {
> @@ -4269,6 +4304,23 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> }
> }
>
> +bool device_rebonding_complete(struct btd_device *device, uint8_t bdaddr_type)
> +{
> + bool ret = true;
> +
> + DBG("");
> +
> + device_set_rebonding(device, bdaddr_type, false);
> +
> + if (bdaddr_type != BDADDR_BREDR && device->att_rebond_io) {
> + ret = device_attach_attrib(device, device->att_rebond_io);
> + g_io_channel_unref(device->att_rebond_io);
> + device->att_rebond_io = NULL;
> + }
> +
> + return ret;
> +}
> +
> static gboolean svc_idle_cb(gpointer user_data)
> {
> struct svc_callback *cb = user_data;
> diff --git a/src/device.h b/src/device.h
> index 2e0473e..18b5c6e 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -94,6 +94,10 @@ bool device_is_retrying(struct btd_device *device);
> void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> uint8_t status);
> gboolean device_is_bonding(struct btd_device *device, const char *sender);
> +bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type);
> +void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
> + bool rebonding);
> +bool device_rebonding_complete(struct btd_device *device, uint8_t bdaddr_type);
> void device_bonding_attempt_failed(struct btd_device *device, uint8_t status);
> void device_bonding_failed(struct btd_device *device, uint8_t status);
> struct btd_adapter_pin_cb_iter *device_bonding_iter(struct btd_device *device);
> --
> 1.9.1
>
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH BlueZ] shared/gatt-db: Expose gatt_db_attribute
From: Luiz Augusto von Dentz @ 2014-10-26 18:48 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
In-Reply-To: <CAHrH25ScMKevmBxzaOqtg3pvk-tzje5YRF--rEENAXXAPouAqQ@mail.gmail.com>
Hi Arman,
On Fri, Oct 24, 2014 at 7:56 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz,
>
> On Fri, Oct 24, 2014 at 6:48 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This is just a draft 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.h | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>> index 8d18434..ab7469a 100644
>> --- a/src/shared/gatt-db.h
>> +++ b/src/shared/gatt-db.h
>> @@ -96,3 +96,25 @@ 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 *attrib,
>> + uint16_t handle, uint16_t offset);
>> +
>
> What's the offset parameter here for? Why not just get the attribute by handle?
Probably an artifact of copy and pasting things around, it should be
just the handle.
>
>> +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);
>> +
>> +bool gatt_db_attribute_read(struct gatt_db_attribute *attrib,
>> + uint8_t opcode, bdaddr_t *bdaddr,
>> + void *callback, void *user_data);
>> +
>> +bool gatt_db_attribute_write(struct gatt_db_attribute *attrib,
>> + const uint8_t *value, size_t len,
>> + uint8_t opcode, bdaddr_t *bdaddr,
>> + void *callback, void *user_data);
>> --
>
> Are these functions meant to replace the existing gatt_db_read, etc?
> We might as well just replace all of those with functions that accept
> a struct gatt_db_attribute. So that all functions would be preceded by
> a call to gatt_db_get_attribute, followed by one of these functions.
> Unless you want to keep the other functions as helpers that accomplish
> the same thing.
Yes this would replace some functions that requires handles, I just
wanted to sync up first to get some feedback but it seems these
changes will be straight forward and perhaps we don't even need to
keep the old functions around.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH BlueZ 6/8] shared/gatt-server: Implement "Read By Type" request.
From: Luiz Augusto von Dentz @ 2014-10-26 18:56 UTC (permalink / raw)
To: Arman Uguray; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CAHrH25RQ87QGAR+x7ZYh7paoM7PRGRE9rxW=cCa6-C7Z_3yopw@mail.gmail.com>
Hi Arman,
On Fri, Oct 24, 2014 at 8:12 PM, Arman Uguray <armansito@google.com> wrote:
> Hi Luiz,
>
>
> On Fri, Oct 24, 2014 at 2:25 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Arman,
>>
>> On Tue, Oct 21, 2014 at 12:00 AM, Arman Uguray <armansito@chromium.org> wrote:
>>> This patch implements the ATT protocol "Read By Type" request for
>>> shared/gatt-server. Logic is implemented that allows asynchronous
>>> reading of non-standard attribute values via the registered read and
>>> read completion callbacks.
>>> ---
>>> src/shared/gatt-server.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 270 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>> index 3233b21..6c5ea2b 100644
>>> --- a/src/shared/gatt-server.c
>>> +++ b/src/shared/gatt-server.c
>>> @@ -38,6 +38,16 @@
>>> #define MIN(a, b) ((a) < (b) ? (a) : (b))
>>> #endif
>>>
>>> +struct async_read_op {
>>> + struct bt_gatt_server *server;
>>> + uint8_t opcode;
>>> + bool done;
>>> + uint8_t *pdu;
>>> + size_t pdu_len;
>>> + int value_len;
>>> + struct queue *db_data;
>>> +};
>>> +
>>> struct bt_gatt_server {
>>> struct gatt_db *db;
>>> struct bt_att *att;
>>> @@ -46,6 +56,9 @@ struct bt_gatt_server {
>>>
>>> unsigned int mtu_id;
>>> unsigned int read_by_grp_type_id;
>>> + unsigned int read_by_type_id;
>>> +
>>> + struct async_read_op *pending_read_op;
>>>
>>> bt_gatt_server_debug_func_t debug_callback;
>>> bt_gatt_server_destroy_func_t debug_destroy;
>>> @@ -59,8 +72,12 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>>>
>>> bt_att_unregister(server->att, server->mtu_id);
>>> bt_att_unregister(server->att, server->read_by_grp_type_id);
>>> + bt_att_unregister(server->att, server->read_by_type_id);
>>> bt_att_unref(server->att);
>>>
>>> + if (server->pending_read_op)
>>> + server->pending_read_op->server = NULL;
>>> +
>>> free(server);
>>> }
>>>
>>> @@ -124,21 +141,21 @@ static bool encode_read_by_grp_type_rsp(struct gatt_db *db, struct queue *q,
>>> * value is seen.
>>> */
>>> if (iter == 0) {
>>> - data_val_len = value_len;
>>> + data_val_len = MIN(MIN(mtu - 6, 251), value_len);
>>> pdu[0] = data_val_len + 4;
>>> iter++;
>>> } else if (value_len != data_val_len)
>>> break;
>>>
>>> /* Stop if this unit would surpass the MTU */
>>> - if (iter + data_val_len + 4 > mtu)
>>> + if (iter + data_val_len + 4 > mtu - 1)
>>> break;
>>>
>>> end_handle = gatt_db_get_end_handle(db, start_handle);
>>>
>>> put_le16(start_handle, pdu + iter);
>>> put_le16(end_handle, pdu + iter + 2);
>>> - memcpy(pdu + iter + 4, value, value_len);
>>> + memcpy(pdu + iter + 4, value, data_val_len);
>>>
>>> iter += data_val_len + 4;
>>> }
>>> @@ -235,6 +252,248 @@ done:
>>> NULL, NULL, NULL);
>>> }
>>>
>>> +static void async_read_op_destroy(struct async_read_op *op)
>>> +{
>>> + if (op->server)
>>> + op->server->pending_read_op = NULL;
>>> +
>>> + queue_destroy(op->db_data, NULL);
>>> + free(op->pdu);
>>> + free(op);
>>> +}
>>> +
>>> +static void process_read_by_type(struct async_read_op *op);
>>> +
>>> +static void read_by_type_read_complete_cb(uint16_t handle, uint16_t att_ecode,
>>> + const uint8_t *value, size_t len,
>>> + void *complete_data)
>>> +{
>>> + struct async_read_op *op = complete_data;
>>> + struct bt_gatt_server *server = op->server;
>>> + uint16_t mtu;
>>> +
>>> + if (!server) {
>>> + async_read_op_destroy(op);
>>> + return;
>>> + }
>>> +
>>> + mtu = bt_att_get_mtu(server->att);
>>> +
>>> + /* Terminate the operation if there was an error */
>>> + if (att_ecode) {
>>> + uint8_t pdu[4];
>>> +
>>> + encode_error_rsp(BT_ATT_OP_READ_BY_TYPE_REQ, handle, att_ecode,
>>> + pdu);
>>> + bt_att_send(server->att, BT_ATT_OP_ERROR_RSP, pdu, 4, NULL,
>>> + NULL, NULL);
>>> + async_read_op_destroy(op);
>>> + return;
>>> + }
>>> +
>>> + if (op->pdu_len == 0) {
>>> + op->value_len = MIN(MIN(mtu - 4, 253), len);
>>> + op->pdu[0] = op->value_len + 2;
>>> + op->pdu_len++;
>>> + } else if (len != op->value_len) {
>>> + op->done = true;
>>> + goto done;
>>> + }
>>> +
>>> + /* Stop if this would surpass the MTU */
>>> + if (op->pdu_len + op->value_len + 2 > mtu - 1) {
>>> + op->done = true;
>>> + goto done;
>>> + }
>>> +
>>> + /* Encode the current value */
>>> + put_le16(handle, op->pdu + op->pdu_len);
>>> + memcpy(op->pdu + op->pdu_len + 2, value, op->value_len);
>>> +
>>> + op->pdu_len += op->value_len + 2;
>>> +
>>> + if (op->pdu_len == mtu - 1)
>>> + op->done = true;
>>
>> The code above is duplicated and it is actually causing some warnings
>> comparing unsigned with signed (usually cause by '-' operation) which
>> I started fixing myself but stop once I realize this was the result of
>> gatt_db_read having 2 modes to read, sync and async, which I don't
>> think is a good idea.
>>
>
> Yes, please see my response to your other comment in this patch set.
>
>
>>> +done:
>>> + process_read_by_type(op);
>>> +}
>>> +
>>> +static void process_read_by_type(struct async_read_op *op)
>>> +{
>>> + struct bt_gatt_server *server = op->server;
>>> + uint16_t mtu = bt_att_get_mtu(server->att);
>>> + uint8_t rsp_opcode;
>>> + uint8_t rsp_len;
>>> + uint8_t ecode;
>>> + uint16_t ehandle;
>>> + uint16_t start_handle;
>>> + uint8_t *value;
>>> + int value_len;
>>> + uint32_t perm;
>>> +
>>> + if (op->done) {
>>> + rsp_opcode = BT_ATT_OP_READ_BY_TYPE_RSP;
>>> + rsp_len = op->pdu_len;
>>> + goto done;
>>> + }
>>> +
>>> + while (queue_peek_head(op->db_data)) {
>>> + start_handle = PTR_TO_UINT(queue_pop_head(op->db_data));
>>> + value = NULL;
>>> + value_len = 0;
>>> +
>>> + if (!gatt_db_get_attribute_permissions(server->db, start_handle,
>>> + &perm)) {
>>> + ecode = BT_ATT_ERROR_UNLIKELY;
>>> + goto error;
>>> + }
>>> +
>>> + /*
>>> + * Check for the READ access permission. Encryption,
>>> + * authentication, and authorization permissions need to be
>>> + * checked by the read handler, since bt_att is agnostic to
>>> + * connection type and doesn't have security information on it.
>>> + */
>>> + if (perm && !(perm & BT_ATT_PERM_READ)) {
>>> + ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
>>> + goto error;
>>> + }
>>
>> I would suggest moving this check to gatt_db_read which should return
>> -EPERM or the actual code if it is too hard to have errno mapping for
>> each error code.
>>
>
> gatt_db currently has this nice property that the permissions field is
> just a uint32_t. This means that while shared/gatt-server can use the
> new permission macros I added to shared/att-types, the android code
> can still use the Android platform definitions. Of course, once
> android starts using shared/gatt-server it'll have to use the new
> macros but for now we can have gatt-db support both types by keeping
> the permission checks outside of gatt-db and this would make the whole
> transition easier. Makes sense?
Well I would like to harmonize the definitions with Android as soon as
possible since you are defining new ones, which Ive applied btw,
otherwise later when we start using it in Android it might break
things, btw Im fine if the server actually handling the permissions,
what I really want is a central place to handle them.
--
Luiz Augusto von Dentz
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox