Neels Hofmeyr has submitted this change and it was merged.

Change subject: fsm_test: more thoroughly test FSM inst ids and names
......................................................................


fsm_test: more thoroughly test FSM inst ids and names

Place id and name testing in its separate section, test_id_api().

Add a test that actually allocates an FSM instance with a NULL id, which is
allowed, but uncovers a bug of an unset FSM instance name. osmo_fsm_inst_name()
falls back to the fsm struct's name on NULL, but osmo_fsm_inst_find_by_name()
fails to match if the instance's name is NULL (and until recently even
crashed). Show this in fsm_test.c with loud comments.

Add test to clear the id by passing NULL.

Add test for setting an empty id.

Add test for setting an invalid identifier (osmo_identifier_valid() == false).

Change-Id: I646ed918576ce196c395dc5f42a1507c52ace2c5
---
M tests/fsm/fsm_test.c
M tests/fsm/fsm_test.err
2 files changed, 138 insertions(+), 7 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved



diff --git a/tests/fsm/fsm_test.c b/tests/fsm/fsm_test.c
index ef7bfe3..0f29a2e 100644
--- a/tests/fsm/fsm_test.c
+++ b/tests/fsm/fsm_test.c
@@ -3,6 +3,7 @@
 #include <stdarg.h>
 #include <unistd.h>
 #include <string.h>
+#include <errno.h>
 
 #include <osmocom/core/utils.h>
 #include <osmocom/core/select.h>
@@ -16,6 +17,12 @@
 
 static void *g_ctx;
 
+static int safe_strcmp(const char *a, const char *b)
+{
+       if (!a || !b)
+               return a == b ? 0 : 1;
+       return strcmp(a, b);
+}
 
 enum test_fsm_states {
        ST_NULL = 0,
@@ -118,7 +125,7 @@
        struct ctrl_cmd *cmd;
 
        cmd = exec_ctrl_cmd(cmdstr);
-       if (strcmp(cmd->reply, expres)) {
+       if (safe_strcmp(cmd->reply, expres)) {
                fprintf(stderr, "Reply '%s' doesn't match expected '%s'\n", 
cmd->reply, expres);
                OSMO_ASSERT(0);
        }
@@ -165,6 +172,89 @@
        return fi;
 }
 
+static void test_id_api()
+{
+       struct osmo_fsm_inst *fi;
+
+       fprintf(stderr, "\n--- %s()\n", __func__);
+
+/* Assert the instance has this name and can be looked up by it */
+#define assert_name(expected_name) \
+do { \
+       const char *name = osmo_fsm_inst_name(fi); \
+       fprintf(stderr, "  osmo_fsm_inst_name() == %s\n", osmo_quote_str(name, 
-1)); \
+       if (safe_strcmp(name, expected_name)) { \
+               fprintf(stderr, "    ERROR: expected %s\n", 
osmo_quote_str(expected_name, -1)); \
+               OSMO_ASSERT(false); \
+       } \
+       OSMO_ASSERT(osmo_fsm_inst_find_by_name(&fsm, expected_name) == fi); \
+       fprintf(stderr, "  osmo_fsm_inst_find_by_name(%s) == fi\n", 
osmo_quote_str(expected_name, -1)); \
+} while(0)
+
+/* Assert the instance can be looked up by this id string */
+#define assert_id(expected_id) \
+do { \
+       OSMO_ASSERT(osmo_fsm_inst_find_by_id(&fsm, expected_id) == fi); \
+       fprintf(stderr, "  osmo_fsm_inst_find_by_id(%s) == fi\n", 
osmo_quote_str(expected_id, -1)); \
+} while(0)
+
+/* Update the id, assert the proper rc, and expect a resulting fsm inst name + 
lookup */
+#define test_id(new_id, expect_rc, expect_name_suffix) do { \
+               int rc; \
+               fprintf(stderr, "osmo_fsm_inst_update_id(%s)\n", 
osmo_quote_str(new_id, -1)); \
+               rc = osmo_fsm_inst_update_id(fi, new_id); \
+               fprintf(stderr, "    rc == %d", rc); \
+               if (rc == (expect_rc)) \
+                       fprintf(stderr, ", ok\n"); \
+               else { \
+                       fprintf(stderr, ", ERROR: expected rc == %d\n", 
expect_rc); \
+                       OSMO_ASSERT(rc == expect_rc); \
+               } \
+               assert_name("Test_FSM" expect_name_suffix); \
+       }while (0)
+
+/* Successfully set a new id, along with name and id lookup assertions */
+#define change_id(new_id) \
+               test_id(new_id, 0, "(" new_id ")"); \
+               assert_id(new_id)
+
+       /* allocate FSM instance without id, there should be a name without id 
*/
+       fi = osmo_fsm_inst_alloc(&fsm, g_ctx, NULL, LOGL_DEBUG, NULL);
+       OSMO_ASSERT(fi);
+       /* CURRENT BUG: here I want to just do
+       assert_name("Test_FSM");
+        * but when allocated with a NULL id, the fsm's name remains unset. 
Hence: */
+       {
+               const char *expected_name = "Test_FSM";
+               const char *name = osmo_fsm_inst_name(fi);
+               fprintf(stderr, "  osmo_fsm_inst_name() == %s\n", 
osmo_quote_str(name, -1));
+               if (safe_strcmp(name, expected_name)) {
+                       fprintf(stderr, "    ERROR: expected %s\n", 
osmo_quote_str(expected_name, -1));
+                       OSMO_ASSERT(false);
+               }
+               OSMO_ASSERT(osmo_fsm_inst_find_by_name(&fsm, "Test_FSM") == 
NULL); /* <- ERROR */
+               fprintf(stderr, "  osmo_fsm_inst_find_by_name(%s) == NULL\n", 
osmo_quote_str(expected_name, -1));
+       }
+
+       change_id("my_id");
+       change_id("another_id");
+
+       test_id(NULL, 0, "");
+       /* clear already cleared id */
+       test_id(NULL, 0, "");
+
+       change_id("arbitrary_id");
+
+       /* clear id by empty string doesn't work */
+       test_id("", -EINVAL, "(arbitrary_id)");
+
+       test_id("invalid.id", -EINVAL, "(arbitrary_id)");
+
+       fprintf(stderr, "\n--- %s() done\n\n", __func__);
+
+       osmo_fsm_inst_term(fi, OSMO_FSM_TERM_REQUEST, NULL);
+}
+
 static const struct log_info_cat default_categories[] = {
        [DMAIN] = {
                .name = "DMAIN",
@@ -198,17 +288,14 @@
 
        OSMO_ASSERT(osmo_fsm_inst_find_by_name(&fsm, "my_id") == NULL);
        finst = foo();
-       OSMO_ASSERT(osmo_fsm_inst_find_by_id(&fsm, "my_id") == finst);
-       OSMO_ASSERT(osmo_fsm_inst_find_by_name(&fsm, "Test_FSM(my_id)") == 
finst);
-       OSMO_ASSERT(osmo_fsm_inst_update_id(finst, "another_id") == 0);
-       OSMO_ASSERT(osmo_fsm_inst_find_by_id(&fsm, "another_id") == finst);
-       OSMO_ASSERT(osmo_fsm_inst_find_by_name(&fsm, "Test_FSM(another_id)") == 
finst);
-       OSMO_ASSERT(osmo_fsm_inst_update_id(finst, "my_id") == 0);
 
        while (main_loop_run) {
                osmo_select_main(0);
        }
        osmo_fsm_inst_free(finst);
+
+       test_id_api();
+
        osmo_fsm_unregister(&fsm);
        exit(0);
 }
diff --git a/tests/fsm/fsm_test.err b/tests/fsm/fsm_test.err
index facc9f7..bc159bd 100644
--- a/tests/fsm/fsm_test.err
+++ b/tests/fsm/fsm_test.err
@@ -9,4 +9,48 @@
 Test_FSM(my_id){TWO}: Timeout of T2342
 Timer
 Test_FSM(my_id){TWO}: Deallocated
+
+--- test_id_api()
+Test_FSM{NULL}: Allocated
+  osmo_fsm_inst_name() == "Test_FSM"
+  osmo_fsm_inst_find_by_name("Test_FSM") == NULL
+osmo_fsm_inst_update_id("my_id")
+    rc == 0, ok
+  osmo_fsm_inst_name() == "Test_FSM(my_id)"
+  osmo_fsm_inst_find_by_name("Test_FSM(my_id)") == fi
+  osmo_fsm_inst_find_by_id("my_id") == fi
+osmo_fsm_inst_update_id("another_id")
+    rc == 0, ok
+  osmo_fsm_inst_name() == "Test_FSM(another_id)"
+  osmo_fsm_inst_find_by_name("Test_FSM(another_id)") == fi
+  osmo_fsm_inst_find_by_id("another_id") == fi
+osmo_fsm_inst_update_id(NULL)
+    rc == 0, ok
+  osmo_fsm_inst_name() == "Test_FSM"
+  osmo_fsm_inst_find_by_name("Test_FSM") == fi
+osmo_fsm_inst_update_id(NULL)
+    rc == 0, ok
+  osmo_fsm_inst_name() == "Test_FSM"
+  osmo_fsm_inst_find_by_name("Test_FSM") == fi
+osmo_fsm_inst_update_id("arbitrary_id")
+    rc == 0, ok
+  osmo_fsm_inst_name() == "Test_FSM(arbitrary_id)"
+  osmo_fsm_inst_find_by_name("Test_FSM(arbitrary_id)") == fi
+  osmo_fsm_inst_find_by_id("arbitrary_id") == fi
+osmo_fsm_inst_update_id("")
+Attempting to allocate FSM instance of type 'Test_FSM' with illegal identifier 
''
+    rc == -22, ok
+  osmo_fsm_inst_name() == "Test_FSM(arbitrary_id)"
+  osmo_fsm_inst_find_by_name("Test_FSM(arbitrary_id)") == fi
+osmo_fsm_inst_update_id("invalid.id")
+Attempting to allocate FSM instance of type 'Test_FSM' with illegal identifier 
'invalid.id'
+    rc == -22, ok
+  osmo_fsm_inst_name() == "Test_FSM(arbitrary_id)"
+  osmo_fsm_inst_find_by_name("Test_FSM(arbitrary_id)") == fi
+
+--- test_id_api() done
+
+Test_FSM(arbitrary_id){NULL}: Terminating (cause = OSMO_FSM_TERM_REQUEST)
+Test_FSM(arbitrary_id){NULL}: Freeing instance
+Test_FSM(arbitrary_id){NULL}: Deallocated
 
\ No newline at end of file

-- 
To view, visit https://gerrit.osmocom.org/7681
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I646ed918576ce196c395dc5f42a1507c52ace2c5
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder

Reply via email to