pespin has submitted this change. ( 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(-) 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 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 https://gerrit.osmocom.org/c/libosmo-gprs/+/35882?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-gprs Gerrit-Branch: master Gerrit-Change-Id: I6d4ae377a60839f76295c0e7e6aaa1fe53e37315 Gerrit-Change-Number: 35882 Gerrit-PatchSet: 1 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: jolly &lt;andreas(a)eversberg.eu&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged