You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@mynewt.apache.org by "sjanc (via GitHub)" <gi...@apache.org> on 2026/05/12 08:58:23 UTC
[PR] nimble/host: Refactor GATT Read Multiple Variable Char Value response [mynewt-nimble]
sjanc opened a new pull request, #2231:
URL: https://github.com/apache/mynewt-nimble/pull/2231
This reworks ATT_READ_MULTIPLE_VARIABLE_RSP parsing. If response doesn't match request (number of values requested) or LV parsing shows incorrect format we ignore that and report error to app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nimble/host: Refactor GATT Read Multiple Variable Char Value response [mynewt-nimble]
Posted by "rymanluk (via GitHub)" <gi...@apache.org>.
rymanluk commented on code in PR #2231:
URL: https://github.com/apache/mynewt-nimble/pull/2231#discussion_r3227446315
##########
nimble/host/src/ble_gattc.c:
##########
@@ -3361,49 +3361,62 @@ ble_gattc_read_mult_cb_var(struct ble_gattc_proc *proc, int status,
for (i = 0; i < proc->read_mult.num_handles; i++) {
attr[i].handle = proc->read_mult.handles[i];
attr[i].offset = 0;
- if (om == NULL || OS_MBUF_PKTLEN(*om) == 0) {
- continue;
- }
+ }
- *om = os_mbuf_pullup(*om, 2);
- assert(*om);
+ if (status == 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ if (OS_MBUF_PKTLEN(*om) < 2) {
+ break;
+ }
- attr_len = get_le16((*om)->om_data);
+ *om = os_mbuf_pullup(*om, 2);
+ if (*om == NULL) {
+ break;
+ }
- os_mbuf_adj(*om, 2);
+ attr_len = get_le16((*om)->om_data);
+ os_mbuf_adj(*om, 2);
- if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
- /*TODO Figure out what to do here */
- break;
- }
+ if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
+ break;
+ }
- attr[i].om = os_msys_get_pkthdr(attr_len, 0);
- if (!attr[i].om) {
- /*TODO Figure out what to do here */
- break;
- }
+ attr[i].om = os_msys_get_pkthdr(attr_len, 0);
+ if (!attr[i].om) {
+ /* this is OOM condition*/
+ status = BLE_HS_ENOMEM;
+ break;
+ }
- rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
- if (rc) {
- /*TODO Figure out what to do here */
- break;
+ rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
+ if (rc) {
+ break;
+ }
+
+ os_mbuf_adj(*om, attr_len);
}
- os_mbuf_adj(*om, attr_len);
- }
+ /* failed to correctly parse response,
+ * cleanup any partial data and set status if not set already
+ */
+ if (i != proc->read_mult.num_handles || OS_MBUF_PKTLEN(*om) != 0) {
Review Comment:
nit: i < proc->read_mult.num_handles
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nimble/host: Refactor GATT Read Multiple Variable Char Value response [mynewt-nimble]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc merged PR #2231:
URL: https://github.com/apache/mynewt-nimble/pull/2231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nimble/host: Refactor GATT Read Multiple Variable Char Value response [mynewt-nimble]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on code in PR #2231:
URL: https://github.com/apache/mynewt-nimble/pull/2231#discussion_r3225295859
##########
nimble/host/src/ble_gattc.c:
##########
@@ -3361,49 +3361,59 @@ ble_gattc_read_mult_cb_var(struct ble_gattc_proc *proc, int status,
for (i = 0; i < proc->read_mult.num_handles; i++) {
attr[i].handle = proc->read_mult.handles[i];
attr[i].offset = 0;
- if (om == NULL || OS_MBUF_PKTLEN(*om) == 0) {
- continue;
- }
+ }
- *om = os_mbuf_pullup(*om, 2);
- assert(*om);
+ if (status == 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ if (OS_MBUF_PKTLEN(*om) < 2) {
+ break;
+ }
- attr_len = get_le16((*om)->om_data);
+ *om = os_mbuf_pullup(*om, 2);
+ if (*om == NULL) {
+ break;
+ }
- os_mbuf_adj(*om, 2);
+ attr_len = get_le16((*om)->om_data);
+ os_mbuf_adj(*om, 2);
- if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
- /*TODO Figure out what to do here */
- break;
- }
+ if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
+ break;
+ }
- attr[i].om = os_msys_get_pkthdr(attr_len, 0);
- if (!attr[i].om) {
- /*TODO Figure out what to do here */
- break;
- }
+ attr[i].om = os_msys_get_pkthdr(attr_len, 0);
+ if (!attr[i].om) {
+ break;
+ }
- rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
- if (rc) {
- /*TODO Figure out what to do here */
- break;
+ rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
+ if (rc) {
+ break;
+ }
+
+ os_mbuf_adj(*om, attr_len);
}
- os_mbuf_adj(*om, attr_len);
- }
+ /* failed to correctly parse response,
+ * cleanup any partial data and set status
+ */
+ if (i != proc->read_mult.num_handles || OS_MBUF_PKTLEN(*om) != 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ os_mbuf_free_chain(attr[i].om);
+ attr[i].om = NULL;
+ }
- /*FIXME Testing assert */
- assert(i == proc->read_mult.num_handles);
+ status = BLE_HS_EBADDATA;
+ }
+ }
proc->read_mult.cb_mult(proc->conn_handle,
- ble_gattc_error(status, att_handle), &attr[0],
- i,
- proc->read_mult.cb_arg);
+ ble_gattc_error(status, att_handle), &attr[0],
+ proc->read_mult.num_handles,
+ proc->read_mult.cb_arg);
for (i = 0; i < proc->read_mult.num_handles; i++) {
- if (attr[i].om != NULL) {
- os_mbuf_free_chain(attr[i].om);
- }
+ os_mbuf_free_chain(attr[i].om);
Review Comment:
os_mbuf_free_chain() handles NULL (just like free() does) on its own
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nimble/host: Refactor GATT Read Multiple Variable Char Value response [mynewt-nimble]
Posted by "MariuszSkamra (via GitHub)" <gi...@apache.org>.
MariuszSkamra commented on code in PR #2231:
URL: https://github.com/apache/mynewt-nimble/pull/2231#discussion_r3225254610
##########
nimble/host/src/ble_gattc.c:
##########
@@ -3361,49 +3361,59 @@ ble_gattc_read_mult_cb_var(struct ble_gattc_proc *proc, int status,
for (i = 0; i < proc->read_mult.num_handles; i++) {
attr[i].handle = proc->read_mult.handles[i];
attr[i].offset = 0;
- if (om == NULL || OS_MBUF_PKTLEN(*om) == 0) {
- continue;
- }
+ }
- *om = os_mbuf_pullup(*om, 2);
- assert(*om);
+ if (status == 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ if (OS_MBUF_PKTLEN(*om) < 2) {
+ break;
+ }
- attr_len = get_le16((*om)->om_data);
+ *om = os_mbuf_pullup(*om, 2);
+ if (*om == NULL) {
+ break;
+ }
- os_mbuf_adj(*om, 2);
+ attr_len = get_le16((*om)->om_data);
+ os_mbuf_adj(*om, 2);
- if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
- /*TODO Figure out what to do here */
- break;
- }
+ if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
+ break;
+ }
- attr[i].om = os_msys_get_pkthdr(attr_len, 0);
- if (!attr[i].om) {
- /*TODO Figure out what to do here */
- break;
- }
+ attr[i].om = os_msys_get_pkthdr(attr_len, 0);
+ if (!attr[i].om) {
+ break;
+ }
- rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
- if (rc) {
- /*TODO Figure out what to do here */
- break;
+ rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
+ if (rc) {
+ break;
+ }
+
+ os_mbuf_adj(*om, attr_len);
}
- os_mbuf_adj(*om, attr_len);
- }
+ /* failed to correctly parse response,
+ * cleanup any partial data and set status
+ */
+ if (i != proc->read_mult.num_handles || OS_MBUF_PKTLEN(*om) != 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ os_mbuf_free_chain(attr[i].om);
+ attr[i].om = NULL;
+ }
- /*FIXME Testing assert */
- assert(i == proc->read_mult.num_handles);
+ status = BLE_HS_EBADDATA;
+ }
+ }
proc->read_mult.cb_mult(proc->conn_handle,
- ble_gattc_error(status, att_handle), &attr[0],
- i,
- proc->read_mult.cb_arg);
+ ble_gattc_error(status, att_handle), &attr[0],
Review Comment:
Or maybe not. The ble_gattc_error() function name seems to be misleading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nimble/host: Refactor GATT Read Multiple Variable Char Value response [mynewt-nimble]
Posted by "MariuszSkamra (via GitHub)" <gi...@apache.org>.
MariuszSkamra commented on code in PR #2231:
URL: https://github.com/apache/mynewt-nimble/pull/2231#discussion_r3225200595
##########
nimble/host/src/ble_gattc.c:
##########
@@ -3361,49 +3361,59 @@ ble_gattc_read_mult_cb_var(struct ble_gattc_proc *proc, int status,
for (i = 0; i < proc->read_mult.num_handles; i++) {
attr[i].handle = proc->read_mult.handles[i];
attr[i].offset = 0;
- if (om == NULL || OS_MBUF_PKTLEN(*om) == 0) {
- continue;
- }
+ }
- *om = os_mbuf_pullup(*om, 2);
- assert(*om);
+ if (status == 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ if (OS_MBUF_PKTLEN(*om) < 2) {
+ break;
+ }
- attr_len = get_le16((*om)->om_data);
+ *om = os_mbuf_pullup(*om, 2);
+ if (*om == NULL) {
+ break;
+ }
- os_mbuf_adj(*om, 2);
+ attr_len = get_le16((*om)->om_data);
+ os_mbuf_adj(*om, 2);
- if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
- /*TODO Figure out what to do here */
- break;
- }
+ if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
+ break;
+ }
- attr[i].om = os_msys_get_pkthdr(attr_len, 0);
- if (!attr[i].om) {
- /*TODO Figure out what to do here */
- break;
- }
+ attr[i].om = os_msys_get_pkthdr(attr_len, 0);
+ if (!attr[i].om) {
+ break;
+ }
- rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
- if (rc) {
- /*TODO Figure out what to do here */
- break;
+ rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
+ if (rc) {
+ break;
+ }
+
+ os_mbuf_adj(*om, attr_len);
}
- os_mbuf_adj(*om, attr_len);
- }
+ /* failed to correctly parse response,
+ * cleanup any partial data and set status
+ */
+ if (i != proc->read_mult.num_handles || OS_MBUF_PKTLEN(*om) != 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ os_mbuf_free_chain(attr[i].om);
+ attr[i].om = NULL;
+ }
- /*FIXME Testing assert */
- assert(i == proc->read_mult.num_handles);
+ status = BLE_HS_EBADDATA;
+ }
+ }
proc->read_mult.cb_mult(proc->conn_handle,
- ble_gattc_error(status, att_handle), &attr[0],
- i,
- proc->read_mult.cb_arg);
+ ble_gattc_error(status, att_handle), &attr[0],
+ proc->read_mult.num_handles,
+ proc->read_mult.cb_arg);
for (i = 0; i < proc->read_mult.num_handles; i++) {
- if (attr[i].om != NULL) {
- os_mbuf_free_chain(attr[i].om);
- }
+ os_mbuf_free_chain(attr[i].om);
Review Comment:
If the mbuf allocation fails in 3384, the attr[i].om will be NULL, or the buffer is dropped in 3402. I think the check should not be removed.
##########
nimble/host/src/ble_gattc.c:
##########
@@ -3361,49 +3361,59 @@ ble_gattc_read_mult_cb_var(struct ble_gattc_proc *proc, int status,
for (i = 0; i < proc->read_mult.num_handles; i++) {
attr[i].handle = proc->read_mult.handles[i];
attr[i].offset = 0;
- if (om == NULL || OS_MBUF_PKTLEN(*om) == 0) {
- continue;
- }
+ }
- *om = os_mbuf_pullup(*om, 2);
- assert(*om);
+ if (status == 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ if (OS_MBUF_PKTLEN(*om) < 2) {
+ break;
+ }
- attr_len = get_le16((*om)->om_data);
+ *om = os_mbuf_pullup(*om, 2);
+ if (*om == NULL) {
+ break;
+ }
- os_mbuf_adj(*om, 2);
+ attr_len = get_le16((*om)->om_data);
+ os_mbuf_adj(*om, 2);
- if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
- /*TODO Figure out what to do here */
- break;
- }
+ if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
+ break;
+ }
- attr[i].om = os_msys_get_pkthdr(attr_len, 0);
- if (!attr[i].om) {
- /*TODO Figure out what to do here */
- break;
- }
+ attr[i].om = os_msys_get_pkthdr(attr_len, 0);
+ if (!attr[i].om) {
+ break;
+ }
- rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
- if (rc) {
- /*TODO Figure out what to do here */
- break;
+ rc = os_mbuf_appendfrom(attr[i].om, *om, 0, attr_len);
+ if (rc) {
+ break;
+ }
+
+ os_mbuf_adj(*om, attr_len);
}
- os_mbuf_adj(*om, attr_len);
- }
+ /* failed to correctly parse response,
+ * cleanup any partial data and set status
+ */
+ if (i != proc->read_mult.num_handles || OS_MBUF_PKTLEN(*om) != 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ os_mbuf_free_chain(attr[i].om);
+ attr[i].om = NULL;
+ }
- /*FIXME Testing assert */
- assert(i == proc->read_mult.num_handles);
+ status = BLE_HS_EBADDATA;
+ }
+ }
proc->read_mult.cb_mult(proc->conn_handle,
- ble_gattc_error(status, att_handle), &attr[0],
- i,
- proc->read_mult.cb_arg);
+ ble_gattc_error(status, att_handle), &attr[0],
Review Comment:
The status set in 3406 (BLE_HS_EBADDATA) is passed to ble_gattc_error() function which is bad IMO.
##########
nimble/host/src/ble_gattc.c:
##########
@@ -3361,49 +3361,59 @@ ble_gattc_read_mult_cb_var(struct ble_gattc_proc *proc, int status,
for (i = 0; i < proc->read_mult.num_handles; i++) {
attr[i].handle = proc->read_mult.handles[i];
attr[i].offset = 0;
- if (om == NULL || OS_MBUF_PKTLEN(*om) == 0) {
- continue;
- }
+ }
- *om = os_mbuf_pullup(*om, 2);
- assert(*om);
+ if (status == 0) {
+ for (i = 0; i < proc->read_mult.num_handles; i++) {
+ if (OS_MBUF_PKTLEN(*om) < 2) {
+ break;
+ }
- attr_len = get_le16((*om)->om_data);
+ *om = os_mbuf_pullup(*om, 2);
+ if (*om == NULL) {
+ break;
+ }
- os_mbuf_adj(*om, 2);
+ attr_len = get_le16((*om)->om_data);
+ os_mbuf_adj(*om, 2);
- if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
- /*TODO Figure out what to do here */
- break;
- }
+ if (attr_len > BLE_ATT_ATTR_MAX_LEN) {
+ break;
+ }
- attr[i].om = os_msys_get_pkthdr(attr_len, 0);
- if (!attr[i].om) {
- /*TODO Figure out what to do here */
- break;
- }
+ attr[i].om = os_msys_get_pkthdr(attr_len, 0);
+ if (!attr[i].om) {
+ break;
+ }
Review Comment:
This will result in BLE_HS_EBADDATA error, which can be misleading I think. Consider BLE_HS_ENOMEM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org