From dd4058016522cb198305869f2e19885f2087bd19 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Fri, 4 Sep 2026 00:37:30 +0200 Subject: [PATCH] Cleanup --- Modules/app-module/include/app/execute.h | 2 +- .../private/app/private/arguments.h | 37 +++++++++ .../app-module/private/app/private/ledger.h | 14 ---- Modules/app-module/source/manager.cpp | 83 ++++++------------- Modules/app-module/source/scheduler.cpp | 21 ++--- 5 files changed, 73 insertions(+), 84 deletions(-) create mode 100644 Modules/app-module/private/app/private/arguments.h diff --git a/Modules/app-module/include/app/execute.h b/Modules/app-module/include/app/execute.h index f876e2442..c51787e7a 100644 --- a/Modules/app-module/include/app/execute.h +++ b/Modules/app-module/include/app/execute.h @@ -83,7 +83,7 @@ error_t app_execute_for_result_with_streams( struct AppStackConfig stack, int argc, const char* const argv[], - const struct AppStreamBinding* binding, + const struct AppStreamBinding* bindings, size_t binding_count, AppInstanceId parent_instance_id, AppInstanceId* out_app_instance_id diff --git a/Modules/app-module/private/app/private/arguments.h b/Modules/app-module/private/app/private/arguments.h new file mode 100644 index 000000000..4200f6a2e --- /dev/null +++ b/Modules/app-module/private/app/private/arguments.h @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: Apache-2.0 +#pragma once + +#include + +/** + * Deep-copies @a argv (@a argc <= 0 => NULL, matching "no parameters"). Caller passes the result + * to app_scheduler_start(), which takes ownership regardless of outcome. + */ +inline char** app_arguments_copy(int argc, const char* const argv[]) { + if (argc <= 0) { + return nullptr; + } + auto* copy = new char*[argc + 1]; + for (int i = 0; i < argc; i++) { + size_t length = strlen(argv[i]); + copy[i] = new char[length + 1]; + memcpy(copy[i], argv[i], length + 1); + } + copy[argc] = nullptr; + return copy; +} + +/** + * Frees a deep-copied argv previously built by app_arguments_copy(): each individually + * heap-allocated string, then the array itself. Safe to call with count == 0 / values == nullptr + * (no-op). + */ +inline void app_arguments_free(int count, char** values) { + if (values == nullptr) { + return; + } + for (int i = 0; i < count; i++) { + delete[] values[i]; + } + delete[] values; +} diff --git a/Modules/app-module/private/app/private/ledger.h b/Modules/app-module/private/app/private/ledger.h index 85e770027..eab9b3dcf 100644 --- a/Modules/app-module/private/app/private/ledger.h +++ b/Modules/app-module/private/app/private/ledger.h @@ -71,17 +71,3 @@ inline AppLedger& app_ledger() { static AppLedger ledger; return ledger; } - -/** - * Frees a deep-copied argv previously built by app_start()/app_start_for_result(): - * each individually heap-allocated string, then the array itself. Safe to call with count == 0 values == nullptr (no-op). - */ -inline void app_ledger_free_arguments(int count, char** values) { - if (values == nullptr) { - return; - } - for (int i = 0; i < count; i++) { - delete[] values[i]; - } - delete[] values; -} diff --git a/Modules/app-module/source/manager.cpp b/Modules/app-module/source/manager.cpp index 6b3d2c46e..d905cf5f7 100644 --- a/Modules/app-module/source/manager.cpp +++ b/Modules/app-module/source/manager.cpp @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 #include #include +#include #include #include #include @@ -71,33 +72,11 @@ void app_manager_for_each_manifest(AppManifestVisitorFn visitor, void* context) mutex_unlock(&ledger.mutex); } -namespace { - -// Deep-copies argv (argc <= 0 => NULL, matching "no parameters"). Caller passes the result to -// app_scheduler_start(), which takes ownership regardless of outcome. -char** copy_arguments(int argc, const char* const argv[]) { - if (argc <= 0) { - return nullptr; - } - auto* copy = new char*[argc + 1]; - for (int i = 0; i < argc; i++) { - size_t length = strlen(argv[i]); - copy[i] = new char[length + 1]; - memcpy(copy[i], argv[i], length + 1); - } - copy[argc] = nullptr; - return copy; -} - -} // namespace - error_t app_manager_start_internal(const AppManifest* manifest, AppLocation location, AppStackConfig stack, AppInstanceId parent_instance_id, int argc, const char* const argv_in[], const AppStreamBinding* bindings, size_t binding_count, AppInstanceId* out_app_instance_id) { - // Owns its own deep copy from here on: app_scheduler_start() takes ownership of it regardless - // of outcome, and the spawned task frees it once its run() returns. - char** argv = copy_arguments(argc, argv_in); + char** argv = app_arguments_copy(argc, argv_in); if (binding_count != 0 && bindings == nullptr) { - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_INVALID_ARGUMENT; } @@ -108,9 +87,8 @@ error_t app_manager_start_internal(const AppManifest* manifest, AppLocation loca AppInstanceRecord record { .id = target_id, .manifest = manifest, .state = APP_INSTANCE_STATE_STARTING, .task = nullptr }; record.parent_id = parent_instance_id; ledger.instances[target_id] = record; - // Constructed on the map-resident copy, not the local `record` about to go out of scope. - // AppFdTable::fds[] entries point into AppFdTable::slots[] by address (see fd_table.h), so - // constructing before the copy above would leave them pointing at stack storage. + // Construct on the map-resident copy, not `record`: fds[] point into slots[] by address + // (fd_table.h), so constructing on the stack-local record would leave them dangling. app_fd_table_construct(&ledger.instances[target_id].fd_table); mutex_unlock(&ledger.mutex); @@ -120,9 +98,8 @@ error_t app_manager_start_internal(const AppManifest* manifest, AppLocation loca error_t bind_result = app_stream_subscribe(bindings[i].stream, bindings[i].buffer, bindings[i].buffer_capacity, bindings[i].event_group, target_id, bindings[i].producer_fd); if (bind_result != ERROR_NONE) { LOG_E(TAG, "[instance %d] Failed to bind stream at fd %d: %s", target_id, bindings[i].producer_fd, error_to_string(bind_result)); - // Undo bindings[0..i): app_fd_table_teardown() below only closes each stream. It - // doesn't release the event bits app_stream_subscribe() claimed or destruct - // stream->internal.mutex; only app_stream_unsubscribe() does that. + // Undo bindings[0..i): teardown() below only closes the fd, not the event bits + // or mutex app_stream_subscribe() claimed; only app_stream_unsubscribe() does. for (size_t j = 0; j < i; j++) { app_stream_unsubscribe(bindings[j].stream); } @@ -130,15 +107,13 @@ error_t app_manager_start_internal(const AppManifest* manifest, AppLocation loca app_fd_table_teardown(&ledger.instances[target_id].fd_table); ledger.instances.erase(target_id); mutex_unlock(&ledger.mutex); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return bind_result; } } error_t error = app_scheduler_start(target_id, location, stack, argc, argv); if (error != ERROR_NONE) { - // Every binding succeeded before app_scheduler_start() failed. Unsubscribe all of them, - // same reasoning as the bind-failure path above. for (size_t j = 0; j < binding_count; j++) { app_stream_unsubscribe(bindings[j].stream); } @@ -172,8 +147,7 @@ error_t app_manager_get_topmost_instance_id(AppInstanceId* out_app_instance_id) mutex_lock(&ledger.mutex); AppInstanceId topmost_id = 0; for (auto& [instance_id, record] : ledger.instances) { - // Instance ids are handed out in increasing order (AppLedger::next_instance_id), so - // the highest Active id is also the most recently started one. + // Ids increase monotonically, so the highest Active id is the most recent. if (record.state == APP_INSTANCE_STATE_ACTIVE && instance_id > topmost_id) { topmost_id = instance_id; } @@ -224,10 +198,8 @@ error_t app_manager_get_topmost_app_id(char* buffer, size_t buffer_size) { namespace { // Owns the AppManifest (and its id/name/path strings) that app_manager_add() only keeps a -// non-owning pointer to (see app_manager_add()'s contract), for manifests registered by -// app_manager_install_path_scan() specifically - separate from app_install.cpp's own registry, -// since scanning only ever adds/removes manifest registrations and never touches files on disk -// or running instances (unlike app_install()/app_uninstall()). +// non-owning pointer to. Separate from app_install.cpp's registry: scanning only +// adds/removes registrations, never touches disk or running instances. struct ScannedAppManifest { std::string id; std::string name; @@ -274,15 +246,15 @@ void app_manager_install_path_scan(void) { app_fs_list_direct_subdirectories(root, found_app_dirs); } - // Snapshot of what's already registered, taken once so the rest of this scan can run without holding registry.mutex + // Snapshot once so the rest of the scan doesn't hold registry.mutex. mutex_lock(®istry.mutex); - std::unordered_map known_paths; // id -> path + std::unordered_map known_paths; for (const auto& [id, record] : registry.scanned) { known_paths.emplace(id, record->path); } mutex_unlock(®istry.mutex); - // Stat each manifest and parse it entirely without registry.mutex held (due to filesystem IO being slow) + // Parses without registry.mutex held; filesystem IO is slow. std::vector> new_records; for (const auto& app_dir : found_app_dirs) { auto manifest_path = app_dir + "/manifest.properties"; @@ -297,7 +269,7 @@ void app_manager_install_path_scan(void) { } if (known_paths.contains(metadata.app_id)) { - continue; // already registered by an earlier scan + continue; } auto record = std::make_unique(); @@ -315,7 +287,6 @@ void app_manager_install_path_scan(void) { new_records.push_back(std::move(record)); } - // Anything a previous scan registered whose directory has since disappeared gets unregistered below. std::vector missing_ids; for (const auto& [id, path] : known_paths) { if (!app_fs_is_directory(path)) { @@ -323,11 +294,9 @@ void app_manager_install_path_scan(void) { } } - // app_manager_add()/app_manager_remove() take app-module's own ledger mutex internally - - // calling them while holding registry.mutex would establish a registry.mutex -> ledger- - // mutex lock order that any future opposite-order path would deadlock against, so these - // also run with registry.mutex released. registry.mutex is taken only afterward, briefly, - // to publish the results (plain in-memory map updates, no I/O or other locks involved). + // app_manager_add()/remove() take the ledger mutex internally, so calling them under + // registry.mutex would fix a lock order an opposite-order caller could deadlock against. + // registry.mutex is retaken afterward only to publish the in-memory results. for (const auto& id : missing_ids) { app_manager_remove(id.c_str()); } @@ -364,10 +333,8 @@ error_t app_manager_install_path_uninstall(const char* app_id) { auto path = iterator->second->path; mutex_unlock(®istry.mutex); - // Stop every running instance that retains this manifest pointer, mirroring - // stop_all_instances_of() in app_install.cpp. Collect under ledger.mutex, - // then call app_manager_stop() outside it (that call bound-joins the - // instance's thread, which itself takes ledger.mutex in its thread_main). + // Mirrors stop_all_instances_of() in app_install.cpp. Collect under ledger.mutex, stop + // outside it: app_manager_stop() bound-joins the thread, which itself takes ledger.mutex. std::vector instance_ids; auto& ledger = app_ledger(); mutex_lock(&ledger.mutex); @@ -382,14 +349,12 @@ error_t app_manager_install_path_uninstall(const char* app_id) { app_manager_stop(id); } - // app_manager_remove takes ledger.mutex internally - call outside both - // registry.mutex and ledger.mutex to match the lock ordering in - // app_manager_install_path_scan(). + // app_manager_remove() takes ledger.mutex; call outside registry.mutex too, matching + // the lock order in app_manager_install_path_scan(). app_manager_remove(app_id); - // Every instance has stopped and the manifest is unregistered — safe to - // delete the on-disk directory. Delete before erasing the scan record so - // that a failed deletion leaves the entry discoverable for a retry. + // Delete before erasing the scan record, so a failed deletion still leaves the + // entry discoverable for a retry. if (!app_fs_delete_recursively(path)) { return ERROR_RESOURCE; } diff --git a/Modules/app-module/source/scheduler.cpp b/Modules/app-module/source/scheduler.cpp index 5b7707951..5155657d6 100644 --- a/Modules/app-module/source/scheduler.cpp +++ b/Modules/app-module/source/scheduler.cpp @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 #include #include +#include #include #include #include @@ -223,7 +224,7 @@ void app_task_main(void* context) { // response to APP_EVENT_CLOSE. set_state(ctx->app_instance_id, APP_INSTANCE_STATE_STOPPED); - app_ledger_free_arguments(ctx->argc, ctx->argv); + app_arguments_free(ctx->argc, ctx->argv); AppInstanceId app_instance_id = ctx->app_instance_id; AppCompletionSignal* completion = ctx->completion; @@ -274,7 +275,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, const AppLoaderApi* loader = find_loader_api(location.type); if (loader == nullptr) { LOG_E(TAG, "[instance %lu] No app loader is registered (service '%s' not found)", app_instance_id, loader_service_id_for(location.type)); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_NOT_FOUND; } @@ -282,7 +283,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, error_t load_result = loader->load(location, &runtime); if (load_result != ERROR_NONE) { LOG_E(TAG, "[instance %lu] Failed to load app: %s", app_instance_id, error_to_string(load_result)); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return load_result; } @@ -290,7 +291,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, if (completion == nullptr) { LOG_E(TAG, "[instance %lu] Failed to allocate app", app_instance_id); loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_OUT_OF_MEMORY; } completion->semaphore = xSemaphoreCreateBinary(); @@ -298,7 +299,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, LOG_E(TAG, "[instance %lu] Failed to allocate app", app_instance_id); delete completion; loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_OUT_OF_MEMORY; } @@ -309,7 +310,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, vSemaphoreDelete(completion->semaphore); delete completion; loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_INVALID_ARGUMENT; } @@ -335,7 +336,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, vSemaphoreDelete(completion->semaphore); delete completion; loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_OUT_OF_MEMORY; } @@ -346,7 +347,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, vSemaphoreDelete(completion->semaphore); delete completion; loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_OUT_OF_MEMORY; } #else @@ -374,7 +375,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, vSemaphoreDelete(completion->semaphore); delete completion; loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_OUT_OF_MEMORY; } @@ -401,7 +402,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location, vSemaphoreDelete(completion->semaphore); delete completion; loader->unload(runtime); - app_ledger_free_arguments(argc, argv); + app_arguments_free(argc, argv); return ERROR_OUT_OF_MEMORY; } vTaskSuspend(task_handle);