Change in libosmocore[master]: osmo_fsm_inst_state_chg(): set T also for zero timeout

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/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Thu Jan 31 16:38:47 UTC 2019


Neels Hofmeyr has submitted this change and it was merged. ( https://gerrit.osmocom.org/12715 )

Change subject: osmo_fsm_inst_state_chg(): set T also for zero timeout
......................................................................

osmo_fsm_inst_state_chg(): set T also for zero timeout

Before this patch, if timeout_secs == 0 was passed to
osmo_fsm_inst_state_chg(), the previous T value remained set in the
osmo_fsm_inst->T.

For example:

  osmo_fsm_inst_state_chg(fi, ST_X, 23, 42);
  // timer == 23 seconds; fi->T == 42
  osmo_fsm_inst_state_chg(fi, ST_Y, 0, 0);
  // no timer; fi->T == 42!

Instead, always set to the T value passed to osmo_fsm_inst_state_chg().

Adjust osmo_fsm_inst_state_chg() API doc; need to rephrase to accurately
describe the otherwise unchanged behaviour independently from T.

Verify in fsm_test.c.

Rationale: it is confusing to have a T number remaining from some past state,
especially since the user explicitly passed a T number to
osmo_fsm_inst_state_chg(). (Usually we are passing timeout_secs=0, T=0).

I first thought this behavior was introduced with
osmo_fsm_inst_state_chg_keep_timer(), but in fact osmo_fsm_inst_state_chg()
behaved this way from the start.

This shows up in the C test for the upcoming tdef API, where the test result
printout was showing some past T value sticking around after FSM state
transitions. After this patch, there will be no such confusion.

Change-Id: I65c7c262674a1bc5f37faeca6aa0320ab0174f3c
---
M src/fsm.c
M tests/fsm/fsm_test.c
M tests/fsm/fsm_test.err
M tests/fsm/fsm_test.ok
4 files changed, 71 insertions(+), 7 deletions(-)

Approvals:
  Jenkins Builder: Verified
  Vadim Yanitskiy: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved



diff --git a/src/fsm.c b/src/fsm.c
index 1f6141f..ae7c0f5 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -458,9 +458,10 @@
 	fi->state = new_state;
 	st = &fsm->states[new_state];
 
-	if (!keep_timer && timeout_secs) {
+	if (!keep_timer) {
 		fi->T = T;
-		osmo_timer_schedule(&fi->timer, timeout_secs, 0);
+		if (timeout_secs)
+			osmo_timer_schedule(&fi->timer, timeout_secs, 0);
 	}
 
 	/* Call 'onenter' last, user might terminate FSM from there */
@@ -480,11 +481,17 @@
  *  function.  It verifies that the existing state actually permits a
  *  transition to new_state.
  *
- *  timeout_secs and T are optional parameters, and only have any effect
- *  if timeout_secs is not 0.  If the timeout function is used, then the
- *  new_state is entered, and the FSM instances timer is set to expire
- *  in timeout_secs functions.   At that time, the FSM's timer_cb
- *  function will be called for handling of the timeout by the user.
+ *  If timeout_secs is 0, stay in the new state indefinitely, without a timeout
+ *  (stop the FSM instance's timer if it was runnning).
+ *
+ *  If timeout_secs > 0, start or reset the FSM instance's timer with this
+ *  timeout. On expiry, invoke the FSM instance's timer_cb -- if no timer_cb is
+ *  set, an expired timer immediately terminates the FSM instance with
+ *  OSMO_FSM_TERM_TIMEOUT.
+ *
+ *  The value of T is stored in fi->T and is then available for query in
+ *  timer_cb. If passing timeout_secs == 0, it is recommended to also pass T ==
+ *  0, so that fi->T is reset to 0 when no timeout is invoked.
  *
  *  \param[in] fi FSM instance whose state is to change
  *  \param[in] new_state The new state into which we should change
diff --git a/tests/fsm/fsm_test.c b/tests/fsm/fsm_test.c
index 34a8399..7aac8d3 100644
--- a/tests/fsm/fsm_test.c
+++ b/tests/fsm/fsm_test.c
@@ -349,6 +349,43 @@
 	fprintf(stderr, "--- %s() done\n", __func__);
 }
 
+static void test_state_chg_T()
+{
+	struct osmo_fsm_inst *fi;
+
+	fprintf(stderr, "\n--- %s()\n", __func__);
+
+	fsm.timer_cb = NULL;
+
+	/* Test setting to timeout_secs = 0, T = 0 */
+	fi = osmo_fsm_inst_alloc(&fsm, g_ctx, NULL, LOGL_DEBUG, NULL);
+	OSMO_ASSERT(fi);
+
+	osmo_fsm_inst_state_chg(fi, ST_ONE, 23, 42);
+	printf("T = %d\n", fi->T);
+	OSMO_ASSERT(fi->T == 42);
+	osmo_fsm_inst_state_chg(fi, ST_TWO, 0, 0);
+	printf("T = %d\n", fi->T);
+	OSMO_ASSERT(fi->T == 0);
+
+	osmo_fsm_inst_term(fi, OSMO_FSM_TERM_REQUEST, NULL);
+
+	/* Test setting to timeout_secs = 0, T != 0 */
+	fi = osmo_fsm_inst_alloc(&fsm, g_ctx, NULL, LOGL_DEBUG, NULL);
+	OSMO_ASSERT(fi);
+
+	osmo_fsm_inst_state_chg(fi, ST_ONE, 23, 42);
+	printf("T = %d\n", fi->T);
+	OSMO_ASSERT(fi->T == 42);
+	osmo_fsm_inst_state_chg(fi, ST_TWO, 0, 11);
+	printf("T = %d\n", fi->T);
+	OSMO_ASSERT(fi->T == 11);
+
+	osmo_fsm_inst_term(fi, OSMO_FSM_TERM_REQUEST, NULL);
+
+	fprintf(stderr, "--- %s() done\n", __func__);
+}
+
 static const struct log_info_cat default_categories[] = {
 	[DMAIN] = {
 		.name = "DMAIN",
@@ -390,6 +427,7 @@
 
 	test_id_api();
 	test_state_chg_keep_timer();
+	test_state_chg_T();
 
 	osmo_fsm_unregister(&fsm);
 	exit(0);
diff --git a/tests/fsm/fsm_test.err b/tests/fsm/fsm_test.err
index 85606e2..bf474ab 100644
--- a/tests/fsm/fsm_test.err
+++ b/tests/fsm/fsm_test.err
@@ -101,3 +101,18 @@
 Test_FSM{TWO}: Freeing instance
 Test_FSM{TWO}: Deallocated
 --- test_state_chg_keep_timer() done
+
+--- test_state_chg_T()
+Test_FSM{NULL}: Allocated
+Test_FSM{NULL}: state_chg to ONE
+Test_FSM{ONE}: state_chg to TWO
+Test_FSM{TWO}: Terminating (cause = OSMO_FSM_TERM_REQUEST)
+Test_FSM{TWO}: Freeing instance
+Test_FSM{TWO}: Deallocated
+Test_FSM{NULL}: Allocated
+Test_FSM{NULL}: state_chg to ONE
+Test_FSM{ONE}: state_chg to TWO
+Test_FSM{TWO}: Terminating (cause = OSMO_FSM_TERM_REQUEST)
+Test_FSM{TWO}: Freeing instance
+Test_FSM{TWO}: Deallocated
+--- test_state_chg_T() done
diff --git a/tests/fsm/fsm_test.ok b/tests/fsm/fsm_test.ok
index e69de29..c3536fb 100644
--- a/tests/fsm/fsm_test.ok
+++ b/tests/fsm/fsm_test.ok
@@ -0,0 +1,4 @@
+T = 42
+T = 0
+T = 42
+T = 11

-- 
To view, visit https://gerrit.osmocom.org/12715
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I65c7c262674a1bc5f37faeca6aa0320ab0174f3c
Gerrit-Change-Number: 12715
Gerrit-PatchSet: 4
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190131/643c4853/attachment.htm>


More information about the gerrit-log mailing list