Change in osmo-pcu[master]: llc_queue::{dequeue,enqueue}() refactor

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Wed Feb 26 19:49:16 UTC 2020


pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/17282 )


Change subject: llc_queue::{dequeue,enqueue}() refactor
......................................................................

llc_queue::{dequeue,enqueue}() refactor

As seen in OS#4420, setting the MetaInfo.recv_time outside of
llc_queue before calling llc_queue::enqueue() and later on using that
value in llc_queue itself at dequeue time is not a good idea, since it
can provoke errors if the recv_time was not set correctly.
For instance, LlcTest was not setting the value for recv_time on some
test, which ended up with a huge millisec value when substracting now()
from it:
"""
llc.cpp:215:29: runtime error: signed integer overflow: 1582738663 * 1000 cannot be represented in type 'long int'
"""
This issue only appeared when started building on a raspberrypi4.

Let's better set/store the MetaInfo.recv_time internally during
llc_queue::enqueue(). Then, enqueue() only needs the
MetaInfo.expire_time, so let's change its arg list to only receive that
to avoid confusions.

Take the chance to move the llc_queue APIs to use osmo_gettimeofday,
since we need to fake the time now that the API itself sets that time.

Also take the chance during this refactor to disallow passing null
pointer by default since no user needs that.

Finally, update the LlcTest accordingly with all API/behavior changes.

Related: OS#4420
Change-Id: Ief6b1464dc779ff22adc2b02da7a006cd772ebce
---
M src/llc.cpp
M src/llc.h
M src/tbf_dl.cpp
M tests/llc/LlcTest.cpp
4 files changed, 55 insertions(+), 45 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/82/17282/1

diff --git a/src/llc.cpp b/src/llc.cpp
index b155063..4cd0cc4 100644
--- a/src/llc.cpp
+++ b/src/llc.cpp
@@ -103,18 +103,19 @@
 	m_avg_queue_delay = 0;
 }
 
-void gprs_llc_queue::enqueue(struct msgb *llc_msg, const MetaInfo *info)
+
+void gprs_llc_queue::enqueue(struct msgb *llc_msg, const struct timeval *expire_time)
 {
-	static const MetaInfo def_meta = {{0}};
 	MetaInfo *meta_storage;
 
-	osmo_static_assert(sizeof(*info) <= sizeof(llc_msg->cb), info_does_not_fit);
+	osmo_static_assert(sizeof(*meta_storage) <= sizeof(llc_msg->cb), info_does_not_fit);
 
 	m_queue_size += 1;
 	m_queue_octets += msgb_length(llc_msg);
 
 	meta_storage = (MetaInfo *)&llc_msg->cb[0];
-	*meta_storage = info ? *info : def_meta;
+	osmo_gettimeofday(&meta_storage->recv_time, NULL);
+	meta_storage->expire_time = *expire_time;
 
 	msgb_enqueue(&m_queue, llc_msg);
 }
@@ -208,7 +209,7 @@
 	m_queue_octets -= msgb_length(msg);
 
 	/* take the second time */
-	gettimeofday(&tv_now, NULL);
+	osmo_gettimeofday(&tv_now, NULL);
 	tv = (struct timeval *)&msg->data[sizeof(*tv)];
 	timersub(&tv_now, &meta_storage->recv_time, &tv_result);
 
@@ -234,7 +235,7 @@
 
 	/* calculate timestamp of timeout */
 	struct timeval now, csec;
-	gettimeofday(&now, NULL);
+	osmo_gettimeofday(&now, NULL);
 	csec.tv_usec = (delay_csec % 100) * 10000;
 	csec.tv_sec = delay_csec / 100;
 
diff --git a/src/llc.h b/src/llc.h
index 2e7229c..8667e00 100644
--- a/src/llc.h
+++ b/src/llc.h
@@ -75,7 +75,7 @@
 
 	void init();
 
-	void enqueue(struct msgb *llc_msg, const MetaInfo *info = 0);
+	void enqueue(struct msgb *llc_msg, const struct timeval *expire_time);
 	struct msgb *dequeue(const MetaInfo **info = 0);
 	void clear(BTS *bts);
 	void move_and_merge(gprs_llc_queue *o);
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index 7113d65..5c0fd9d 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -108,16 +108,17 @@
 				const uint16_t pdu_delay_csec,
 				const uint8_t *data, const uint16_t len)
 {
+	struct timeval expire_time;
+
 	LOGPTBFDL(this, LOGL_DEBUG, "appending %u bytes\n", len);
-	gprs_llc_queue::MetaInfo info;
+
 	struct msgb *llc_msg = msgb_alloc(len, "llc_pdu_queue");
 	if (!llc_msg)
 		return -ENOMEM;
 
-	gprs_llc_queue::calc_pdu_lifetime(bts, pdu_delay_csec, &info.expire_time);
-	gettimeofday(&info.recv_time, NULL);
+	gprs_llc_queue::calc_pdu_lifetime(bts, pdu_delay_csec, &expire_time);
 	memcpy(msgb_put(llc_msg, len), data, len);
-	llc_queue()->enqueue(llc_msg, &info);
+	llc_queue()->enqueue(llc_msg, &expire_time);
 	tbf_update_ms_class(this, ms_class);
 	start_llc_timer();
 
diff --git a/tests/llc/LlcTest.cpp b/tests/llc/LlcTest.cpp
index 8163f34..d3bd871 100644
--- a/tests/llc/LlcTest.cpp
+++ b/tests/llc/LlcTest.cpp
@@ -34,6 +34,7 @@
 #include <osmocom/core/msgb.h>
 #include <osmocom/core/talloc.h>
 #include <osmocom/core/utils.h>
+#include <osmocom/core/timer.h>
 #include <osmocom/vty/vty.h>
 }
 
@@ -43,7 +44,7 @@
 bool spoof_mnc_3_digits = false;
 
 static void enqueue_data(gprs_llc_queue *queue, const uint8_t *data, size_t len,
-	gprs_llc_queue::MetaInfo *info = 0)
+			 const struct timeval *expire_time)
 {
 	struct timeval *tv;
 	uint8_t *msg_data;
@@ -54,11 +55,11 @@
 
 	memcpy(msg_data, data, len);
 
-	queue->enqueue(llc_msg, info);
+	queue->enqueue(llc_msg, expire_time);
 }
 
 static void dequeue_and_check(gprs_llc_queue *queue, const uint8_t *exp_data,
-	size_t len, const gprs_llc_queue::MetaInfo *exp_info = 0)
+	size_t len, const gprs_llc_queue::MetaInfo *exp_info)
 {
 	struct msgb *llc_msg;
 	const gprs_llc_queue::MetaInfo *info_res;
@@ -73,15 +74,16 @@
 		fprintf(stderr, "check failed!\n");
 
 	if (exp_info)
-		OSMO_ASSERT(memcmp(exp_info, info_res, sizeof(*exp_info)) == 0);
+		OSMO_ASSERT(memcmp(&exp_info->recv_time, &info_res->recv_time, sizeof(info_res->recv_time)) == 0 &&
+			    memcmp(&exp_info->expire_time, &info_res->expire_time, sizeof(info_res->expire_time)) == 0);
 
 	msgb_free(llc_msg);
 }
 
 static void enqueue_data(gprs_llc_queue *queue, const char *message,
-	gprs_llc_queue::MetaInfo *info = 0)
+			 const struct timeval *expire_time)
 {
-	enqueue_data(queue, (uint8_t *)(message), strlen(message), info);
+	enqueue_data(queue, (uint8_t *)(message), strlen(message), expire_time);
 }
 
 static void dequeue_and_check(gprs_llc_queue *queue, const char *exp_message,
@@ -94,6 +96,7 @@
 static void test_llc_queue()
 {
 	gprs_llc_queue queue;
+	struct timeval expire_time = {0};
 
 	printf("=== start %s ===\n", __func__);
 
@@ -101,11 +104,11 @@
 	OSMO_ASSERT(queue.size() == 0);
 	OSMO_ASSERT(queue.octets() == 0);
 
-	enqueue_data(&queue, "LLC message");
+	enqueue_data(&queue, "LLC message", &expire_time);
 	OSMO_ASSERT(queue.size() == 1);
 	OSMO_ASSERT(queue.octets() == 11);
 
-	enqueue_data(&queue, "other LLC message");
+	enqueue_data(&queue, "other LLC message", &expire_time);
 	OSMO_ASSERT(queue.size() == 2);
 	OSMO_ASSERT(queue.octets() == 28);
 
@@ -117,7 +120,7 @@
 	OSMO_ASSERT(queue.size() == 0);
 	OSMO_ASSERT(queue.octets() == 0);
 
-	enqueue_data(&queue, "LLC");
+	enqueue_data(&queue, "LLC",  &expire_time);
 	OSMO_ASSERT(queue.size() == 1);
 	OSMO_ASSERT(queue.octets() == 3);
 
@@ -131,18 +134,8 @@
 static void test_llc_meta()
 {
 	gprs_llc_queue queue;
-	gprs_llc_queue::MetaInfo info1;
-	gprs_llc_queue::MetaInfo info2;
-
-	info1.recv_time.tv_sec    = 123456777;
-	info1.recv_time.tv_usec   = 123456;
-	info1.expire_time.tv_sec  = 123456789;
-	info1.expire_time.tv_usec = 987654;
-
-	info2.recv_time.tv_sec    = 987654321;
-	info2.recv_time.tv_usec   = 547352;
-	info2.expire_time.tv_sec  = 987654327;
-	info2.expire_time.tv_usec = 867252;
+	gprs_llc_queue::MetaInfo info1 = {0};
+	gprs_llc_queue::MetaInfo info2 = {0};
 
 	printf("=== start %s ===\n", __func__);
 
@@ -150,8 +143,19 @@
 	OSMO_ASSERT(queue.size() == 0);
 	OSMO_ASSERT(queue.octets() == 0);
 
-	enqueue_data(&queue, "LLC message 1", &info1);
-	enqueue_data(&queue, "LLC message 2", &info2);
+	info1.recv_time.tv_sec = 123456777;
+	info1.recv_time.tv_usec = 123456;
+	info1.expire_time.tv_sec = 123456789;
+	info1.expire_time.tv_usec = 987654;
+	osmo_gettimeofday_override_time = info1.recv_time;
+	enqueue_data(&queue, "LLC message 1", &info1.expire_time);
+
+	info2.recv_time.tv_sec = 987654321;
+	info2.recv_time.tv_usec = 547352;
+	info2.expire_time.tv_sec = 987654327;
+	info2.expire_time.tv_usec = 867252;
+	osmo_gettimeofday_override_time = info2.recv_time;
+	enqueue_data(&queue, "LLC message 2", &info2.expire_time);
 
 	dequeue_and_check(&queue, "LLC message 1", &info1);
 	dequeue_and_check(&queue, "LLC message 2", &info2);
@@ -167,27 +171,27 @@
 {
 	gprs_llc_queue queue1;
 	gprs_llc_queue queue2;
-	gprs_llc_queue::MetaInfo info = {0};
+	struct timeval expire_time = {0};
 
 	printf("=== start %s ===\n", __func__);
 
 	queue1.init();
 	queue2.init();
 
-	info.recv_time.tv_sec += 1;
-	enqueue_data(&queue1, "*A*", &info);
+	osmo_gettimeofday_override_time.tv_sec += 1;
+	enqueue_data(&queue1, "*A*", &expire_time);
 
-	info.recv_time.tv_sec += 1;
-	enqueue_data(&queue1, "*B*", &info);
+	osmo_gettimeofday_override_time.tv_sec += 1;
+	enqueue_data(&queue1, "*B*", &expire_time);
 
-	info.recv_time.tv_sec += 1;
-	enqueue_data(&queue2, "*C*", &info);
+	osmo_gettimeofday_override_time.tv_sec += 1;
+	enqueue_data(&queue2, "*C*", &expire_time);
 
-	info.recv_time.tv_sec += 1;
-	enqueue_data(&queue1, "*D*", &info);
+	osmo_gettimeofday_override_time.tv_sec += 1;
+	enqueue_data(&queue1, "*D*", &expire_time);
 
-	info.recv_time.tv_sec += 1;
-	enqueue_data(&queue2, "*E*", &info);
+	osmo_gettimeofday_override_time.tv_sec += 1;
+	enqueue_data(&queue2, "*E*", &expire_time);
 
 	OSMO_ASSERT(queue1.size() == 3);
 	OSMO_ASSERT(queue1.octets() == 9);
@@ -231,6 +235,10 @@
 	vty_init(&pcu_vty_info);
 	pcu_vty_init();
 
+	osmo_gettimeofday_override = true;
+	osmo_gettimeofday_override_time.tv_sec = 123456777;
+	osmo_gettimeofday_override_time.tv_usec = 123456;
+
 	test_llc_queue();
 	test_llc_meta();
 	test_llc_merge();

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/17282
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Ief6b1464dc779ff22adc2b02da7a006cd772ebce
Gerrit-Change-Number: 17282
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200226/1bdf0381/attachment.htm>


More information about the gerrit-log mailing list