Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1413838861-29956-1-git-send-email-armansito@chromium.org> <1413838861-29956-7-git-send-email-armansito@chromium.org> Date: Fri, 24 Oct 2014 10:12:20 -0700 Message-ID: Subject: Re: [PATCH BlueZ 6/8] shared/gatt-server: Implement "Read By Type" request. From: Arman Uguray To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, On Fri, Oct 24, 2014 at 2:25 AM, Luiz Augusto von Dentz wrote: > Hi Arman, > > On Tue, Oct 21, 2014 at 12:00 AM, Arman Uguray 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