[PATCH] osmo-pcu[master]: tbf: add llc_queue_size() to check llc_queue is valid before...

[Neels Hofmeyr]

Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/724

to look at the new patch set (#4).

tbf: add llc_queue_size() to check llc_queue is valid before calling size()

gcc6 is optimizing if (!this) {CODE} as this is assumed to never be a
std::nullptr here. Move the null check to the caller. In preparation of
removing the check within llc_queue->size(), all callers must check the object
before calling it. Make sure of that: make the llc_queue() access function
protected and offer only a public llc_queue_size() function that incorporates
the NULL check. All current callers are only interested in the
llc_queue_size().

Tweaked-by: nhofmeyr
Change-Id: I88cc3180f8f86785e3f07981895dabddf50b60a2
---
M src/tbf.cpp
M src/tbf.h
M src/tbf_dl.cpp
3 files changed, 15 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/24/724/4

diff --git a/src/tbf.cpp b/src/tbf.cpp
index 97696cb..48d89b9 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -189,6 +189,13 @@
 	return m_ms ? m_ms->llc_queue() : NULL;
 }
 
+int gprs_rlcmac_tbf::llc_queue_size() const
+{
+	/* m_ms->llc_queue() never returns NULL: GprsMs::m_llc_queue is a
+	 * member instance. */
+	return m_ms ? m_ms->llc_queue()->size() : 0;
+}
+
 void gprs_rlcmac_tbf::set_ms(GprsMs *ms)
 {
 	if (m_ms == ms)
diff --git a/src/tbf.h b/src/tbf.h
index 3a6f42d..37401bf 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -142,8 +142,7 @@
 	uint8_t ms_class() const;
 	void set_ms_class(uint8_t);
 	GprsCodingScheme current_cs() const;
-	gprs_llc_queue *llc_queue();
-	const gprs_llc_queue *llc_queue() const;
+	int llc_queue_size() const;
 
 	time_t created_ts() const;
 	uint8_t dl_slots() const;
@@ -231,6 +230,9 @@
 	int set_tlli_from_ul(uint32_t new_tlli);
 	void merge_and_clear_ms(GprsMs *old_ms);
 
+	gprs_llc_queue *llc_queue();
+	const gprs_llc_queue *llc_queue() const;
+
 	static const char *tbf_state_name[7];
 
 	class GprsMs *m_ms;
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index 457f2c9..c89cd03 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -310,7 +310,7 @@
 		LOGP(DRLCMACDL, LOGL_NOTICE, "%s Discarding LLC PDU "
 			"because lifetime limit reached, "
 			"count=%u new_queue_size=%zu\n",
-			tbf_name(this), frames, llc_queue()->size());
+			tbf_name(this), frames, llc_queue_size());
 		if (frames > 0xff)
 			frames = 0xff;
 		if (octets > 0xffffff)
@@ -572,7 +572,7 @@
 				m_llc.frame_length(), frames_since_last_drain(fn));
 		}
 
-		is_final = llc_queue()->size() == 0 && !keep_open(fn);
+		is_final = llc_queue_size() == 0 && !keep_open(fn);
 
 		ar = Encoding::rlc_data_to_dl_append(rdbi, cs,
 			&m_llc, &write_offset, &num_chunks, data, is_final, &payload_written);
@@ -1050,7 +1050,7 @@
 	release();
 
 	/* check for LLC PDU in the LLC Queue */
-	if (llc_queue()->size() > 0)
+	if (llc_queue_size() > 0)
 		/* we have more data so we will re-use this tbf */
 		establish_dl_tbf_on_pacch();
 
@@ -1168,7 +1168,7 @@
 bool gprs_rlcmac_dl_tbf::have_data() const
 {
 	return m_llc.chunk_size() > 0 ||
-		(llc_queue() && llc_queue()->size() > 0);
+		(llc_queue_size() > 0);
 }
 
 int gprs_rlcmac_dl_tbf::frames_since_last_poll(unsigned fn) const

-- 
To view, visit https://gerrit.osmocom.org/724
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88cc3180f8f86785e3f07981895dabddf50b60a2
Gerrit-PatchSet: 4
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: lynxis lazus <lynxis at fe80.eu>