pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/35882?usp=email )

Change subject: tbf_ul: Clean and assert code path generating new UL data block ......................................................................

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(-)

git pull ssh://gerrit.osmocom.org:29418/libosmo-gprs refs/changes/82/35882/1

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,