From bd5a1dc84f0124d99a89ac187f15c7d34beea210 Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Mon, 28 Jan 2019 15:38:09 +0100 Subject: 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 --- src/fsm.c | 21 ++++++++++++++------- tests/fsm/fsm_test.c | 38 ++++++++++++++++++++++++++++++++++++++ tests/fsm/fsm_test.err | 15 +++++++++++++++ tests/fsm/fsm_test.ok | 4 ++++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/src/fsm.c b/src/fsm.c index 1f6141fa..ae7c0f53 100644 --- a/src/fsm.c +++ b/src/fsm.c @@ -458,9 +458,10 @@ static int state_chg(struct osmo_fsm_inst *fi, uint32_t new_state, 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 @@ static int state_chg(struct osmo_fsm_inst *fi, uint32_t new_state, * 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 34a83993..7aac8d3e 100644 --- a/tests/fsm/fsm_test.c +++ b/tests/fsm/fsm_test.c @@ -349,6 +349,43 @@ static void test_state_chg_keep_timer() 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 @@ int main(int argc, char **argv) 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 85606e2a..bf474aba 100644 --- a/tests/fsm/fsm_test.err +++ b/tests/fsm/fsm_test.err @@ -101,3 +101,18 @@ Test_FSM{TWO}: Timeout of T10 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 e69de29b..c3536fba 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 -- cgit v1.2.3