* [PATCHv2 2/3] android/gatt: Use g_attrib buffer where possible for att
2014-05-20 10:52 [PATCHv2 1/3] android/gatt: Send Exchange MTU request on connect Jakub Tyszkowski
@ 2014-05-20 10:52 ` Jakub Tyszkowski
2014-05-20 10:52 ` [PATCHv2 3/3] android/gatt: Fix possible invalid read Jakub Tyszkowski
2014-05-20 21:52 ` [PATCHv2 1/3] android/gatt: Send Exchange MTU request on connect Szymon Janc
2 siblings, 0 replies; 5+ messages in thread
From: Jakub Tyszkowski @ 2014-05-20 10:52 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Tyszkowski
This replaces fixed size pdu usage with g_attrib buffer when possible.
When only received packets are decoded we use dynamic allocation with
current mtu.
---
android/gatt.c | 109 +++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 83 insertions(+), 26 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 98a52d2..dcb347b 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -3570,7 +3570,8 @@ static bool is_service(const bt_uuid_t *type)
static void send_dev_pending_response(struct gatt_device *device,
uint8_t opcode)
{
- uint8_t rsp[ATT_DEFAULT_LE_MTU];
+ size_t mtu;
+ uint8_t *rsp = g_attrib_get_buffer(device->attrib, &mtu);
struct pending_request *val;
uint16_t len = 0;
uint8_t error = 0;
@@ -3615,7 +3616,7 @@ static void send_dev_pending_response(struct gatt_device *device,
val = queue_pop_head(temp);
}
- len = enc_read_by_type_resp(adl, rsp, sizeof(rsp));
+ len = enc_read_by_type_resp(adl, rsp, mtu);
att_data_list_free(adl);
queue_destroy(temp, destroy_pending_request);
@@ -3630,7 +3631,7 @@ static void send_dev_pending_response(struct gatt_device *device,
}
len = enc_read_blob_resp(val->value, val->length, val->offset,
- rsp, sizeof(rsp));
+ rsp, mtu);
destroy_pending_request(val);
break;
case ATT_OP_READ_REQ:
@@ -3640,7 +3641,7 @@ static void send_dev_pending_response(struct gatt_device *device,
goto done;
}
- len = enc_read_resp(val->value, val->length, rsp, sizeof(rsp));
+ len = enc_read_resp(val->value, val->length, rsp, mtu);
destroy_pending_request(val);
break;
case ATT_OP_READ_BY_GROUP_REQ: {
@@ -3686,7 +3687,7 @@ static void send_dev_pending_response(struct gatt_device *device,
val = queue_pop_head(temp);
}
- len = enc_read_by_grp_resp(adl, rsp, sizeof(rsp));
+ len = enc_read_by_grp_resp(adl, rsp, mtu);
att_data_list_free(adl);
queue_destroy(temp, destroy_pending_request);
@@ -3734,7 +3735,7 @@ static void send_dev_pending_response(struct gatt_device *device,
}
if (list && !error)
- len = enc_find_by_type_resp(list, rsp, sizeof(rsp));
+ len = enc_find_by_type_resp(list, rsp, mtu);
else
error = ATT_ECODE_ATTR_NOT_FOUND;
@@ -3770,7 +3771,7 @@ static void send_dev_pending_response(struct gatt_device *device,
}
len = enc_prep_write_resp(val->handle, val->offset, val->value,
- val->length, rsp, sizeof(rsp));
+ val->length, rsp, mtu);
destroy_pending_request(val);
break;
default:
@@ -3779,8 +3780,7 @@ static void send_dev_pending_response(struct gatt_device *device,
done:
if (!len)
- len = enc_error_resp(opcode, 0x0000, error, rsp,
- ATT_DEFAULT_LE_MTU);
+ len = enc_error_resp(opcode, 0x0000, error, rsp, mtu);
g_attrib_send(device->attrib, 0, rsp, len, NULL, NULL, NULL);
@@ -4216,10 +4216,11 @@ failed:
static void handle_server_send_indication(const void *buf, uint16_t len)
{
const struct hal_cmd_gatt_server_send_indication *cmd = buf;
- uint8_t pdu[ATT_DEFAULT_LE_MTU];
struct app_connection *conn;
uint8_t status;
uint16_t length;
+ uint8_t *pdu;
+ size_t mtu;
DBG("");
@@ -4230,15 +4231,17 @@ static void handle_server_send_indication(const void *buf, uint16_t len)
goto reply;
}
+ pdu = g_attrib_get_buffer(conn->device->attrib, &mtu);
+
if (cmd->confirm)
/* TODO: Add data to track confirmation for this request */
length = enc_indication(cmd->attribute_handle,
- (uint8_t *)cmd->value, cmd->len,
- pdu, sizeof(pdu));
+ (uint8_t *)cmd->value, cmd->len, pdu,
+ mtu);
else
length = enc_notification(cmd->attribute_handle,
(uint8_t *)cmd->value, cmd->len,
- pdu, sizeof(pdu));
+ pdu, mtu);
g_attrib_send(conn->device->attrib, 0, pdu, length, NULL, NULL, NULL);
@@ -4677,7 +4680,7 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
struct gatt_device *device)
{
- uint8_t search_value[ATT_DEFAULT_LE_MTU];
+ uint8_t *search_value;
size_t search_vlen;
uint16_t start, end;
uint16_t handle;
@@ -4687,14 +4690,24 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
DBG("");
+ search_value = malloc0(get_device_att_mtu(device));
+ if (!search_value)
+ return ATT_ECODE_INSUFF_RESOURCES;
+
len = dec_find_by_type_req(cmd, cmd_len, &start, &end, &uuid,
search_value, &search_vlen);
- if (!len)
+ if (!len) {
+ free(search_value);
+
return ATT_ECODE_INVALID_PDU;
+ }
q = queue_new();
- if (!q)
+ if (!q) {
+ free(search_value);
+
return ATT_ECODE_UNLIKELY;
+ }
gatt_db_find_by_type(gatt_db, start, end, &uuid, q);
@@ -4705,6 +4718,8 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
data = new0(struct pending_request, 1);
if (!data) {
queue_destroy(q, NULL);
+ free(search_value);
+
return ATT_ECODE_INSUFF_RESOURCES;
}
@@ -4712,6 +4727,8 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
if (!data) {
destroy_pending_request(data);
queue_destroy(q, NULL);
+ free(search_value);
+
return ATT_ECODE_INSUFF_RESOURCES;
}
@@ -4726,6 +4743,7 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
}
queue_destroy(q, NULL);
+ free(search_value);
process_dev_pending_requests(device, ATT_OP_FIND_BY_TYPE_REQ);
@@ -4735,18 +4753,30 @@ static uint8_t find_by_type_request(const uint8_t *cmd, uint16_t cmd_len,
static uint8_t write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
struct gatt_device *dev)
{
- uint8_t value[ATT_DEFAULT_LE_MTU];
+ uint8_t *value;
uint16_t handle;
uint16_t len;
size_t vlen;
+ value = malloc0(get_device_att_mtu(dev));
+ if (!value)
+ return ATT_ECODE_INSUFF_RESOURCES;
+
len = dec_write_cmd(cmd, cmd_len, &handle, value, &vlen);
- if (!len)
+ if (!len) {
+ free(value);
+
return ATT_ECODE_INVALID_PDU;
+ }
if (!gatt_db_write(gatt_db, handle, 0, value, vlen, cmd[0],
- &dev->bdaddr))
+ &dev->bdaddr)) {
+ free(value);
+
return ATT_ECODE_UNLIKELY;
+ }
+
+ free(value);
return 0;
}
@@ -4754,18 +4784,30 @@ static uint8_t write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
struct gatt_device *dev)
{
- uint8_t value[ATT_DEFAULT_LE_MTU];
+ uint8_t *value;
uint16_t handle;
uint16_t len;
size_t vlen;
+ value = malloc0(get_device_att_mtu(dev));
+ if (!value)
+ return ATT_ECODE_INSUFF_RESOURCES;
+
len = dec_write_req(cmd, cmd_len, &handle, value, &vlen);
- if (!len)
+ if (!len) {
+ free(value);
+
return ATT_ECODE_INVALID_PDU;
+ }
if (!gatt_db_write(gatt_db, handle, 0, value, vlen, cmd[0],
- &dev->bdaddr))
+ &dev->bdaddr)) {
+ free(value);
+
return ATT_ECODE_UNLIKELY;
+ }
+
+ free(value);
return 0;
}
@@ -4773,20 +4815,32 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
struct gatt_device *dev)
{
- uint8_t value[ATT_DEFAULT_LE_MTU];
+ uint8_t *value;
uint16_t handle;
uint16_t offset;
uint16_t len;
size_t vlen;
+ value = malloc0(get_device_att_mtu(dev));
+ if (!value)
+ return ATT_ECODE_INSUFF_RESOURCES;
+
len = dec_prep_write_req(cmd, cmd_len, &handle, &offset,
value, &vlen);
- if (!len)
+ if (!len) {
+ free(value);
+
return ATT_ECODE_INVALID_PDU;
+ }
if (!gatt_db_write(gatt_db, handle, offset, value, vlen, cmd[0],
- &dev->bdaddr))
+ &dev->bdaddr)) {
+ free(value);
+
return ATT_ECODE_UNLIKELY;
+ }
+
+ free(value);
return 0;
}
@@ -5169,9 +5223,10 @@ static void gatt_srvc_change_register_cb(uint16_t handle, uint16_t offset,
bdaddr_t *bdaddr,
void *user_data)
{
- uint8_t pdu[ATT_DEFAULT_LE_MTU];
struct gatt_device *dev;
uint16_t length;
+ size_t mtu;
+ uint8_t *pdu;
dev = find_device_by_addr(bdaddr);
if (!dev) {
@@ -5179,6 +5234,8 @@ static void gatt_srvc_change_register_cb(uint16_t handle, uint16_t offset,
return;
}
+ pdu = g_attrib_get_buffer(dev->attrib, &mtu);
+
/* TODO handle CCC */
/* Set services changed notification flag */
--
1.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCHv2 1/3] android/gatt: Send Exchange MTU request on connect
2014-05-20 10:52 [PATCHv2 1/3] android/gatt: Send Exchange MTU request on connect Jakub Tyszkowski
2014-05-20 10:52 ` [PATCHv2 2/3] android/gatt: Use g_attrib buffer where possible for att Jakub Tyszkowski
2014-05-20 10:52 ` [PATCHv2 3/3] android/gatt: Fix possible invalid read Jakub Tyszkowski
@ 2014-05-20 21:52 ` Szymon Janc
2 siblings, 0 replies; 5+ messages in thread
From: Szymon Janc @ 2014-05-20 21:52 UTC (permalink / raw)
To: Jakub Tyszkowski; +Cc: linux-bluetooth
Hi Jakub,
On Tuesday 20 May 2014 12:52:42 Jakub Tyszkowski wrote:
> This adds sending exchange MTU request on connection. If exchange MTU
> request is received when negotiating MTU as client, we send default MTU
> as described in spec, without setting the MTU. It will be set in
> response to our MTU exchange request.
>
> According to spec: "This request shall only be sent once during a
> connection by the client." This is needed to pass TC_GAC_CL_BV_01_C.
> ---
> android/gatt.c | 84
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> 84 insertions(+)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 8e0d72a..98a52d2 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -144,6 +144,7 @@ struct gatt_device {
>
> gatt_device_state_t state;
>
> + size_t negotiated_mtu;
> GAttrib *attrib;
> GIOChannel *att_io;
> struct queue *services;
> @@ -978,6 +979,80 @@ static void send_app_connect_notifications(void *data,
> void *user_data)
>
> static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer
> user_data);
>
> +static size_t get_device_att_mtu(struct gatt_device *device)
> +{
> + size_t mtu;
> +
> + g_attrib_get_buffer(device->attrib, &mtu);
> +
> + return mtu;
> +}
> +
> +static void exchange_mtu_cb(guint8 status, const guint8 *pdu, guint16 plen,
> + gpointer user_data)
> +{
> + struct gatt_device *device = user_data;
> + GError *gerr = NULL;
> + uint16_t cid, imtu;
> + GIOChannel *io;
> + uint16_t rmtu;
> +
> + if (status) {
> + error("gatt: MTU exchange: %s", att_ecode2str(status));
> + device->negotiated_mtu = 0;
> + goto done;
> + }
Use "failed" label and do cleanup there.
> +
> + if (!dec_mtu_resp(pdu, plen, &rmtu)) {
> + error("gatt: MTU exchange: protocol error");
> + device->negotiated_mtu = 0;
> + goto done;
> + }
> +
> + if (rmtu < ATT_DEFAULT_LE_MTU) {
> + error("gatt: MTU exchange: mtu error");
> + device->negotiated_mtu = 0;
> + goto done;
> + }
> +
> + io = g_attrib_get_channel(device->attrib);
> +
> + if (bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu, BT_IO_OPT_CID, &cid,
> + BT_IO_OPT_INVALID) && cid == ATT_CID) {
> + device->negotiated_mtu = MIN(rmtu, imtu);
Why not set it only if all steps succeed?
> + if (g_attrib_set_mtu(device->attrib, device->negotiated_mtu)) {
> + DBG("MTU exchange succeeded: %ld",
> + device->negotiated_mtu);
> + goto done;
> + }
Please use more common convention:
bt_io_get(io, &gerr,...);
if (gerr) {
error(..);
g_error_free(gerr);
}
There is also missing g_error_free().
> + }
> +
> + DBG("gatt: MTU exchange failed");
error() ?
> + device->negotiated_mtu = 0;
> +
> +done:
> + device_unref(device);
This is hard to follow what is error and success path here.
> +}
> +
> +static void send_exchange_mtu_request(struct gatt_device *device)
> +{
> + GError *gerr = NULL;
> + uint16_t cid, imtu;
> + GIOChannel *io;
> +
> + io = g_attrib_get_channel(device->attrib);
> +
> + if (bt_io_get(io, &gerr, BT_IO_OPT_IMTU, &imtu, BT_IO_OPT_CID, &cid,
> + BT_IO_OPT_INVALID) && cid == ATT_CID) {
Please use similar convention as mentioned above.
> + device->negotiated_mtu = imtu;
Why this needs to be set here? Also if this is set here...
> + if (!gatt_exchange_mtu(device->attrib, device->negotiated_mtu,
> + exchange_mtu_cb, device_ref(device))) {
> + device->negotiated_mtu = 0;
> + device_unref(device);
> + }
> + }
> +}
> +
> static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> {
> struct gatt_device *dev = user_data;
> @@ -1023,6 +1098,8 @@ static void connect_cb(GIOChannel *io, GError *gerr,
> gpointer user_data)
>
> status = GATT_SUCCESS;
>
> + send_exchange_mtu_request(dev);
> +
> reply:
> data.dev = dev;
> data.status = status;
> @@ -4497,6 +4574,12 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
> uint16_t cmd_len, if (mtu < ATT_DEFAULT_LE_MTU)
> return ATT_ECODE_REQ_NOT_SUPP;
>
> + /* If mtu negotiation is ongoing, send default MTU (See Spec 3.4.2.2) */
> + if (dev->negotiated_mtu) {
...then this check means negotiation is either ongoing or didn't failed.
This is in contradiction to what comment says.
Also if I get spec correctly using default MTU is about ATT Req being send
before received MTU Exchange Req, and not about MTU Exchange Response. Values
in Req and Resp should be same and here you change negotiated_mtu in
exchange_mtu_cb() meaning that we may reply with different values (default vs
imtu from bt_io).
> + imtu = get_device_att_mtu(dev);
> + goto done;
> + }
> +
> io = g_attrib_get_channel(dev->attrib);
>
> bt_io_get(io, &gerr,
> @@ -4513,6 +4596,7 @@ static uint8_t mtu_att_handle(const uint8_t *cmd,
> uint16_t cmd_len, mtu = MIN(mtu, omtu);
> g_attrib_set_mtu(dev->attrib, mtu);
>
> +done:
> /* Respond with our IMTU */
> len = enc_mtu_resp(imtu, rsp, rsp_size);
> if (!len)
--
Szymon K. Janc
szymon.janc@gmail.com
^ permalink raw reply [flat|nested] 5+ messages in thread