pespin submitted this change.

View Change


Approvals: jolly: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve Jenkins Builder: Verified pespin: Looks good to me, approved
tbf_ul: Clean and assert code path generating new UL data block

Clarify the several paths when generating a new BSN (UL data block).
The function create_new_bsn() is only called if there's some LLC frame
ready to be transmitted and put in ul_tbf->llc_tx_msg, so the do while
{} condition checking for no llc_tx_msg is guaranteed to only be run in
the event some LLC frame chunks have been included in the UL RLC block.
This fact allows simplifying the code and at the same time fix a
Coverity CID regarding a code path which could not really happen. We now
guard this case with an ASSERT.

Related: Coverity CID#345063
Change-Id: I6d4ae377a60839f76295c0e7e6aaa1fe53e37315
---
M src/rlcmac/tbf_ul.c
1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/src/rlcmac/tbf_ul.c b/src/rlcmac/tbf_ul.c
index 0ea0a9c..67c9e98 100644
--- a/src/rlcmac/tbf_ul.c
+++ b/src/rlcmac/tbf_ul.c
@@ -684,6 +684,7 @@

if (!ul_tbf->llc_tx_msg || msgb_length(ul_tbf->llc_tx_msg) == 0)
gprs_rlcmac_ul_tbf_schedule_next_llc_frame(ul_tbf);
+ /* This function shall only be called if there's some LLC payload not yet transmitted: */
OSMO_ASSERT(ul_tbf->llc_tx_msg);

OSMO_ASSERT(gprs_rlcmac_mcs_is_valid(cs));
@@ -734,27 +735,29 @@
int payload_written = 0;

if (!ul_tbf->llc_tx_msg || msgb_length(ul_tbf->llc_tx_msg) == 0) {
+ const int space = block_data_len - write_offset;
+ /* Only get here in case we already encoded the end of
+ * some LLC payload chunk and there's no more new LLC
+ * payloads to send */
+ OSMO_ASSERT(num_chunks > 0);
+
/* The data just drained, store the current fn */
if (ul_tbf->last_ul_drained_fn < 0)
ul_tbf->last_ul_drained_fn = bi->fn;

- int space = block_data_len - write_offset;
+ /* Nothing to send, and we already put some data in
+ * rlcmac data block, we are done */
+ LOGPTBFUL(ul_tbf, LOGL_DEBUG,
+ "LLC queue completely drained and there's "
+ "still %d free bytes in rlcmac data block\n", space);

- if (num_chunks != 0) {
- /* Nothing to send, and we already put some data in
- * rlcmac data block, we are done */
- LOGPTBFUL(ul_tbf, LOGL_DEBUG,
- "LLC queue completely drained and there's "
- "still %d free bytes in rlcmac data block\n", space);
-
- if (gprs_rlcmac_mcs_is_edge(cs)) {
- /* in EGPRS there's no M bit, so we need
- * to flag padding with LI=127 */
- gprs_rlcmac_rlc_data_to_ul_append_egprs_li_padding(
- rdbi, &write_offset, &num_chunks, data);
- }
- break;
+ if (gprs_rlcmac_mcs_is_edge(cs)) {
+ /* in EGPRS there's no M bit, so we need to flag
+ * padding with LI=127 */
+ gprs_rlcmac_rlc_data_to_ul_append_egprs_li_padding(
+ rdbi, &write_offset, &num_chunks, data);
}
+ break;
}

ar = gprs_rlcmac_enc_append_ul_data(rdbi, cs, ul_tbf->llc_tx_msg, &write_offset,

To view, visit change 35882. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: I6d4ae377a60839f76295c0e7e6aaa1fe53e37315
Gerrit-Change-Number: 35882
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin@sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de>
Gerrit-Reviewer: jolly <andreas@eversberg.eu>
Gerrit-Reviewer: pespin <pespin@sysmocom.de>
Gerrit-MessageType: merged