<p>Neels Hofmeyr <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/12715">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
Vadim Yanitskiy: Looks good to me, but someone else must approve
Harald Welte: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">osmo_fsm_inst_state_chg(): set T also for zero timeout<br><br>Before this patch, if timeout_secs == 0 was passed to<br>osmo_fsm_inst_state_chg(), the previous T value remained set in the<br>osmo_fsm_inst->T.<br><br>For example:<br><br> osmo_fsm_inst_state_chg(fi, ST_X, 23, 42);<br> // timer == 23 seconds; fi->T == 42<br> osmo_fsm_inst_state_chg(fi, ST_Y, 0, 0);<br> // no timer; fi->T == 42!<br><br>Instead, always set to the T value passed to osmo_fsm_inst_state_chg().<br><br>Adjust osmo_fsm_inst_state_chg() API doc; need to rephrase to accurately<br>describe the otherwise unchanged behaviour independently from T.<br><br>Verify in fsm_test.c.<br><br>Rationale: it is confusing to have a T number remaining from some past state,<br>especially since the user explicitly passed a T number to<br>osmo_fsm_inst_state_chg(). (Usually we are passing timeout_secs=0, T=0).<br><br>I first thought this behavior was introduced with<br>osmo_fsm_inst_state_chg_keep_timer(), but in fact osmo_fsm_inst_state_chg()<br>behaved this way from the start.<br><br>This shows up in the C test for the upcoming tdef API, where the test result<br>printout was showing some past T value sticking around after FSM state<br>transitions. After this patch, there will be no such confusion.<br><br>Change-Id: I65c7c262674a1bc5f37faeca6aa0320ab0174f3c<br>---<br>M src/fsm.c<br>M tests/fsm/fsm_test.c<br>M tests/fsm/fsm_test.err<br>M tests/fsm/fsm_test.ok<br>4 files changed, 71 insertions(+), 7 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/fsm.c b/src/fsm.c</span><br><span>index 1f6141f..ae7c0f5 100644</span><br><span>--- a/src/fsm.c</span><br><span>+++ b/src/fsm.c</span><br><span>@@ -458,9 +458,10 @@</span><br><span> fi->state = new_state;</span><br><span> st = &fsm->states[new_state];</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!keep_timer && timeout_secs) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!keep_timer) {</span><br><span> fi->T = T;</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_timer_schedule(&fi->timer, timeout_secs, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (timeout_secs)</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_timer_schedule(&fi->timer, timeout_secs, 0);</span><br><span> }</span><br><span> </span><br><span> /* Call 'onenter' last, user might terminate FSM from there */</span><br><span>@@ -480,11 +481,17 @@</span><br><span> * function. It verifies that the existing state actually permits a</span><br><span> * transition to new_state.</span><br><span> *</span><br><span style="color: hsl(0, 100%, 40%);">- * timeout_secs and T are optional parameters, and only have any effect</span><br><span style="color: hsl(0, 100%, 40%);">- * if timeout_secs is not 0. If the timeout function is used, then the</span><br><span style="color: hsl(0, 100%, 40%);">- * new_state is entered, and the FSM instances timer is set to expire</span><br><span style="color: hsl(0, 100%, 40%);">- * in timeout_secs functions. At that time, the FSM's timer_cb</span><br><span style="color: hsl(0, 100%, 40%);">- * function will be called for handling of the timeout by the user.</span><br><span style="color: hsl(120, 100%, 40%);">+ * If timeout_secs is 0, stay in the new state indefinitely, without a timeout</span><br><span style="color: hsl(120, 100%, 40%);">+ * (stop the FSM instance's timer if it was runnning).</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * If timeout_secs > 0, start or reset the FSM instance's timer with this</span><br><span style="color: hsl(120, 100%, 40%);">+ * timeout. On expiry, invoke the FSM instance's timer_cb -- if no timer_cb is</span><br><span style="color: hsl(120, 100%, 40%);">+ * set, an expired timer immediately terminates the FSM instance with</span><br><span style="color: hsl(120, 100%, 40%);">+ * OSMO_FSM_TERM_TIMEOUT.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * The value of T is stored in fi->T and is then available for query in</span><br><span style="color: hsl(120, 100%, 40%);">+ * timer_cb. If passing timeout_secs == 0, it is recommended to also pass T ==</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0, so that fi->T is reset to 0 when no timeout is invoked.</span><br><span> *</span><br><span> * \param[in] fi FSM instance whose state is to change</span><br><span> * \param[in] new_state The new state into which we should change</span><br><span>diff --git a/tests/fsm/fsm_test.c b/tests/fsm/fsm_test.c</span><br><span>index 34a8399..7aac8d3 100644</span><br><span>--- a/tests/fsm/fsm_test.c</span><br><span>+++ b/tests/fsm/fsm_test.c</span><br><span>@@ -349,6 +349,43 @@</span><br><span> fprintf(stderr, "--- %s() done\n", __func__);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static void test_state_chg_T()</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_fsm_inst *fi;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ fprintf(stderr, "\n--- %s()\n", __func__);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ fsm.timer_cb = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Test setting to timeout_secs = 0, T = 0 */</span><br><span style="color: hsl(120, 100%, 40%);">+ fi = osmo_fsm_inst_alloc(&fsm, g_ctx, NULL, LOGL_DEBUG, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(fi);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_state_chg(fi, ST_ONE, 23, 42);</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("T = %d\n", fi->T);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(fi->T == 42);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_state_chg(fi, ST_TWO, 0, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("T = %d\n", fi->T);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(fi->T == 0);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_term(fi, OSMO_FSM_TERM_REQUEST, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Test setting to timeout_secs = 0, T != 0 */</span><br><span style="color: hsl(120, 100%, 40%);">+ fi = osmo_fsm_inst_alloc(&fsm, g_ctx, NULL, LOGL_DEBUG, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(fi);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_state_chg(fi, ST_ONE, 23, 42);</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("T = %d\n", fi->T);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(fi->T == 42);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_state_chg(fi, ST_TWO, 0, 11);</span><br><span style="color: hsl(120, 100%, 40%);">+ printf("T = %d\n", fi->T);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(fi->T == 11);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_term(fi, OSMO_FSM_TERM_REQUEST, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ fprintf(stderr, "--- %s() done\n", __func__);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static const struct log_info_cat default_categories[] = {</span><br><span> [DMAIN] = {</span><br><span> .name = "DMAIN",</span><br><span>@@ -390,6 +427,7 @@</span><br><span> </span><br><span> test_id_api();</span><br><span> test_state_chg_keep_timer();</span><br><span style="color: hsl(120, 100%, 40%);">+ test_state_chg_T();</span><br><span> </span><br><span> osmo_fsm_unregister(&fsm);</span><br><span> exit(0);</span><br><span>diff --git a/tests/fsm/fsm_test.err b/tests/fsm/fsm_test.err</span><br><span>index 85606e2..bf474ab 100644</span><br><span>--- a/tests/fsm/fsm_test.err</span><br><span>+++ b/tests/fsm/fsm_test.err</span><br><span>@@ -101,3 +101,18 @@</span><br><span> [0;mTest_FSM{TWO}: Freeing instance</span><br><span> [0;mTest_FSM{TWO}: Deallocated</span><br><span> [0;m--- test_state_chg_keep_timer() done</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+--- test_state_chg_T()</span><br><span style="color: hsl(120, 100%, 40%);">+Test_FSM{NULL}: Allocated</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{NULL}: state_chg to ONE</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{ONE}: state_chg to TWO</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{TWO}: Terminating (cause = OSMO_FSM_TERM_REQUEST)</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{TWO}: Freeing instance</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{TWO}: Deallocated</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{NULL}: Allocated</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{NULL}: state_chg to ONE</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{ONE}: state_chg to TWO</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{TWO}: Terminating (cause = OSMO_FSM_TERM_REQUEST)</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{TWO}: Freeing instance</span><br><span style="color: hsl(120, 100%, 40%);">+[0;mTest_FSM{TWO}: Deallocated</span><br><span style="color: hsl(120, 100%, 40%);">+[0;m--- test_state_chg_T() done</span><br><span>diff --git a/tests/fsm/fsm_test.ok b/tests/fsm/fsm_test.ok</span><br><span>index e69de29..c3536fb 100644</span><br><span>--- a/tests/fsm/fsm_test.ok</span><br><span>+++ b/tests/fsm/fsm_test.ok</span><br><span>@@ -0,0 +1,4 @@</span><br><span style="color: hsl(120, 100%, 40%);">+T = 42</span><br><span style="color: hsl(120, 100%, 40%);">+T = 0</span><br><span style="color: hsl(120, 100%, 40%);">+T = 42</span><br><span style="color: hsl(120, 100%, 40%);">+T = 11</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/12715">change 12715</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/12715"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I65c7c262674a1bc5f37faeca6aa0320ab0174f3c </div>
<div style="display:none"> Gerrit-Change-Number: 12715 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>