From ca5b071859a4d0955ebfcb3ff663a2f81ddb63b3 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sat, 25 Jul 2026 18:05:19 +0200 Subject: [PATCH] Fix module logic, updated tests, add tests (#584) --- TactilityKernel/include/tactility/module.h | 4 +- TactilityKernel/source/module.cpp | 6 + .../TactilityKernel/Source/FileMutexTest.cpp | 122 ++++++++++++++++++ Tests/TactilityKernel/Source/ModuleTest.cpp | 94 ++++++++++++++ 4 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 Tests/TactilityKernel/Source/FileMutexTest.cpp diff --git a/TactilityKernel/include/tactility/module.h b/TactilityKernel/include/tactility/module.h index 5bd03c569..b91091cbb 100644 --- a/TactilityKernel/include/tactility/module.h +++ b/TactilityKernel/include/tactility/module.h @@ -124,7 +124,7 @@ error_t module_construct_add_start(struct Module* module); /** * @brief Tries to ensure the module is in a started state. - * Calls module_construct if needed, calls module_start if needed. + * Calls module_construct if needed, calls module_add if needed, calls module_start if needed. * @param module the module * @return ERROR_NONE if module is in a started state */ @@ -132,7 +132,7 @@ error_t module_ensure_started(struct Module* module); /** * @brief Tries to ensure the module is in a started state. - * Calls module_stop if needed, calls module_destruct if needed. + * Calls module_stop if needed, calls module_remove, calls module_destruct if needed. * @param module the module * @return ERROR_NONE if module is in a destructed state */ diff --git a/TactilityKernel/source/module.cpp b/TactilityKernel/source/module.cpp index cd0060531..3fb5f2dff 100644 --- a/TactilityKernel/source/module.cpp +++ b/TactilityKernel/source/module.cpp @@ -146,6 +146,9 @@ error_t module_ensure_started(Module* module) { if (result != ERROR_NONE) { return result; } } + error_t add_result = module_add(module); + if (add_result != ERROR_NONE && add_result != ERROR_INVALID_STATE) { return add_result; } + if (!module->internal->started) { error_t result = module_start(module); if (result != ERROR_NONE) { return result; } @@ -162,6 +165,9 @@ error_t module_ensure_destructed(Module* module) { if (result != ERROR_NONE) { return result; } } + result = module_remove(module); + if (result != ERROR_NONE) { return result; } + result = module_destruct(module); if (result != ERROR_NONE) { return result; } } diff --git a/Tests/TactilityKernel/Source/FileMutexTest.cpp b/Tests/TactilityKernel/Source/FileMutexTest.cpp new file mode 100644 index 000000000..8e92e009c --- /dev/null +++ b/Tests/TactilityKernel/Source/FileMutexTest.cpp @@ -0,0 +1,122 @@ +#include "doctest.h" +#include + +namespace { + +int lock_calls = 0; +int unlock_calls = 0; +int try_lock_calls = 0; +bool try_lock_result = true; +uint32_t try_lock_timeout_seen = 0; + +void mock_lock() { lock_calls++; } +void mock_unlock() { unlock_calls++; } +bool mock_try_lock(uint32_t timeout) { + try_lock_calls++; + try_lock_timeout_seen = timeout; + return try_lock_result; +} + +int lock_a_calls = 0; +int lock_b_calls = 0; +void mock_lock_a() { lock_a_calls++; } +void mock_lock_b() { lock_b_calls++; } + +void reset_mocks() { + lock_calls = 0; + unlock_calls = 0; + try_lock_calls = 0; + try_lock_result = true; + try_lock_timeout_seen = 0; + lock_a_calls = 0; + lock_b_calls = 0; +} + +} // namespace + +TEST_CASE("file_mutex_get with zero registrations returns a no-op mutex") { + FileMutex mutex; + file_mutex_get(&mutex, "/nowhere/file.txt"); + + CHECK_EQ(mutex.lock, nullptr); + CHECK_EQ(mutex.try_lock, nullptr); + CHECK_EQ(mutex.unlock, nullptr); + + // Calling through a no-op mutex must be safe, and try_lock must report success. + file_mutex_lock(&mutex); + CHECK_EQ(file_mutex_try_lock(&mutex, 123), true); + file_mutex_unlock(&mutex); +} + +TEST_CASE("file_mutex_register/get with a single registration") { + reset_mocks(); + FileMutex registered = { .lock = mock_lock, .try_lock = mock_try_lock, .unlock = mock_unlock }; + file_mutex_register(®istered, "/mock1"); + + FileMutex mutex; + + // Exact mount path match. + file_mutex_get(&mutex, "/mock1"); + CHECK_EQ(mutex.lock, mock_lock); + CHECK_EQ(mutex.try_lock, mock_try_lock); + CHECK_EQ(mutex.unlock, mock_unlock); + + // Descendant path match. + file_mutex_get(&mutex, "/mock1/nested/file.txt"); + CHECK_EQ(mutex.lock, mock_lock); + + // Unrelated path falls back to no-op. + FileMutex unrelated; + file_mutex_get(&unrelated, "/other/file.txt"); + CHECK_EQ(unrelated.lock, nullptr); + + // Prefix-but-not-descendant path (e.g. "/mock1x") must not match "/mock1". + FileMutex prefix_only; + file_mutex_get(&prefix_only, "/mock1x/file.txt"); + CHECK_EQ(prefix_only.lock, nullptr); + + // Exercise the resolved callbacks. + file_mutex_get(&mutex, "/mock1"); + file_mutex_lock(&mutex); + CHECK_EQ(lock_calls, 1); + CHECK_EQ(file_mutex_try_lock(&mutex, 42), true); + CHECK_EQ(try_lock_calls, 1); + CHECK_EQ(try_lock_timeout_seen, 42); + file_mutex_unlock(&mutex); + CHECK_EQ(unlock_calls, 1); + + // Re-registering the same path is a no-op: original callbacks remain in place. + FileMutex replacement = { .lock = nullptr, .try_lock = nullptr, .unlock = nullptr }; + file_mutex_register(&replacement, "/mock1"); + file_mutex_get(&mutex, "/mock1"); + CHECK_EQ(mutex.lock, mock_lock); +} + +TEST_CASE("file_mutex_register/get with two registrations resolves to the matching path") { + reset_mocks(); + + FileMutex mutex_a = { .lock = mock_lock_a, .try_lock = nullptr, .unlock = nullptr }; + FileMutex mutex_b = { .lock = mock_lock_b, .try_lock = nullptr, .unlock = nullptr }; + + file_mutex_register(&mutex_a, "/mock2a"); + file_mutex_register(&mutex_b, "/mock2b"); + + FileMutex resolved; + + file_mutex_get(&resolved, "/mock2a/file.txt"); + CHECK_EQ(resolved.lock, mock_lock_a); + + file_mutex_get(&resolved, "/mock2b/file.txt"); + CHECK_EQ(resolved.lock, mock_lock_b); + + // Path matching neither registration falls back to no-op. + file_mutex_get(&resolved, "/mock2c/file.txt"); + CHECK_EQ(resolved.lock, nullptr); + + // Registration order matters: the first matching entry wins, not the longest + // prefix. A mount nested under an earlier one is shadowed by it. + FileMutex mutex_nested = { .lock = nullptr, .try_lock = nullptr, .unlock = nullptr }; + file_mutex_register(&mutex_nested, "/mock2a/nested"); + file_mutex_get(&resolved, "/mock2a/nested/file.txt"); + CHECK_EQ(resolved.lock, mock_lock_a); // still /mock2a, registered first +} diff --git a/Tests/TactilityKernel/Source/ModuleTest.cpp b/Tests/TactilityKernel/Source/ModuleTest.cpp index 24d60fb72..d99c32bd9 100644 --- a/Tests/TactilityKernel/Source/ModuleTest.cpp +++ b/Tests/TactilityKernel/Source/ModuleTest.cpp @@ -5,8 +5,14 @@ static void symbol_test_function() { /* NO-OP */ } static error_t test_start_result = ERROR_NONE; static bool start_called = false; +static struct Module* start_add_order_check_module = nullptr; static error_t test_start() { start_called = true; + if (start_add_order_check_module != nullptr) { + // If the module was already added to the ledger before start() runs, + // a duplicate module_add() must report that it already exists. + CHECK_EQ(module_add(start_add_order_check_module), ERROR_INVALID_STATE); + } return test_start_result; } @@ -139,3 +145,91 @@ TEST_CASE("Global symbol resolution") { CHECK_EQ(module_destruct(&module), ERROR_NONE); } + +TEST_CASE("module_ensure_started adds module to global ledger") { + start_called = false; + stop_called = false; + test_start_result = ERROR_NONE; + test_stop_result = ERROR_NONE; + + static const struct ModuleSymbol test_symbols[] = { + DEFINE_MODULE_SYMBOL(symbol_test_function), + MODULE_SYMBOL_TERMINATOR + }; + + struct Module module = { + .name = "test_ensure_started", + .start = test_start, + .stop = test_stop, + .symbols = test_symbols, + .internal = nullptr + }; + + uintptr_t addr; + // Not resolvable before module_ensure_started is called + CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), false); + + // test_start() asserts module_add(&module) is already ERROR_INVALID_STATE by the + // time start() runs, proving module_add happens before module_start (not after). + start_add_order_check_module = &module; + CHECK_EQ(module_ensure_started(&module), ERROR_NONE); + start_add_order_check_module = nullptr; + CHECK_EQ(module_is_started(&module), true); + CHECK_EQ(start_called, true); + + // Module must be both added to the ledger and started to be resolvable + CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true); + + // Calling again should be idempotent: no duplicate start, still resolvable + start_called = false; + CHECK_EQ(module_ensure_started(&module), ERROR_NONE); + CHECK_EQ(start_called, false); + CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true); + + // Cleanup + CHECK_EQ(module_stop(&module), ERROR_NONE); + CHECK_EQ(module_remove(&module), ERROR_NONE); + CHECK_EQ(module_destruct(&module), ERROR_NONE); +} + +TEST_CASE("module_ensure_destructed removes module from global ledger") { + start_called = false; + stop_called = false; + test_start_result = ERROR_NONE; + test_stop_result = ERROR_NONE; + + static const struct ModuleSymbol test_symbols[] = { + DEFINE_MODULE_SYMBOL(symbol_test_function), + MODULE_SYMBOL_TERMINATOR + }; + + struct Module module = { + .name = "test_ensure_destructed", + .start = test_start, + .stop = test_stop, + .symbols = test_symbols, + .internal = nullptr + }; + + CHECK_EQ(module_ensure_started(&module), ERROR_NONE); + + uintptr_t addr; + CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true); + + CHECK_EQ(module_ensure_destructed(&module), ERROR_NONE); + CHECK_EQ(module_is_started(&module), false); + CHECK_EQ(stop_called, true); + + // Module must no longer be resolvable once destructed. Note: this alone doesn't + // prove removal from the ledger, since module_resolve_symbol_global() also skips + // non-started modules — a leaked-but-stopped ledger entry would look the same. + CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), false); + + // Directly prove detachment from the ledger: if module_ensure_destructed had left + // the module in place, this module_add() would return ERROR_INVALID_STATE. + CHECK_EQ(module_add(&module), ERROR_NONE); + CHECK_EQ(module_remove(&module), ERROR_NONE); + + // Calling again on an already-destructed module should be a no-op + CHECK_EQ(module_ensure_destructed(&module), ERROR_NONE); +}