<p>pespin has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17282">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">llc_queue::{dequeue,enqueue}() refactor<br><br>As seen in OS#4420, setting the MetaInfo.recv_time outside of<br>llc_queue before calling llc_queue::enqueue() and later on using that<br>value in llc_queue itself at dequeue time is not a good idea, since it<br>can provoke errors if the recv_time was not set correctly.<br>For instance, LlcTest was not setting the value for recv_time on some<br>test, which ended up with a huge millisec value when substracting now()<br>from it:<br>"""<br>llc.cpp:215:29: runtime error: signed integer overflow: 1582738663 * 1000 cannot be represented in type 'long int'<br>"""<br>This issue only appeared when started building on a raspberrypi4.<br><br>Let's better set/store the MetaInfo.recv_time internally during<br>llc_queue::enqueue(). Then, enqueue() only needs the<br>MetaInfo.expire_time, so let's change its arg list to only receive that<br>to avoid confusions.<br><br>Take the chance to move the llc_queue APIs to use osmo_gettimeofday,<br>since we need to fake the time now that the API itself sets that time.<br><br>Also take the chance during this refactor to disallow passing null<br>pointer by default since no user needs that.<br><br>Finally, update the LlcTest accordingly with all API/behavior changes.<br><br>Related: OS#4420<br>Change-Id: Ief6b1464dc779ff22adc2b02da7a006cd772ebce<br>---<br>M src/llc.cpp<br>M src/llc.h<br>M src/tbf_dl.cpp<br>M tests/llc/LlcTest.cpp<br>4 files changed, 55 insertions(+), 45 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/82/17282/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/llc.cpp b/src/llc.cpp</span><br><span>index b155063..4cd0cc4 100644</span><br><span>--- a/src/llc.cpp</span><br><span>+++ b/src/llc.cpp</span><br><span>@@ -103,18 +103,19 @@</span><br><span> m_avg_queue_delay = 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-void gprs_llc_queue::enqueue(struct msgb *llc_msg, const MetaInfo *info)</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+void gprs_llc_queue::enqueue(struct msgb *llc_msg, const struct timeval *expire_time)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- static const MetaInfo def_meta = {{0}};</span><br><span> MetaInfo *meta_storage;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- osmo_static_assert(sizeof(*info) <= sizeof(llc_msg->cb), info_does_not_fit);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_static_assert(sizeof(*meta_storage) <= sizeof(llc_msg->cb), info_does_not_fit);</span><br><span> </span><br><span> m_queue_size += 1;</span><br><span> m_queue_octets += msgb_length(llc_msg);</span><br><span> </span><br><span> meta_storage = (MetaInfo *)&llc_msg->cb[0];</span><br><span style="color: hsl(0, 100%, 40%);">- *meta_storage = info ? *info : def_meta;</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday(&meta_storage->recv_time, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ meta_storage->expire_time = *expire_time;</span><br><span> </span><br><span> msgb_enqueue(&m_queue, llc_msg);</span><br><span> }</span><br><span>@@ -208,7 +209,7 @@</span><br><span> m_queue_octets -= msgb_length(msg);</span><br><span> </span><br><span> /* take the second time */</span><br><span style="color: hsl(0, 100%, 40%);">- gettimeofday(&tv_now, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday(&tv_now, NULL);</span><br><span> tv = (struct timeval *)&msg->data[sizeof(*tv)];</span><br><span> timersub(&tv_now, &meta_storage->recv_time, &tv_result);</span><br><span> </span><br><span>@@ -234,7 +235,7 @@</span><br><span> </span><br><span> /* calculate timestamp of timeout */</span><br><span> struct timeval now, csec;</span><br><span style="color: hsl(0, 100%, 40%);">- gettimeofday(&now, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday(&now, NULL);</span><br><span> csec.tv_usec = (delay_csec % 100) * 10000;</span><br><span> csec.tv_sec = delay_csec / 100;</span><br><span> </span><br><span>diff --git a/src/llc.h b/src/llc.h</span><br><span>index 2e7229c..8667e00 100644</span><br><span>--- a/src/llc.h</span><br><span>+++ b/src/llc.h</span><br><span>@@ -75,7 +75,7 @@</span><br><span> </span><br><span> void init();</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- void enqueue(struct msgb *llc_msg, const MetaInfo *info = 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ void enqueue(struct msgb *llc_msg, const struct timeval *expire_time);</span><br><span> struct msgb *dequeue(const MetaInfo **info = 0);</span><br><span> void clear(BTS *bts);</span><br><span> void move_and_merge(gprs_llc_queue *o);</span><br><span>diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp</span><br><span>index 7113d65..5c0fd9d 100644</span><br><span>--- a/src/tbf_dl.cpp</span><br><span>+++ b/src/tbf_dl.cpp</span><br><span>@@ -108,16 +108,17 @@</span><br><span> const uint16_t pdu_delay_csec,</span><br><span> const uint8_t *data, const uint16_t len)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct timeval expire_time;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> LOGPTBFDL(this, LOGL_DEBUG, "appending %u bytes\n", len);</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::MetaInfo info;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> struct msgb *llc_msg = msgb_alloc(len, "llc_pdu_queue");</span><br><span> if (!llc_msg)</span><br><span> return -ENOMEM;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::calc_pdu_lifetime(bts, pdu_delay_csec, &info.expire_time);</span><br><span style="color: hsl(0, 100%, 40%);">- gettimeofday(&info.recv_time, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ gprs_llc_queue::calc_pdu_lifetime(bts, pdu_delay_csec, &expire_time);</span><br><span> memcpy(msgb_put(llc_msg, len), data, len);</span><br><span style="color: hsl(0, 100%, 40%);">- llc_queue()->enqueue(llc_msg, &info);</span><br><span style="color: hsl(120, 100%, 40%);">+ llc_queue()->enqueue(llc_msg, &expire_time);</span><br><span> tbf_update_ms_class(this, ms_class);</span><br><span> start_llc_timer();</span><br><span> </span><br><span>diff --git a/tests/llc/LlcTest.cpp b/tests/llc/LlcTest.cpp</span><br><span>index 8163f34..d3bd871 100644</span><br><span>--- a/tests/llc/LlcTest.cpp</span><br><span>+++ b/tests/llc/LlcTest.cpp</span><br><span>@@ -34,6 +34,7 @@</span><br><span> #include <osmocom/core/msgb.h></span><br><span> #include <osmocom/core/talloc.h></span><br><span> #include <osmocom/core/utils.h></span><br><span style="color: hsl(120, 100%, 40%);">+#include <osmocom/core/timer.h></span><br><span> #include <osmocom/vty/vty.h></span><br><span> }</span><br><span> </span><br><span>@@ -43,7 +44,7 @@</span><br><span> bool spoof_mnc_3_digits = false;</span><br><span> </span><br><span> static void enqueue_data(gprs_llc_queue *queue, const uint8_t *data, size_t len,</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::MetaInfo *info = 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ const struct timeval *expire_time)</span><br><span> {</span><br><span> struct timeval *tv;</span><br><span> uint8_t *msg_data;</span><br><span>@@ -54,11 +55,11 @@</span><br><span> </span><br><span> memcpy(msg_data, data, len);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- queue->enqueue(llc_msg, info);</span><br><span style="color: hsl(120, 100%, 40%);">+ queue->enqueue(llc_msg, expire_time);</span><br><span> }</span><br><span> </span><br><span> static void dequeue_and_check(gprs_llc_queue *queue, const uint8_t *exp_data,</span><br><span style="color: hsl(0, 100%, 40%);">- size_t len, const gprs_llc_queue::MetaInfo *exp_info = 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ size_t len, const gprs_llc_queue::MetaInfo *exp_info)</span><br><span> {</span><br><span> struct msgb *llc_msg;</span><br><span> const gprs_llc_queue::MetaInfo *info_res;</span><br><span>@@ -73,15 +74,16 @@</span><br><span> fprintf(stderr, "check failed!\n");</span><br><span> </span><br><span> if (exp_info)</span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_ASSERT(memcmp(exp_info, info_res, sizeof(*exp_info)) == 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(memcmp(&exp_info->recv_time, &info_res->recv_time, sizeof(info_res->recv_time)) == 0 &&</span><br><span style="color: hsl(120, 100%, 40%);">+ memcmp(&exp_info->expire_time, &info_res->expire_time, sizeof(info_res->expire_time)) == 0);</span><br><span> </span><br><span> msgb_free(llc_msg);</span><br><span> }</span><br><span> </span><br><span> static void enqueue_data(gprs_llc_queue *queue, const char *message,</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::MetaInfo *info = 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ const struct timeval *expire_time)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(queue, (uint8_t *)(message), strlen(message), info);</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(queue, (uint8_t *)(message), strlen(message), expire_time);</span><br><span> }</span><br><span> </span><br><span> static void dequeue_and_check(gprs_llc_queue *queue, const char *exp_message,</span><br><span>@@ -94,6 +96,7 @@</span><br><span> static void test_llc_queue()</span><br><span> {</span><br><span> gprs_llc_queue queue;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct timeval expire_time = {0};</span><br><span> </span><br><span> printf("=== start %s ===\n", __func__);</span><br><span> </span><br><span>@@ -101,11 +104,11 @@</span><br><span> OSMO_ASSERT(queue.size() == 0);</span><br><span> OSMO_ASSERT(queue.octets() == 0);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue, "LLC message");</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue, "LLC message", &expire_time);</span><br><span> OSMO_ASSERT(queue.size() == 1);</span><br><span> OSMO_ASSERT(queue.octets() == 11);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue, "other LLC message");</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue, "other LLC message", &expire_time);</span><br><span> OSMO_ASSERT(queue.size() == 2);</span><br><span> OSMO_ASSERT(queue.octets() == 28);</span><br><span> </span><br><span>@@ -117,7 +120,7 @@</span><br><span> OSMO_ASSERT(queue.size() == 0);</span><br><span> OSMO_ASSERT(queue.octets() == 0);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue, "LLC");</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue, "LLC", &expire_time);</span><br><span> OSMO_ASSERT(queue.size() == 1);</span><br><span> OSMO_ASSERT(queue.octets() == 3);</span><br><span> </span><br><span>@@ -131,18 +134,8 @@</span><br><span> static void test_llc_meta()</span><br><span> {</span><br><span> gprs_llc_queue queue;</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::MetaInfo info1;</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::MetaInfo info2;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- info1.recv_time.tv_sec = 123456777;</span><br><span style="color: hsl(0, 100%, 40%);">- info1.recv_time.tv_usec = 123456;</span><br><span style="color: hsl(0, 100%, 40%);">- info1.expire_time.tv_sec = 123456789;</span><br><span style="color: hsl(0, 100%, 40%);">- info1.expire_time.tv_usec = 987654;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- info2.recv_time.tv_sec = 987654321;</span><br><span style="color: hsl(0, 100%, 40%);">- info2.recv_time.tv_usec = 547352;</span><br><span style="color: hsl(0, 100%, 40%);">- info2.expire_time.tv_sec = 987654327;</span><br><span style="color: hsl(0, 100%, 40%);">- info2.expire_time.tv_usec = 867252;</span><br><span style="color: hsl(120, 100%, 40%);">+ gprs_llc_queue::MetaInfo info1 = {0};</span><br><span style="color: hsl(120, 100%, 40%);">+ gprs_llc_queue::MetaInfo info2 = {0};</span><br><span> </span><br><span> printf("=== start %s ===\n", __func__);</span><br><span> </span><br><span>@@ -150,8 +143,19 @@</span><br><span> OSMO_ASSERT(queue.size() == 0);</span><br><span> OSMO_ASSERT(queue.octets() == 0);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue, "LLC message 1", &info1);</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue, "LLC message 2", &info2);</span><br><span style="color: hsl(120, 100%, 40%);">+ info1.recv_time.tv_sec = 123456777;</span><br><span style="color: hsl(120, 100%, 40%);">+ info1.recv_time.tv_usec = 123456;</span><br><span style="color: hsl(120, 100%, 40%);">+ info1.expire_time.tv_sec = 123456789;</span><br><span style="color: hsl(120, 100%, 40%);">+ info1.expire_time.tv_usec = 987654;</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time = info1.recv_time;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue, "LLC message 1", &info1.expire_time);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ info2.recv_time.tv_sec = 987654321;</span><br><span style="color: hsl(120, 100%, 40%);">+ info2.recv_time.tv_usec = 547352;</span><br><span style="color: hsl(120, 100%, 40%);">+ info2.expire_time.tv_sec = 987654327;</span><br><span style="color: hsl(120, 100%, 40%);">+ info2.expire_time.tv_usec = 867252;</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time = info2.recv_time;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue, "LLC message 2", &info2.expire_time);</span><br><span> </span><br><span> dequeue_and_check(&queue, "LLC message 1", &info1);</span><br><span> dequeue_and_check(&queue, "LLC message 2", &info2);</span><br><span>@@ -167,27 +171,27 @@</span><br><span> {</span><br><span> gprs_llc_queue queue1;</span><br><span> gprs_llc_queue queue2;</span><br><span style="color: hsl(0, 100%, 40%);">- gprs_llc_queue::MetaInfo info = {0};</span><br><span style="color: hsl(120, 100%, 40%);">+ struct timeval expire_time = {0};</span><br><span> </span><br><span> printf("=== start %s ===\n", __func__);</span><br><span> </span><br><span> queue1.init();</span><br><span> queue2.init();</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- info.recv_time.tv_sec += 1;</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue1, "*A*", &info);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_sec += 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue1, "*A*", &expire_time);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- info.recv_time.tv_sec += 1;</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue1, "*B*", &info);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_sec += 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue1, "*B*", &expire_time);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- info.recv_time.tv_sec += 1;</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue2, "*C*", &info);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_sec += 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue2, "*C*", &expire_time);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- info.recv_time.tv_sec += 1;</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue1, "*D*", &info);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_sec += 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue1, "*D*", &expire_time);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- info.recv_time.tv_sec += 1;</span><br><span style="color: hsl(0, 100%, 40%);">- enqueue_data(&queue2, "*E*", &info);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_sec += 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ enqueue_data(&queue2, "*E*", &expire_time);</span><br><span> </span><br><span> OSMO_ASSERT(queue1.size() == 3);</span><br><span> OSMO_ASSERT(queue1.octets() == 9);</span><br><span>@@ -231,6 +235,10 @@</span><br><span> vty_init(&pcu_vty_info);</span><br><span> pcu_vty_init();</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override = true;</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_sec = 123456777;</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_gettimeofday_override_time.tv_usec = 123456;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> test_llc_queue();</span><br><span> test_llc_meta();</span><br><span> test_llc_merge();</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17282">change 17282</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-pcu/+/17282"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ief6b1464dc779ff22adc2b02da7a006cd772ebce </div>
<div style="display:none"> Gerrit-Change-Number: 17282 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>