diff --git a/docs/CHECKFORUPDATE_PROGRESS.md b/docs/CHECKFORUPDATE_PROGRESS.md new file mode 100755 index 00000000..655a0ffb --- /dev/null +++ b/docs/CHECKFORUPDATE_PROGRESS.md @@ -0,0 +1,126 @@ +# CheckForUpdate Redesign: Progress & Next Steps + +> **Last updated:** 2026-03-17 +> **Reference:** [`DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md`](./DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md) + +--- + +## ✅ Completed + +### Design & Documentation +- [x] Design document created: rationale, architecture, edge cases, migration phases, unit test plan +- [x] File-by-file change specification (§11 in design doc) +- [x] Multi-client scenario walkthrough (§6) +- [x] Thread safety proof (§8) +- [x] Resource cost comparison (§13) +- [x] Inline code documentation added to all modified source files (TL;DR comments) + +### Implementation (Phase 1) +- [x] `rdkFwupdateMgr_async_internal.h` — Added `CheckRequestContext` struct, worker thread declarations, session-state query API +- [x] `rdkFwupdateMgr_async_internal.h` — Removed legacy `CallbackEntry`, `CallbackRegistry`, `CallbackEntryState`, `CALLBACK_TIMEOUT_SECONDS` +- [x] `rdkFwupdateMgr_async.c` — Implemented `internal_check_worker_thread()` (on-demand worker) +- [x] `rdkFwupdateMgr_async.c` — Implemented `on_check_signal_handler()` (fires callback directly) +- [x] `rdkFwupdateMgr_async.c` — Implemented `on_check_timeout()` (120s safety net) +- [x] `rdkFwupdateMgr_async.c` — Implemented `internal_is_check_in_progress()` (session-state query) +- [x] `rdkFwupdateMgr_async.c` — Implemented `internal_cancel_all_active_check_threads()` (destructor cleanup) +- [x] `rdkFwupdateMgr_async.c` — Removed legacy `g_registry`, `on_check_complete_signal()`, `dispatch_all_pending()`, `internal_register_callback()` +- [x] `rdkFwupdateMgr_async.c` — Removed `CheckForUpdateComplete` subscription from background thread +- [x] `rdkFwupdateMgr_async.c` — Background thread now unsubscribes Download/Update signals on exit +- [x] `rdkFwupdateMgr_api.c` — Rewrote `checkForUpdate()` to use on-demand worker thread model +- [x] `rdkFwupdateMgr_api.c` — Updated library destructor to cancel/join active worker before BG thread cleanup +- [x] `rdkFwupdateMgr_process.c` — Added session-state guard in `unregisterProcess()` (rejects if check in progress) +- [x] All modified files compile cleanly (zero errors) + +### Verification +- [x] `example_app.c` verified — works with new API, no changes needed +- [x] Public API (`rdkFwupdateMgr_client.h`) unchanged — zero ABI breakage +- [x] Download/Update code paths unchanged and unaffected + +--- + +## 🔄 In Progress + +### Device Testing +- [ ] **Cross-compile for target device** — verify build succeeds on device toolchain +- [ ] **Runtime smoke test** — `registerProcess()` → `checkForUpdate()` → callback fires → `unregisterProcess()` +- [ ] **Session-state guard test** — call `unregisterProcess()` during active check, verify rejection log +- [ ] **Timeout test** — stop daemon, call `checkForUpdate()`, verify 120s timeout and clean exit +- [ ] **Library unload test** — `dlclose()` during active check, verify destructor joins worker + +--- + +## ⏳ Pending (Next Steps) + +### Unit Tests (Priority: HIGH) +| # | Test | File | Status | +|---|------|------|--------| +| 1 | `WorkerThread_StartsAndStops` | new gtest file | ⬜ | +| 2 | `WorkerThread_FiresCallback` | new gtest file | ⬜ | +| 3 | `WorkerThread_Timeout` | new gtest file | ⬜ | +| 4 | `WorkerThread_DBusFailure` | new gtest file | ⬜ | +| 5 | `DuplicateRequest_Rejected` | new gtest file | ⬜ | +| 6 | `UnregisterDuringCheck_Rejected` | new gtest file | ⬜ | +| 7 | `UnregisterAfterCallback_Succeeds` | new gtest file | ⬜ | +| 8 | `LibraryUnloadDuringCheck` | new gtest file | ⬜ | +| 9 | `CallbackDataValidity` | new gtest file | ⬜ | +| 10 | `MultiProcess_BothReceiveSignal` | integration test | ⬜ | +| 11 | `SIGTERM_DuringCheck_ExitClean` | new gtest file | ⬜ | + +### Legacy Tests to Rewrite +| # | File | Reason | +|---|------|--------| +| 1 | `rdkFwupdateMgr_async_cleanup_gtest.cpp` | References old registry init/cleanup | +| 2 | `rdkFwupdateMgr_async_refcount_gtest.cpp` | Tests old registry slot refcounting | +| 3 | `rdkFwupdateMgr_async_signal_gtest.cpp` | Tests old signal dispatch through registry | +| 4 | `rdkFwupdateMgr_async_stress_gtest.cpp` | Uses old `g_async_registry`, concurrent registration | +| 5 | `rdkFwupdateMgr_async_threadsafety_gtest.cpp` | Tests old concurrent registration/dispatch | + +### Integration Testing +- [ ] Multi-process scenario: two separate apps call `checkForUpdate()`, both receive callback +- [ ] Daemon restart during active check: verify 120s timeout fires, clean exit +- [ ] Rapid register/check/unregister cycles: no leaks, no crashes + +--- + +## 🔮 Future Phases + +### Phase 1.5: `cancelCheckForUpdate()` API +- Add ability to tear down an active worker thread mid-flight +- Enables clean `SIGTERM → cancel → unregister → exit` flow +- Estimated effort: ~4 hours + +### Phase 2: Migrate Download to On-Demand Thread +- Same pattern as CheckForUpdate but with multi-fire callback +- Worker stays alive across multiple `DownloadProgress` signals +- Estimated effort: ~8 hours + +### Phase 3: Migrate Update to On-Demand Thread +- Same as Phase 2 but for `UpdateProgress` +- Estimated effort: ~6 hours + +### Phase 4: Remove Persistent Background Thread +- Remove `internal_system_init()` / `internal_system_deinit()` +- Remove `BackgroundThread` struct +- Library constructor becomes a true no-op +- Zero resource cost when library is loaded but no API calls made +- Estimated effort: ~4 hours + +### API Improvements +- Change `unregisterProcess()` return type from `void` to `UnregisterResult` enum +- Add error codes for session-state violations (currently log-only) +- Add configurable timeout (env var or RFC parameter) + +--- + +## 📁 Modified Files Summary + +| File | Changes | +|------|---------| +| `librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h` | Added `CheckRequestContext`, worker declarations, session-state API. Removed legacy registry types. | +| `librdkFwupdateMgr/src/rdkFwupdateMgr_async.c` | On-demand worker engine, signal/timeout handlers, cancel/query APIs. Removed old registry + dispatch code. | +| `librdkFwupdateMgr/src/rdkFwupdateMgr_api.c` | Rewrote `checkForUpdate()`, updated destructor. | +| `librdkFwupdateMgr/src/rdkFwupdateMgr_process.c` | Session-state guard in `unregisterProcess()`. | +| `librdkFwupdateMgr/include/rdkFwupdateMgr_client.h` | **NO CHANGES** (public API unchanged) | +| `librdkFwupdateMgr/examples/example_app.c` | **NO CHANGES** (works as-is) | +| `docs/DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md` | Full design document | +| `docs/CHECKFORUPDATE_PROGRESS.md` | This file | diff --git a/docs/DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md b/docs/DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md new file mode 100755 index 00000000..672bb9d2 --- /dev/null +++ b/docs/DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md @@ -0,0 +1,1316 @@ +# CheckForUpdate API — On-Demand Worker Thread Redesign + +## Document Version + +| Version | Date | Author | Description | +|---------|------------|--------|------------------------------------------| +| 1.0 | 2026-03-16 | — | Initial design, analysis, and migration plan | +| 1.1 | 2026-03-16 | — | REVISED §5.4: Block unregisterProcess() during active checkForUpdate(). Added §9.9 (SIGTERM handling). Updated §11.5 (process.c changes). Updated §15.1 resolved items. | + +--- + +## Table of Contents + +1. [Executive Summary](#1-executive-summary) +2. [Terminology & Clarifications](#2-terminology--clarifications) +3. [Current Architecture (Before)](#3-current-architecture-before) +4. [Proposed Architecture (After)](#4-proposed-architecture-after) +5. [Design Decisions & Rationale](#5-design-decisions--rationale) +6. [Multi-Client Scenario Walkthrough](#6-multi-client-scenario-walkthrough) +7. [Thread Lifecycle & Memory Ownership](#7-thread-lifecycle--memory-ownership) +8. [Thread Safety Proof](#8-thread-safety-proof) +9. [Edge Cases & Robustness](#9-edge-cases--robustness) +10. [Dead Code Removal Plan](#10-dead-code-removal-plan) +11. [File-by-File Change Specification](#11-file-by-file-change-specification) +12. [Unit Test Impact](#12-unit-test-impact) +13. [Resource Cost Comparison](#13-resource-cost-comparison) +14. [Migration Phases](#14-migration-phases) +15. [Open Items & Future Work](#15-open-items--future-work) + +--- + +## 1. Executive Summary + +This document describes the redesign of the `checkForUpdate()` API implementation +within `librdkFwupdateMgr.so`. The change replaces the **persistent background +thread** model (thread created at library load, lives until library unload) with an +**on-demand worker thread** model (thread created per `checkForUpdate()` call, +destroyed after the callback fires). + +**Goals:** + +- Zero resource cost when no `checkForUpdate()` is in progress +- Thread exists only for the duration of one firmware check operation +- No change to the public API (`rdkFwupdateMgr_client.h`) +- Correct multi-client behavior (separate processes A and B both get callbacks) +- No memory leaks, no crashes, no dangling threads +- Clean dead code removal of the old CheckForUpdate registry + +**Scope:** `checkForUpdate()` API only. `downloadFirmware()` and `updateFirmware()` +remain on the existing persistent-thread model in this phase and will be migrated +subsequently. + +--- + +## 2. Terminology & Clarifications + +### 2.1 What is "the caller"? + +**The caller** is the **client application's thread** that calls `checkForUpdate()`. +This is the app's main thread (or whichever thread the app uses to invoke the API). + +Example from `example_app.c`: +```c +// This is the CALLER — it's the app's main() thread +CheckForUpdateResult cfu_result = checkForUpdate(g_handle, on_firmware_check_callback); +// ← checkForUpdate() returns here. The caller is free to do anything after this. +``` + +After `checkForUpdate()` returns `CHECK_FOR_UPDATE_SUCCESS`, the caller's +involvement is **over**. The caller does not wait, does not block, does not touch +any internal state. It is the caller's application code that continues executing. + +The caller's stack frame for `checkForUpdate()` is indeed "gone" after the function +returns — meaning the local variables inside the `checkForUpdate()` function body +are deallocated. But this is irrelevant because: + +### 2.2 What is `ctx` (CheckRequestContext)? + +`ctx` is a **heap-allocated** structure (`calloc`/`malloc`). It is NOT a stack +variable. It lives on the heap, which means it survives after `checkForUpdate()` +returns. + +**Lifecycle of `ctx`:** + +``` +CALLER THREAD WORKER THREAD +───────────── ───────────── +checkForUpdate() { + ctx = calloc(1, sizeof(*ctx)); ← ctx BORN on the heap + ctx->handle_key = strdup(handle); + ctx->callback = callback; + pthread_create(worker, ctx); ← ownership TRANSFERRED to worker + pthread_cond_wait(ctx->ready); ← caller reads ctx->is_ready (under mutex) + return SUCCESS; ← caller NEVER touches ctx again +} ← stack frame gone, but ctx is on heap! + │ + ├─ worker uses ctx throughout its life + ├─ worker fires ctx->callback + ├─ worker frees ctx->handle_key + ├─ worker destroys ctx->ready_mutex + ├─ worker destroys ctx->ready_cond + └─ free(ctx) ← ctx DIES +``` + +**Key point:** `ctx` is owned by the heap. The caller allocates it, then transfers +ownership to the worker thread. After the condvar handshake, the caller never +reads or writes `ctx` again. The worker thread is the sole owner and is responsible +for freeing it. + +### 2.3 What is "the worker thread"? + +The **worker thread** is a `pthread` spawned by `checkForUpdate()`. It: + +1. Creates a GLib event loop +2. Connects to D-Bus +3. Subscribes to `CheckForUpdateComplete` signal +4. Sends the `CheckForUpdate` D-Bus method call to the daemon +5. Signals the caller "I'm ready" via condvar +6. Runs the event loop, waiting for the daemon's signal +7. When signal arrives: parses it, fires the client's callback +8. Cleans up all resources and exits (thread terminates) + +The worker thread is **not** a persistent thread. It is born for one request and +dies when that request is complete. + +--- + +## 3. Current Architecture (Before) + +### 3.1 What happens today + +``` +Library load (__attribute__((constructor))) + │ + └─► internal_system_init() + ├─ Initialize g_registry (30-slot CallbackEntry array + mutex) + ├─ Initialize g_dwnl_registry (30-slot DwnlCallbackEntry array + mutex) + ├─ Initialize g_update_registry (30-slot UpdateCbEntry array + mutex) + ├─ Create GMainContext + GMainLoop + └─ pthread_create(background_thread_func) + │ + ├─ Connect to D-Bus + ├─ Subscribe to CheckForUpdateComplete + ├─ Subscribe to DownloadProgress + ├─ Subscribe to UpdateProgress + ├─ Signal ready (spin-wait) + └─ g_main_loop_run() ← BLOCKS FOREVER until library unload + │ + │ (idle... idle... idle... for hours/days) + │ + │ signal arrives → on_check_complete_signal() + │ → dispatch_all_pending() + │ → fires ALL PENDING callbacks (broadcast to everyone) + │ + │ (idle again...) + +checkForUpdate(handle, callback) + ├─ Validate handle + callback + ├─ Connect to D-Bus (from caller thread — a SECOND connection) + ├─ internal_register_callback(handle, callback) → puts in g_registry[slot] + ├─ g_dbus_connection_call("CheckForUpdate") → fire-and-forget from caller thread + └─ Return CHECK_FOR_UPDATE_SUCCESS + +Library unload (__attribute__((destructor))) + └─► internal_system_deinit() + ├─ g_main_loop_quit() → background thread wakes up + ├─ pthread_join() → wait for thread to exit + └─ Free all registries, mutexes, GLib objects +``` + +### 3.2 Problems with current design + +| Problem | Details | +|---------|---------| +| Persistent idle thread | Thread + D-Bus connection + GMainContext consume ~14KB even when no requests are active | +| No signal routing | `dispatch_all_pending()` fires ALL pending callbacks regardless of which handler_id the signal is for | +| Constructor overhead | Thread, D-Bus connection, and 3 registries created at library load even if the app never calls `checkForUpdate()` | +| Spin-wait at init | `internal_system_init()` uses 50 × 100ms nanosleep polling loop instead of a proper condvar | +| Timeout not implemented | `CALLBACK_TIMEOUT_SECONDS = 60` is defined but never enforced — stale PENDING entries accumulate forever | +| Two D-Bus connections | The caller thread creates a connection for fire-and-forget, while the BG thread has a separate connection for signal listening | + +--- + +## 4. Proposed Architecture (After) + +### 4.1 New flow for checkForUpdate() + +``` +Library load (__attribute__((constructor))) + │ + └─► internal_system_init() ← STILL CALLED (for Download/Update) + ├─ Initialize g_dwnl_registry ← KEPT (for downloadFirmware) + ├─ Initialize g_update_registry ← KEPT (for updateFirmware) + ├─ Create GMainContext + GMainLoop + └─ pthread_create(background_thread_func) + ├─ Connect to D-Bus + ├─ Subscribe to DownloadProgress ← KEPT + ├─ Subscribe to UpdateProgress ← KEPT + ├─ (CheckForUpdateComplete subscription REMOVED) + └─ g_main_loop_run() + +checkForUpdate(handle, callback) + │ + ├─ [1] Validate handle (not NULL, not empty) + ├─ [2] Validate callback (not NULL) + ├─ [3] Check: is a checkForUpdate already in progress for this process? + │ If YES → log warning, return CHECK_FOR_UPDATE_FAIL + ├─ [4] Allocate CheckRequestContext on heap + │ ctx->handle_key = strdup(handle) + │ ctx->callback = callback + │ init ready_mutex, ready_cond + ├─ [5] Set g_check_in_progress = true + ├─ [6] Track ctx in active list (for library unload safety) + ├─ [7] pthread_create(internal_check_worker_thread, ctx) + │ │ + │ ├─ [A] g_main_context_new() (isolated) + │ ├─ [B] g_main_loop_new() + │ ├─ [C] g_main_context_push_thread_default() + │ ├─ [D] g_bus_get_sync() → connection + │ │ (if FAIL: set init_failed, signal ready, goto cleanup) + │ ├─ [E] g_dbus_connection_signal_subscribe( + │ │ "CheckForUpdateComplete", + │ │ handler = on_check_signal_handler, + │ │ user_data = ctx) + │ ├─ [F] g_dbus_connection_call( + │ │ "CheckForUpdate", handle) + │ │ ← D-Bus request sent from worker thread + │ ├─ [G] Add 120s timeout to GMainContext + │ ├─ [H] Signal ready: ctx->is_ready = true + │ │ pthread_cond_signal() + │ │ + ├─ [8] pthread_cond_wait(ctx->ready_cond) │ + │ ← NO TIMEOUT on this wait │ + │ (see Section 5.1 for rationale) │ + │ │ + │ ← wakes up when worker signals ├─ [I] g_main_loop_run() + │ │ ← BLOCKS until signal or 120s timeout + ├─ [9] Check ctx->init_failed │ + │ If true → return CHECK_FOR_UPDATE_FAIL + │ (worker thread cleans itself up) │ + │ │ ... daemon does XConf query (5s - 2min+) ... + ├─ [10] Return CHECK_FOR_UPDATE_SUCCESS │ + │ ← CALLER IS FREE │ + │ ├─ [J] Signal arrives from daemon + │ on_check_signal_handler(ctx): + │ parse GVariant → FwInfoData + │ ctx->callback(&fwinfo_data) + │ g_main_loop_quit() + │ + ├─ [K] g_main_loop_run() returns + ├─ [L] Cleanup: + │ unsubscribe signal + │ g_object_unref(connection) + │ g_main_context_pop_thread_default() + │ g_main_loop_unref() + │ g_main_context_unref() + │ untrack from active list + │ Set g_check_in_progress = false + │ free(ctx->handle_key) + │ destroy ready_mutex, ready_cond + │ free(ctx) + └─ [M] return NULL ← thread exits + +Library unload (__attribute__((destructor))) + └─► rdkFwupdateMgr_lib_deinit() + ├─ internal_cancel_all_active_check_threads() + │ ├─ For each active ctx: g_main_loop_quit() + │ └─ For each active ctx: pthread_join() + └─ internal_system_deinit() ← for Download/Update cleanup +``` + +--- + +## 5. Design Decisions & Rationale + +### 5.1 DECIDED: No timeout on the condvar wait in checkForUpdate() + +**Question raised:** "If the worker thread takes >5 seconds to reach the ready signal, +`pthread_cond_timedwait()` returns `ETIMEDOUT`." + +**Clarification:** There are **two different waits** to reason about: + +| Wait | What it waits for | How long? | Timeout? | +|------|-------------------|-----------|----------| +| **Wait #1** — in `checkForUpdate()` (caller thread) | Worker thread to start up, connect D-Bus, subscribe, send request, and signal "ready" | Typically <100ms (D-Bus connect + subscribe + call) | **NO TIMEOUT** | +| **Wait #2** — in worker thread (`g_main_loop_run()`) | Daemon to emit `CheckForUpdateComplete` signal after XConf query | 5 seconds to 2+ minutes | **120 second timeout** | + +**Wait #1 is NOT waiting for the daemon.** It is only waiting for the worker thread +to set up its GLib event loop and fire the D-Bus call. This is a purely local +operation (~10-100ms). If D-Bus itself is completely dead, `g_bus_get_sync()` will +fail and the worker will signal `init_failed = true`. So Wait #1 does not need a +timeout. + +**Wait #2 IS waiting for the daemon** (XConf query). This is where the daemon can +take 2+ minutes. The 120-second timeout on the GMainLoop protects against the +daemon never responding. But the caller never experiences this wait — the caller +already returned `SUCCESS` at step [10]. + +**Decision:** `checkForUpdate()` uses `pthread_cond_wait()` (**no timeout**) for Wait #1. +The worker thread uses a 120-second `g_timeout_source` for Wait #2. + +**What if D-Bus is extremely slow but not dead?** `g_bus_get_sync()` has its own +internal timeout (GLib default: 25 seconds). If it takes that long, the worker +thread is stuck at step [D] for 25 seconds, and the caller is stuck at step [8] +for 25 seconds. This is the worst case for Wait #1. + +**Is 25 seconds acceptable for Wait #1?** On an embedded STB, if D-Bus itself is +unresponsive for 25 seconds, the system has bigger problems. The caller blocking +for 25 seconds is acceptable in this extreme scenario. If we wanted to cap it, +we could use a 10-second `pthread_cond_timedwait()`, but the failure handling +gets complex (see next section). + +**Final decision: Use plain `pthread_cond_wait()` (no timeout) for Wait #1.** +Rationale: simpler, avoids the complex failure/cancellation path, and the +scenario where this blocks for more than ~100ms is extremely rare. + +### 5.2 DECIDED: No timeout cancellation complexity + +**Question raised:** "If the caller returned FAIL due to timeout, but the worker +eventually succeeds and fires the callback — is that acceptable?" + +**This question is now MOOT** because we decided NOT to timeout Wait #1. The caller +will always wait until the worker signals ready. The worker either: + +- Succeeds → signals `is_ready = true`, `init_failed = false` → caller returns SUCCESS +- Fails (D-Bus error) → signals `is_ready = true`, `init_failed = true` → caller returns FAIL + +There is no scenario where the caller returns FAIL but the worker later fires the +callback. The only way the caller returns FAIL is if the worker itself failed to +initialize, in which case the worker goes directly to cleanup and never fires any +callback. + +**Result:** No need for a `cancelled` flag. No ghost callbacks. No ambiguity. ✅ + +### 5.3 DECIDED: Reject duplicate checkForUpdate() calls from the same process + +**Question raised:** "Worker thread A and worker thread B (if two `checkForUpdate()` +calls are made from the same process) share the same underlying D-Bus connection — +we should actually stop app from making such multiple requests." + +**Agreed.** A single client process should not have two concurrent `checkForUpdate()` +requests in flight. The reasons: + +1. **Daemon side:** The daemon does one XConf query and broadcasts one signal. + Two concurrent requests from the same process would create two threads both + listening for the same signal, both firing the same callback with the same data. + This is wasteful and confusing for the client. + +2. **Resource waste:** Two threads, two GMainContexts, two signal subscriptions + for identical data. + +3. **Client confusion:** If the client gets two callbacks, it may double-process + the firmware info. + +**Implementation:** Add a process-global flag `g_check_in_progress` (protected by a +mutex) that is set to `true` when `checkForUpdate()` spawns a worker, and reset to +`false` when the worker exits (after callback or timeout). + +```c +/* In rdkFwupdateMgr_async.c */ +static pthread_mutex_t g_check_in_progress_mutex = PTHREAD_MUTEX_INITIALIZER; +static bool g_check_in_progress = false; + +/* In checkForUpdate(): */ +pthread_mutex_lock(&g_check_in_progress_mutex); +if (g_check_in_progress) { + pthread_mutex_unlock(&g_check_in_progress_mutex); + FWUPMGR_WARN("checkForUpdate: already in progress, rejecting\n"); + return CHECK_FOR_UPDATE_FAIL; +} +g_check_in_progress = true; +pthread_mutex_unlock(&g_check_in_progress_mutex); + +/* In worker thread cleanup: */ +pthread_mutex_lock(&g_check_in_progress_mutex); +g_check_in_progress = false; +pthread_mutex_unlock(&g_check_in_progress_mutex); +``` + +### 5.4 ~~DECIDED: Do NOT block unregisterProcess() during active checkForUpdate()~~ + +### 5.4 REVISED: BLOCK unregisterProcess() during active checkForUpdate() + +> **History:** The original decision (v1.0) was to keep `unregisterProcess()` +> completely independent. After deeper system-level analysis, this was reversed. +> The original rationale is preserved below (struck through) for audit trail, +> followed by the revised decision. + +**Original question:** "Do you think we should stop the app from calling unregister +until the current checkForUpdate is completed?" + +#### ~~Original Decision (v1.0): Do NOT block~~ — SUPERSEDED + +~~Rationale was: (1) unregisterProcess() is stateless, (2) blocking could cause +2-minute hangs on SIGTERM, (3) worker has its own strdup'd handle so no UAF, +(4) callback function pointers stay valid. While these technical observations +are true, they miss the architectural point.~~ + +#### Revised Decision (v1.1): BLOCK unregisterProcess() — Return failure if checkForUpdate is active + +**The fundamental insight:** `registerProcess()` and `unregisterProcess()` represent +a **session** between the client and the daemon, not just memory allocation/deallocation. + +| API Call | Semantic Meaning | +|----------|-----------------| +| `registerProcess()` | "I am a client. I exist. I want to interact with you." | +| `checkForUpdate()` | "Within my active session, check firmware and tell me when done." | +| `unregisterProcess()` | "I'm done. Forget about me. I will not interact further." | + +**Calling `unregisterProcess()` while `checkForUpdate()` is in flight is a semantic +contradiction.** The app is saying "forget about me" while simultaneously expecting +"tell me when you're done." This is like hanging up the phone and expecting to hear +the answer. + +**What happens on the daemon side if this is allowed:** +- The daemon receives `UnregisterProcess(handler_id)` and removes the client from + its internal tracking. +- The daemon may or may not still emit the `CheckForUpdateComplete` signal (the + XConf query may already be in flight and can't be cancelled). +- The relationship is logically severed. The signal might arrive, might not. + The data might reference a handle the daemon no longer recognizes. +- This is **undefined territory** — exactly what good API design prevents. + +**The universal pattern:** You cannot end a session while you have outstanding +operations. This principle appears everywhere in systems programming: +- You can't `close()` a file descriptor while an `aio_read()` is pending (UB) +- You can't destroy a socket while an async `recv()` is in flight +- You can't `dlclose()` a library while its threads are still running +- You can't `CloseHandle()` on a Windows IOCP while completion packets are pending + +**Implementation:** `unregisterProcess()` will check the process-global +`g_check_in_progress` flag and **reject the call** (not block/wait): + +```c +/* In unregisterProcess(), before any D-Bus call: */ +#include "rdkFwupdateMgr_async_internal.h" /* for internal_is_check_in_progress() */ + +if (internal_is_check_in_progress()) { + FWUPMGR_ERROR("unregisterProcess: cannot unregister while " + "checkForUpdate() is in progress. Wait for the " + "callback to fire, then unregister.\n"); + return; /* Do NOT free(handler) — caller still owns it */ +} +/* ... proceed with normal unregistration ... */ +``` + +**Critical detail — reject, don't block:** We return immediately with a logged error +rather than blocking. If we blocked (`pthread_cond_wait` on the worker to finish), +we'd risk a 2-minute hang during SIGTERM. By rejecting, we give the app clear +feedback: "Your call sequence is wrong. Fix it." + +**The correct app sequence:** +```c +registerProcess() → checkForUpdate() → [wait for callback] → unregisterProcess() +``` + +**If the app receives SIGTERM during a check:** +1. **Best:** Wait for the callback (120s max), then unregister. The callback has a + bounded timeout, so the app will not hang forever. +2. **Acceptable:** Just `exit()`. The daemon will detect the D-Bus peer disconnect + and clean up the registration automatically. No resource leak. +3. **Future enhancement:** Add a `cancelCheckForUpdate()` API that cleanly tears + down the worker thread, then the app can unregister. + +**Note on `void` return type:** The current `unregisterProcess()` signature returns +`void`. We cannot return an error code without an API break. Options: +- **Option A (recommended for Phase 1):** Log a loud error and return without + doing anything. The caller still holds a valid handle and can retry after + the callback fires. This is a **logical no-op** when check is in progress. +- **Option B (Phase 2 API update):** Change return type to `UnregisterResult` + enum. This is an API break but a cleaner contract. + +**Why the original "don't block" decision was wrong:** +The original reasoning was technically correct (no memory corruption, no crashes) +but architecturally wrong. Just because something doesn't crash doesn't mean it +should be allowed. Allowing `unregisterProcess()` during an active check creates +an **undefined state** in the daemon-client relationship. Good API design makes +illegal states unrepresentable — or at minimum, rejects them at the call site. + +**Summary:** `unregisterProcess()` now validates session state before proceeding. +If a `checkForUpdate()` is in progress, the call is rejected with a log message. +The caller must wait for the callback before unregistering. + +### 5.5 DECIDED: Persistent thread stays for Download/Update (Phase 1) + +**Decision:** In this phase, `internal_system_init()` is still called from the +library constructor. The persistent background thread still runs. But it is +**modified** to only subscribe to `DownloadProgress` and `UpdateProgress` — the +`CheckForUpdateComplete` subscription is **removed** from it. + +**Why not leave the old CheckForUpdate subscription and let it be "harmless"?** + +Because that would be dead code. The old `on_check_complete_signal()` handler +would fire, call `dispatch_all_pending()`, find zero entries, and return. This +wastes CPU cycles parsing the GVariant for nothing. More importantly: + +- It makes the codebase confusing (two handlers for the same signal) +- It makes debugging harder (signal appears to be handled twice in logs) +- It violates the principle of removing dead code + +**Clean approach:** Remove the `CheckForUpdateComplete` subscription from the +persistent thread, and remove all CheckForUpdate registry code. See Section 10. + +--- + +## 6. Multi-Client Scenario Walkthrough + +### Scenario: Process A and Process B both call checkForUpdate() + +**Important:** A and B are **separate OS processes**. Each has its own copy of +`librdkFwupdateMgr.so` loaded. They share **nothing** in memory. The only shared +channel is the D-Bus system bus. + +``` +PROCESS A D-BUS SYSTEM BUS PROCESS B +───────── ────────────────── ───────── + +registerProcess("AppA") ──────► Daemon assigns ID=1 +handle_A = "1" ◄────── registerProcess("AppB") + Daemon assigns ID=2 ◄────── + ──────► handle_B = "2" + +checkForUpdate("1", cbA) checkForUpdate("2", cbB) +├─ Validate ✓ ├─ Validate ✓ +├─ g_check_in_progress=true ├─ g_check_in_progress=true +├─ Alloc ctx_A ├─ Alloc ctx_B +├─ spawn worker_A ├─ spawn worker_B +│ │ +│ worker_A: │ worker_B: +│ ├─ subscribe(Complete) │ ├─ subscribe(Complete) +│ ├─ call(CheckForUpdate,"1") ───► Daemon receives "1" │ ├─ call(CheckForUpdate,"2") +│ ├─ signal ready Daemon receives "2" ◄─── │ ├─ signal ready +│ └─ g_main_loop_run() │ └─ g_main_loop_run() +│ │ +├─ condvar wakes up ├─ condvar wakes up +├─ Return SUCCESS ├─ Return SUCCESS +│ │ +│ App A does other work Daemon queries XConf... │ App B does other work +│ ... 5-30 seconds ... │ +│ │ +│ Daemon emits signal │ +│ (BROADCAST, dest=NULL, │ +│ handler_id=1, │ +│ firmware data) │ +│ │ │ +│ worker_A receives signal ◄─────────────┤──────────────────────► worker_B receives signal +│ ├─ Parse GVariant │ ├─ Parse GVariant +│ ├─ Build FwInfoData ├─ Build FwInfoData +│ ├─ cbA(&fwinfo_data) ├─ cbB(&fwinfo_data) +│ ├─ g_main_loop_quit() ├─ g_main_loop_quit() +│ ├─ cleanup ├─ cleanup +│ ├─ g_check_in_progress=false ├─ g_check_in_progress=false +│ └─ thread exits └─ thread exits +│ │ +│ App A's callback data ready App B's callback data ready │ +``` + +**Why both receive the signal:** D-Bus broadcast signals (destination=NULL) are +delivered to **every connection** on the system bus that has a matching subscription. +Process A and Process B have separate D-Bus connections (separate socket FDs). +Both subscribed to `CheckForUpdateComplete`. Both receive it. + +**Why both should fire their callbacks:** The daemon queries XConf once and +broadcasts the result. The firmware data (available version, download URL, etc.) +is **the same for the device** regardless of which client asked. Both A and B +want the same answer. So both callbacks firing with the same data is **correct +behavior**. + +**The handler_id in the signal** (`handler_id=1` from the first requester) is +present in the GVariant payload. In this design, we do NOT filter by handler_id. +Both worker threads fire their callbacks regardless of which handler_id is in the +signal. This is correct because: + +1. XConf response is device-global, not client-specific +2. The daemon may batch requests (one XConf query for multiple clients) +3. The handler_id in the signal is the first requester's ID, not a per-client field + +--- + +## 7. Thread Lifecycle & Memory Ownership + +### 7.1 Complete lifecycle diagram + +``` + HEAP + ┌─────────────────────────────────────┐ +CALLER THREAD │ CheckRequestContext *ctx │ WORKER THREAD +───────────── │ │ ───────────── + │ handle_key ──► strdup("1") │ +calloc(ctx) ───────►│ callback ──► cbA │ + │ ready_mutex, ready_cond │ + │ is_ready = false │ + │ init_failed = false │ +pthread_create() ──►│ thread ──► worker thread ID │◄── thread starts + │ │ +cond_wait() │ (worker sets up GLib, D-Bus...) │ g_main_context_new() + │ blocked │ │ g_bus_get_sync() + │ │ is_ready = true ◄──────────────────│ subscribe + call + │ wakes up ◄──────│ cond_signal() │ g_main_loop_run() + │ │ │ │ blocked +reads init_failed │ │ │ + │ │ OWNERSHIP WALL │ │ + │ │ ═══════════════ │ │ + ▼ │ Caller NEVER touches ctx again │ │ +return SUCCESS │ │ │ + │ │ ▼ signal arrives + │ │ callback fires + │ │ g_main_loop_quit() + │ │ + │ free(handle_key) ◄──────────────────│ cleanup + │ destroy mutex, cond ◄──────────────│ + └─────────────────────────────────────┘ + free(ctx) ◄────────────────────────────│ thread exits +``` + +### 7.2 Memory ownership rules + +| Memory | Allocated by | Owned by | Freed by | +|--------|-------------|----------|----------| +| `ctx` itself | Caller (`calloc`) | Worker thread (after condvar handshake) | Worker thread (`free`) | +| `ctx->handle_key` | Caller (`strdup`) | Worker thread | Worker thread (`free`) | +| `ctx->callback` | N/A (function pointer, not heap memory) | N/A | N/A | +| `ctx->ready_mutex` | Caller (`pthread_mutex_init`) | Worker thread | Worker thread (`pthread_mutex_destroy`) | +| `ctx->ready_cond` | Caller (`pthread_cond_init`) | Worker thread | Worker thread (`pthread_cond_destroy`) | +| `ctx->context` (GMainContext) | Worker thread | Worker thread | Worker thread (`g_main_context_unref`) | +| `ctx->main_loop` (GMainLoop) | Worker thread | Worker thread | Worker thread (`g_main_loop_unref`) | +| `ctx->connection` (GDBusConnection) | Worker thread (via GLib singleton) | GLib | Worker thread (`g_object_unref`) | + +**No double-free risk:** Every allocation has exactly one owner and one free point. + +**No use-after-free risk:** After the condvar handshake, the caller never touches +`ctx`. The worker is the sole accessor. + +--- + +## 8. Thread Safety Proof + +### 8.1 Shared mutable state inventory + +| State | Accessed by | Protection | +|-------|------------|------------| +| `ctx->is_ready`, `ctx->init_failed` | Caller (read), Worker (write) | `ctx->ready_mutex` + `ctx->ready_cond` | +| `g_check_in_progress` | Caller (read/write), Worker (write) | `g_check_in_progress_mutex` | +| `g_active_check_ctx` | Caller (write), Worker (write), Destructor (read/write) | `g_check_in_progress_mutex` (reuse same mutex) | + +**That's it.** Only 3 pieces of shared mutable state, all mutex-protected. + +Compare with current design: `g_registry` (30-entry array + mutex), `g_bg_thread` +(multiple fields + mutex) — significantly more shared state. + +### 8.2 Condvar handshake correctness + +```c +// CALLER: +pthread_mutex_lock(&ctx->ready_mutex); +while (!ctx->is_ready) { + pthread_cond_wait(&ctx->ready_cond, &ctx->ready_mutex); +} +bool failed = ctx->init_failed; +pthread_mutex_unlock(&ctx->ready_mutex); + +// WORKER: +pthread_mutex_lock(&ctx->ready_mutex); +ctx->init_failed = false; // or true on error +ctx->is_ready = true; +pthread_cond_signal(&ctx->ready_cond); +pthread_mutex_unlock(&ctx->ready_mutex); +``` + +This is the textbook condvar pattern. Safe against: + +- **Spurious wakeup:** `while (!ctx->is_ready)` re-checks the predicate. +- **Missed signal:** If worker signals before caller enters `pthread_cond_wait`, + the `while` loop checks `is_ready` which is already `true`, so the wait is + skipped entirely. +- **Data race:** Both `is_ready` and `init_failed` are read/written under the + same mutex. + +### 8.3 g_check_in_progress flag correctness + +```c +// Entry (in checkForUpdate): +pthread_mutex_lock(&g_check_in_progress_mutex); +if (g_check_in_progress) { + pthread_mutex_unlock(&g_check_in_progress_mutex); + return CHECK_FOR_UPDATE_FAIL; // reject duplicate +} +g_check_in_progress = true; +pthread_mutex_unlock(&g_check_in_progress_mutex); + +// Exit (in worker thread cleanup, ALWAYS reached): +pthread_mutex_lock(&g_check_in_progress_mutex); +g_check_in_progress = false; +pthread_mutex_unlock(&g_check_in_progress_mutex); +``` + +This guarantees: +- At most one worker thread exists at any time per process. +- The flag is always reset, even on error/timeout paths. +- No race between two rapid `checkForUpdate()` calls. + +--- + +## 9. Edge Cases & Robustness + +### 9.1 Client calls checkForUpdate() twice quickly + +```c +checkForUpdate("1", cbA); // → SUCCESS, worker spawned +checkForUpdate("1", cbA); // → FAIL, "already in progress" +``` + +**Behavior:** Second call returns `CHECK_FOR_UPDATE_FAIL` immediately with a log +message. No second thread is spawned. ✅ + +### 9.2 Client calls unregisterProcess() while check is pending + +**Behavior (revised v1.1):** `unregisterProcess()` checks `internal_is_check_in_progress()` +and **rejects the call** with a loud `FWUPMGR_ERROR` log message. The handle is NOT +freed. The caller still owns it and must retry `unregisterProcess()` after the +`checkForUpdate()` callback fires. + +```c +checkForUpdate("12345", myCallback); // → SUCCESS, worker spawned +unregisterProcess(handle); // → REJECTED (logged), handle NOT freed +// ... callback fires with FwInfoData ... +unregisterProcess(handle); // → SUCCESS, handle freed +``` + +**Rationale:** See §5.4. Unregistering during an active operation creates an +undefined daemon-client state. The API enforces the correct sequencing. ✅ + +### 9.9 App receives SIGTERM while checkForUpdate() is pending + +**Scenario:** The app's `checkForUpdate()` returned SUCCESS. The callback hasn't +fired yet. The app receives SIGTERM and wants to exit. + +**Options for the app:** + +1. **Wait for callback, then exit (recommended):** The callback is bounded by the + 120-second worker timeout. The app can install a SIGTERM handler that sets a + "shutting_down" flag. When the callback fires, the app checks the flag, calls + `unregisterProcess()`, and exits. Worst case: 120 seconds. + +2. **Just exit immediately (acceptable):** Call `_exit()` or `exit()`. The library + destructor (`__attribute__((destructor))`) will join the worker thread (via + `internal_cancel_all_active_check_threads()`). The daemon will detect the + D-Bus peer disconnect and clean up the registration automatically. No resource + leak on the daemon side. + +3. **Force-skip unregisterProcess() (acceptable):** The daemon is designed to + handle client disappearance gracefully. Orphaned registrations are cleaned up + when the D-Bus connection drops. The only "leak" is the handle's 32 bytes of + heap memory, which the OS reclaims on process exit. + +**What the app should NOT do:** +```c +// WRONG: unregisterProcess() will be rejected +signal_handler(SIGTERM) { + unregisterProcess(handle); // REJECTED — check still in progress! + exit(0); // handle leaked (not freed) +} +``` + +**Future enhancement:** A `cancelCheckForUpdate()` API would allow the app to: +```c +cancelCheckForUpdate(); // Worker thread is torn down +unregisterProcess(handle); // Now succeeds +exit(0); +``` +This is deferred to a future phase. ✅ + +### 9.3 Daemon crashes/restarts while check is pending + +**Behavior:** The D-Bus subscription becomes orphaned. The 120-second timeout +fires. `g_main_loop_quit()` is called. Worker exits cleanly. No crash. ✅ + +### 9.4 Library unloaded (dlclose) while worker thread is active + +**Behavior:** `__attribute__((destructor))` calls +`internal_cancel_all_active_check_threads()`: + +1. Calls `g_main_loop_quit(ctx->main_loop)` on the active worker (if any). +2. Calls `pthread_join(ctx->thread, NULL)` to wait for worker to exit. +3. Library code is not unmapped until `pthread_join()` returns. + +**No crash.** No code executing in unmapped memory. ✅ + +### 9.5 Signal arrives after timeout already fired + +**Timeline:** +``` +T=0s Worker starts, subscribes, sends D-Bus call +T=120s Timeout fires → g_main_loop_quit() +T=120s Worker enters cleanup, unsubscribes signal +T=121s Daemon finally emits signal +``` + +At T=121s, the signal arrives but the subscription is already removed (step +`g_dbus_connection_signal_unsubscribe()` at cleanup). GLib does not deliver +the signal. No crash. No dangling callback. ✅ + +### 9.6 Signal arrives between g_main_loop_quit() and unsubscribe + +**Timeline:** +``` +T=120.000s Timeout fires → g_main_loop_quit() +T=120.001s Signal arrives (queued in GMainContext) +T=120.002s g_main_loop_run() returns (loop is quit) +T=120.003s Worker calls g_dbus_connection_signal_unsubscribe() +``` + +At T=120.001s, the signal is queued but `g_main_loop_run()` is already returning. +The handler does NOT fire because the loop has exited. `g_dbus_connection_signal_unsubscribe()` +at T=120.003s cleans up the subscription. No crash. ✅ + +### 9.7 Worker thread D-Bus connection fails + +**Behavior:** `g_bus_get_sync()` returns NULL. Worker sets `init_failed = true`, +signals ready via condvar, goes to cleanup, frees ctx, exits. Caller sees +`init_failed = true`, returns `CHECK_FOR_UPDATE_FAIL`. No thread leak. ✅ + +### 9.8 Worker thread signal subscribe fails + +**Behavior:** `g_dbus_connection_signal_subscribe()` returns 0 on failure. The +worker should check this, set `init_failed = true`, signal ready, and go to +cleanup. The D-Bus method call is NOT sent (preventing a request with no listener). ✅ + +--- + +## 10. Dead Code Removal Plan + +### 10.1 What to remove from `rdkFwupdateMgr_async_internal.h` + +| Item | Action | Reason | +|------|--------|--------| +| `CallbackEntryState` enum | **REMOVE** | Only used by CheckForUpdate registry | +| `CallbackEntry` struct | **REMOVE** | CheckForUpdate registry entry — replaced by per-request ctx | +| `CallbackRegistry` struct | **REMOVE** | Global registry — no longer needed | +| `BackgroundThread.subscription_id` | **KEEP** (but this field is reused for Download/Update subscriptions) | Still needed for DownloadProgress/UpdateProgress | +| `internal_register_callback()` declaration | **REMOVE** | No registry to register in | +| `internal_system_init()` declaration | **KEEP** | Still initializes Download/Update registries and BG thread | +| `internal_system_deinit()` declaration | **KEEP** | Still cleans up Download/Update | +| `MAX_PENDING_CALLBACKS` | **KEEP** | Still used by Download/Update registries | +| `CALLBACK_TIMEOUT_SECONDS` | **REMOVE** | Was never used. New design has explicit 120s timeout. | + +### 10.2 What to remove from `rdkFwupdateMgr_async.c` + +| Item | Action | Reason | +|------|--------|--------| +| `static CallbackRegistry g_registry;` | **REMOVE** | No global registry | +| `on_check_complete_signal()` function | **REMOVE** | Old BG thread signal handler for CheckForUpdate | +| `dispatch_all_pending()` function | **REMOVE** | Old broadcast dispatch — replaced by direct callback in worker | +| `internal_register_callback()` function | **REMOVE** | No registry | +| `registry_reset_slot()` function | **REMOVE** | No registry slots | +| `g_registry` cleanup in `internal_system_deinit()` | **REMOVE** | No `g_registry` to clean up | +| `g_registry` init in `internal_system_init()` | **REMOVE** | No `g_registry` to init | +| `CheckForUpdateComplete` subscription in `background_thread_func()` | **REMOVE** | BG thread no longer handles CheckForUpdate signals | + +### 10.3 What to remove from `rdkFwupdateMgr_api.c` + +| Item | Action | Reason | +|------|--------|--------| +| Old `checkForUpdate()` body | **REPLACE** with new on-demand implementation | Core change | + +### 10.4 What to keep + +**Everything related to Download and Update is UNTOUCHED:** + +- `g_dwnl_registry`, `g_update_registry` — kept +- `on_download_progress_signal()` — kept +- `on_update_progress_signal()` — kept +- `dispatch_all_dwnl_active()` — kept +- `dispatch_all_update_active()` — kept +- `internal_dwnl_register_callback()` — kept +- `internal_update_register_callback()` — kept +- `internal_dwnl_system_deinit()` — kept +- `internal_update_system_deinit()` — kept +- `background_thread_func()` — kept (but removes CheckForUpdateComplete subscription) +- `internal_system_init()` — kept (but removes g_registry init) +- `internal_system_deinit()` — kept (but removes g_registry cleanup) + +**Helper functions kept (shared with new handler):** + +- `internal_parse_signal_data()` — reused by new `on_check_signal_handler()` +- `internal_cleanup_signal_data()` — reused +- `internal_map_status_code()` — reused +- `parse_update_details()` — reused + +--- + +## 11. File-by-File Change Specification + +### 11.1 `rdkFwupdateMgr_client.h` — NO CHANGES + +Public API unchanged. Zero breakage. + +### 11.2 `rdkFwupdateMgr_async_internal.h` + +**Removals:** +- `CallbackEntryState` enum +- `CallbackEntry` struct +- `CallbackRegistry` struct +- `CALLBACK_TIMEOUT_SECONDS` define +- `internal_register_callback()` declaration + +**Additions:** +```c +/* Timeout for worker thread waiting for daemon signal (seconds) */ +#define CHECK_SIGNAL_TIMEOUT_SECONDS 120 + +/** + * Per-request context for on-demand CheckForUpdate worker thread. + * + * Lifecycle: + * - Allocated in checkForUpdate() (caller thread) + * - Ownership transferred to worker thread after condvar handshake + * - Freed by worker thread after callback fires (or timeout) + * + * Memory: ~100 bytes (excluding GLib objects) + */ +typedef struct { + /* Condvar handshake: worker signals "I'm ready" to caller */ + pthread_mutex_t ready_mutex; + pthread_cond_t ready_cond; + bool is_ready; /**< true = worker finished setup */ + bool init_failed; /**< true = D-Bus connect failed */ + + /* GLib event loop (isolated, per-thread) */ + GMainContext *context; + GMainLoop *main_loop; + GDBusConnection *connection; + guint subscription_id; + + /* Request data */ + char *handle_key; /**< strdup of FirmwareInterfaceHandle */ + UpdateEventCallback callback; /**< Client's callback function ptr */ + + /* Thread handle (for join in destructor) */ + pthread_t thread; +} CheckRequestContext; + +/** + * Worker thread entry point for on-demand CheckForUpdate. + * @param arg CheckRequestContext* (ownership transferred) + * @return NULL + */ +void *internal_check_worker_thread(void *arg); +``` + +**No changes to:** +- `InternalSignalData` struct +- `internal_parse_signal_data()` / `internal_cleanup_signal_data()` / `internal_map_status_code()` declarations +- All Download types (`DwnlCallbackState`, `InternalDwnlSignalData`, `DwnlCallbackEntry`, `DwnlCallbackRegistry`) +- All Update types +- `BackgroundThread` struct (still used for Download/Update BG thread) +- `internal_system_init()` / `internal_system_deinit()` declarations + +### 11.3 `rdkFwupdateMgr_api.c` + +**Replace `checkForUpdate()` body entirely.** New implementation: + +1. Validate handle and callback (same as today) +2. Check `g_check_in_progress` — reject if already active +3. Allocate `CheckRequestContext`, copy handle and callback +4. Track context for library-unload safety +5. `pthread_create()` worker thread +6. `pthread_cond_wait()` for worker to signal ready +7. If `init_failed` → return `CHECK_FOR_UPDATE_FAIL` +8. Return `CHECK_FOR_UPDATE_SUCCESS` + +**Modify constructor:** Keep `internal_system_init()` call (for Download/Update). +Add init of `g_check_in_progress_mutex`. + +**Modify destructor:** Add `internal_cancel_all_active_check_threads()` call +before `internal_system_deinit()`. + +### 11.4 `rdkFwupdateMgr_async.c` + +**Remove** (CheckForUpdate-specific old code): +- `static CallbackRegistry g_registry;` +- `g_registry` init in `internal_system_init()` +- `g_registry` cleanup in `internal_system_deinit()` +- `on_check_complete_signal()` function +- `dispatch_all_pending()` function +- `internal_register_callback()` function +- `registry_reset_slot()` function +- `CheckForUpdateComplete` subscription in `background_thread_func()` + +**Add** (new on-demand CheckForUpdate code): + +1. `static pthread_mutex_t g_check_in_progress_mutex;` +2. `static bool g_check_in_progress;` +3. `static CheckRequestContext *g_active_check_ctx;` + (only one can be active at a time due to dedup, so a single pointer suffices) +4. `void *internal_check_worker_thread(void *arg)` — worker function +5. `static void on_check_signal_handler(...)` — signal handler (fires callback, quits loop) +6. `static gboolean on_check_timeout(gpointer user_data)` — timeout handler +7. `void internal_cancel_all_active_check_threads(void)` — for destructor + +**No changes to:** +- All Download engine functions +- All Update engine functions +- `internal_parse_signal_data()`, `internal_cleanup_signal_data()`, `internal_map_status_code()` +- `parse_update_details()` +- `background_thread_func()` (except removing CheckForUpdateComplete subscription) +- `internal_system_init()` (except removing g_registry init) +- `internal_system_deinit()` (except removing g_registry cleanup) + +### 11.5 `rdkFwupdateMgr_process.c` — MODIFIED (Session State Validation) + +**Context:** `unregisterProcess()` must now validate that no `checkForUpdate()` is +in progress before proceeding. This introduces a dependency from `_process.c` to +the async engine's state, but through a clean, narrow API boundary. + +**Changes:** + +1. **Add include:** `#include "rdkFwupdateMgr_async_internal.h"` (for `internal_is_check_in_progress()`) + +2. **Add guard at top of `unregisterProcess()` body** (before any NULL checks): + ```c + void unregisterProcess(FirmwareInterfaceHandle handler) + { + /* Session state validation: reject if checkForUpdate() is active */ + if (internal_is_check_in_progress()) { + FWUPMGR_ERROR("unregisterProcess: REJECTED — checkForUpdate() is in " + "progress. Wait for the callback to fire, then retry " + "unregisterProcess().\n"); + /* Do NOT free(handler): caller still owns it and will need it later */ + return; + } + + /* ... rest of existing function unchanged ... */ + } + ``` + +3. **New function exposed by async engine** (in `rdkFwupdateMgr_async.c`): + ```c + bool internal_is_check_in_progress(void) + { + pthread_mutex_lock(&g_check_in_progress_mutex); + bool result = g_check_in_progress; + pthread_mutex_unlock(&g_check_in_progress_mutex); + return result; + } + ``` + +4. **Declaration in `rdkFwupdateMgr_async_internal.h`:** + ```c + /** + * @brief Query whether a checkForUpdate() operation is currently in progress. + * + * Used by unregisterProcess() to enforce the session-state invariant: + * a client cannot unregister while it has outstanding operations. + * + * Thread-safe: protected by internal mutex. + * + * @return true if a checkForUpdate worker thread is active, false otherwise. + */ + bool internal_is_check_in_progress(void); + ``` + +**Design notes:** +- The coupling is minimal: one `bool` query function. `_process.c` has zero + knowledge of mutexes, threads, or contexts. +- The function is `internal_*` prefixed (library-internal, not exported). +- If the async engine is not initialized (library in bad state), the mutex is + statically initialized (`PTHREAD_MUTEX_INITIALIZER`), so the query is safe + even if `internal_system_init()` hasn't been called. +- The `void` return type of `unregisterProcess()` means we can't return an error + code. The rejection is signaled via a loud `FWUPMGR_ERROR` log. This is + acceptable for Phase 1. A future API revision (Phase 2+) could add a return type. + +### 11.6 `rdkFwupdateMgr_log.c` / `rdkFwupdateMgr_log.h` — NO CHANGES + +### 11.7 `example_app.c` — NO CHANGES + +The example app's callback runs in the worker thread (previously ran in the +persistent BG thread). The condvar signaling in the example works identically. + +--- + +## 12. Unit Test Impact + +### 12.1 Tests that need updating (CheckForUpdate-specific) + +| Test File | Impact | +|-----------|--------| +| `rdkFwupdateMgr_async_cleanup_gtest.cpp` | **REWRITE** — references `rdkFwupdateMgr_async_init_for_test()`, `get_pending_count` (registry-based) | +| `rdkFwupdateMgr_async_refcount_gtest.cpp` | **REWRITE** — likely tests registry slot refcounting | +| `rdkFwupdateMgr_async_signal_gtest.cpp` | **REWRITE** — tests signal dispatch through registry | +| `rdkFwupdateMgr_async_stress_gtest.cpp` | **REWRITE** — uses `g_async_registry`, concurrent registration | +| `rdkFwupdateMgr_async_threadsafety_gtest.cpp` | **REWRITE** — concurrent registration/dispatch | + +### 12.2 Tests that remain unchanged (Download/Update) + +| Test File | Impact | +|-----------|--------| +| `dbus_handlers.cpp` | **UNCHANGED** — tests daemon-side handlers | +| `device_status_helper_gtest.cpp` | **UNCHANGED** | +| `fwdl_interface_gtest.cpp` | **UNCHANGED** | +| `basic_rdkv_main_gtest.cpp` | **UNCHANGED** | +| `rdkfwupdatemgr_main_flow_gtest.cpp` | **UNCHANGED** | +| `rdkFwupdateMgr_handlers_gtest.cpp` | **UNCHANGED** — tests daemon-side | +| `deviceutils/device_api_gtest.cpp` | **UNCHANGED** | +| `deviceutils/deviceutils_gtest.cpp` | **UNCHANGED** | + +### 12.3 New tests needed + +| Test | Description | +|------|-------------| +| `WorkerThread_StartsAndStops` | Verify thread is created on `checkForUpdate()` and exits after signal | +| `WorkerThread_FiresCallback` | Verify callback is invoked with correct FwInfoData | +| `WorkerThread_Timeout` | Verify thread exits cleanly after 120s with no signal | +| `WorkerThread_DBusFailure` | Verify `CHECK_FOR_UPDATE_FAIL` returned when D-Bus is unavailable | +| `DuplicateRequest_Rejected` | Verify second `checkForUpdate()` returns FAIL while first is active | +| `UnregisterDuringCheck_Rejected` | Verify `unregisterProcess()` is rejected (no-op) while `checkForUpdate()` is active. Handle is NOT freed. | +| `UnregisterAfterCallback_Succeeds` | Verify `unregisterProcess()` succeeds after callback fires and `g_check_in_progress` is cleared. | +| `LibraryUnloadDuringCheck` | Verify destructor joins active worker thread | +| `CallbackDataValidity` | Verify FwInfoData fields are correct (version, UpdateDetails, status) | +| `MultiProcess_BothReceiveSignal` | Integration test: two processes, both get callbacks | +| `SIGTERM_DuringCheck_ExitClean` | Verify that calling `exit()` during an active check does not crash or leak (destructor joins thread). | + +--- + +## 13. Resource Cost Comparison + +### 13.1 Memory comparison + +| State | Current Design | New Design | +|-------|---------------|------------| +| Library loaded, no API calls | ~14KB (persistent thread + registries + D-Bus conn) | ~14KB* | +| Library loaded, never calls checkForUpdate() | ~14KB (same) | ~14KB* | +| One checkForUpdate() in progress | ~14KB (same) | ~14KB* + ~10KB (worker) = ~24KB | +| checkForUpdate() completed, idle | ~14KB (thread still alive) | ~14KB* (worker exited) | + +*~14KB is for the persistent BG thread that still runs for Download/Update. +When Download/Update are also migrated to on-demand (Phase 2), this drops to ~0. + +### 13.2 Per-request cost + +| Resource | Size | Duration | +|----------|------|----------| +| `CheckRequestContext` | ~128 bytes | Request lifetime | +| pthread stack | ~8KB (default) | Request lifetime | +| GMainContext | ~1.5KB | Request lifetime | +| GMainLoop | ~200 bytes | Request lifetime | +| D-Bus signal subscription | ~100 bytes | Request lifetime | +| **Total** | **~10KB** | **5s to 2min (daemon response time)** | + +All resources freed to zero after callback fires. + +--- + +## 14. Migration Phases + +### Phase 1 (This Document): CheckForUpdate on-demand thread + +| Step | Task | Effort | Risk | +|------|------|--------|------| +| 1.1 | Add `CheckRequestContext` to `_async_internal.h` | 0.5h | Low | +| 1.2 | Remove CheckForUpdate registry types from `_async_internal.h` | 0.5h | Low | +| 1.3 | Implement `internal_check_worker_thread()` in `_async.c` | 2h | Medium | +| 1.4 | Implement signal handler, timeout handler in `_async.c` | 1h | Medium | +| 1.5 | Implement in-progress guard and active thread tracking in `_async.c` | 1h | Low | +| 1.6 | Remove old CheckForUpdate code from `_async.c` | 1h | Low | +| 1.7 | Remove CheckForUpdateComplete subscription from BG thread | 0.5h | Low | +| 1.8 | Remove g_registry init/cleanup from system_init/deinit | 0.5h | Low | +| 1.9 | Rewrite `checkForUpdate()` in `_api.c` | 1.5h | Medium | +| 1.10 | Update constructor/destructor in `_api.c` | 0.5h | Low | +| 1.11 | Update/rewrite unit tests | 3-4h | High | +| 1.12 | Integration testing (multi-process) | 2h | Medium | +| **Total** | | **~14h (2 days)** | | + +### Phase 2 (Future): DownloadFirmware on-demand thread + +Same pattern but with multi-fire callback (thread stays alive across +multiple `DownloadProgress` signals, exits on COMPLETED/ERROR). + +### Phase 3 (Future): UpdateFirmware on-demand thread + +Same pattern as Download. + +### Phase 4 (Future): Remove persistent background thread entirely + +After Download and Update are migrated, `internal_system_init()` and the +persistent BG thread can be removed entirely. Constructor becomes a true no-op. + +--- + +## 15. Open Items & Future Work + +### 15.1 Resolved in this document + +| Item | Resolution | +|------|-----------| +| Timeout on condvar wait in checkForUpdate() | **No timeout.** Worker setup is fast (~100ms). Plain `pthread_cond_wait()`. | +| Caller returns FAIL but callback fires later | **Cannot happen.** No timeout means caller always waits for worker's answer. | +| Duplicate checkForUpdate() calls | **Rejected** with `CHECK_FOR_UPDATE_FAIL` and log message. | +| Block unregisterProcess() during check | **YES — REVISED (v1.1).** `unregisterProcess()` is rejected (returns immediately with error log) if `checkForUpdate()` is in progress. Caller must wait for callback, then unregister. Rationale: ending a session while operations are outstanding is a semantic contradiction and creates undefined daemon-client state. See §5.4 for full analysis. | +| Dead code in persistent BG thread | **Remove it.** Strip CheckForUpdateComplete subscription and all registry code. | +| handler_id routing in signal | **Not filtered.** Both processes receive broadcast and fire callbacks. This is correct because XConf data is device-global. | + +### 15.2 Items for Phase 2+ + +| Item | Phase | +|------|-------| +| Add `cancelCheckForUpdate()` API for graceful in-flight cancellation | Phase 1.5 | +| Change `unregisterProcess()` return type to `UnregisterResult` enum | Phase 2 | +| Migrate downloadFirmware() to on-demand thread | Phase 2 | +| Migrate updateFirmware() to on-demand thread | Phase 3 | +| Remove persistent BG thread entirely | Phase 4 | +| Remove `internal_system_init()` / `internal_system_deinit()` | Phase 4 | +| Remove `BackgroundThread` struct | Phase 4 | +| Remove `DwnlCallbackRegistry` / `UpdateCbRegistry` | Phase 2-3 | +| Make library constructor a true no-op | Phase 4 | + +### 15.3 Considerations for production hardening + +| Item | Priority | Notes | +|------|----------|-------| +| Log rotation for worker thread logs | Medium | Each worker thread logs to same file — ensure thread-safe logging | +| Configurable timeout | Low | Currently hardcoded to 120s. Could be made configurable via env var or RFC. | +| D-Bus reconnection | Low | If D-Bus daemon restarts, `g_bus_get_sync()` should reconnect. GLib handles this internally for new connections. | +| Memory sanitizer validation | High | Run with AddressSanitizer/ThreadSanitizer to validate no leaks or races | +| Coverity scan | High | Current codebase uses Coverity. New code must pass. | + +--- + +## Appendix A: D-Bus Signal Introspection Reference + +```xml + + + + + + + + + +``` + +GVariant signature: `(tiissss)` + +Parsed by: `internal_parse_signal_data()` in `rdkFwupdateMgr_async.c` + +--- + +## Appendix B: g_bus_get_sync() Singleton Behavior + +`g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error)` returns a **process-wide +singleton** `GDBusConnection`. Multiple calls within the same process return the +same object with an incremented reference count. + +**Implications:** + +- Worker thread's `g_bus_get_sync()` shares the underlying socket FD with any + other GLib code in the process (including the persistent BG thread for + Download/Update). +- `g_object_unref()` in the worker's cleanup decrements the refcount but does NOT + close the connection (other users still hold references). +- Signal subscriptions are per-context: the worker's subscription dispatches to + the worker's `GMainContext`, even though the underlying connection is shared. +- Between separate processes (A and B), the connections are completely independent + (separate socket FDs to the D-Bus daemon). + +--- + +## Appendix C: Complete Ordering Proof + +``` +TIME WORKER THREAD D-BUS DAEMON FIRMWARE DAEMON +──── ───────────── ──────────── ─────────────── + +T1 g_main_context_new() +T2 g_main_loop_new() +T3 g_main_context_push_thread_default() +T4 g_bus_get_sync() → connection +T5 g_dbus_connection_signal_subscribe() (subscription registered + → subscription_id locally in GLib, no + round-trip to D-Bus daemon) + +T6 g_dbus_connection_call(CheckForUpdate) → message queued → received + (NOTE: subscribe at T5 is LOCAL. XConf query starts + The call at T6 goes over the wire. + The subscription is guaranteed to be + active before the call is sent because + both use the same connection object + and GLib processes them in order.) + +T7 pthread_cond_signal(ready) +T8 g_main_loop_run() (waiting for events...) + ↓ blocked in poll() + + XConf query done + Build GVariant +T9 ← emit_signal(broadcast) + → deliver to all subscribers + +T10 poll() returns, GLib dispatches signal +T11 on_check_signal_handler() fires +T12 ctx->callback(&fwinfo_data) +T13 g_main_loop_quit() +T14 g_main_loop_run() returns +T15 g_dbus_connection_signal_unsubscribe() +T16 g_object_unref(connection) +T17 g_main_context_pop_thread_default() +T18 g_main_loop_unref() +T19 g_main_context_unref() +T20 free(ctx) +T21 return NULL → thread exits + +GUARANTEE: Signal at T5 is always registered before method call at T6. + No signal can be missed. +``` diff --git a/docs/TRACKING_CHECKFORUPDATE_REDESIGN.md b/docs/TRACKING_CHECKFORUPDATE_REDESIGN.md new file mode 100755 index 00000000..e69de29b diff --git a/librdkFwupdateMgr/src/rdkFwupdateMgr_api.c b/librdkFwupdateMgr/src/rdkFwupdateMgr_api.c index 261af3ce..f70e7448 100644 --- a/librdkFwupdateMgr/src/rdkFwupdateMgr_api.c +++ b/librdkFwupdateMgr/src/rdkFwupdateMgr_api.c @@ -14,31 +14,24 @@ * @file rdkFwupdateMgr_api.c * @brief Public API implementations: checkForUpdate, downloadFirmware, updateFirmware * - * ALL THREE APIS USE THE SAME ASYNC PATTERN: - * =========================================== - * All APIs are NON-BLOCKING fire-and-forget calls that return immediately. - * Results are delivered asynchronously via D-Bus signals to registered callbacks. - * - * CHECKFORUPDATE: - * --------------- + * CHECKFORUPDATE (Phase 1 - on-demand worker thread): + * ==================================================== * 1. Validate handle and callback - * 2. Register callback in registry (BEFORE D-Bus call to avoid race) - * 3. Fire CheckForUpdate D-Bus method call (fire-and-forget) - * 4. Return CHECK_FOR_UPDATE_SUCCESS immediately - * - * [Later - typically 5-30 seconds] - * Daemon queries XConf server and emits CheckForUpdateComplete signal - * → on_check_complete_signal() fires in background thread - * → dispatch_all_pending() calls registered UpdateEventCallback - * → Callback receives FwInfoData with version info and update details + * 2. Reject if another checkForUpdate is already in progress + * 3. Allocate CheckRequestContext on heap + * 4. Spawn worker thread (internal_check_worker_thread) + * 5. Wait for worker to signal "ready" via condvar (~10-100ms) + * 6. Return SUCCESS or FAIL immediately + * + * [Later - typically 5-30 seconds, max 120 seconds] + * Worker thread receives CheckForUpdateComplete signal from daemon + * → Parses payload → Fires client callback with FwInfoData + * → Cleans up all resources → Thread exits * - * DOWNLOAD / UPDATE FIRMWARE: - * ============================ - * Same pattern but with progress signals: - * - DownloadFirmware → DownloadProgress signals (multiple, 0%-100%) - * - UpdateFirmware → UpdateProgress signals (multiple, 0%-100%) - * - * Callbacks fire repeatedly until COMPLETED or ERROR status. + * DOWNLOAD / UPDATE FIRMWARE (unchanged — persistent BG thread): + * =============================================================== + * Same fire-and-forget pattern as before. + * Callbacks registered in registry, dispatched from background thread. */ #include "rdkFwupdateMgr_client.h" @@ -49,24 +42,37 @@ #include #include +/* ---- Extern references to CheckForUpdate on-demand thread state ---- + * + * These live in rdkFwupdateMgr_async.c. We access them here to: + * (1) check/set g_check_in_progress - enforce one-at-a-time per process + * (2) track g_active_check_ctx - so the destructor can cancel/join the worker + * + * All access is protected by g_check_in_progress_mutex. + */ +extern pthread_mutex_t g_check_in_progress_mutex; +extern bool g_check_in_progress; +extern CheckRequestContext *g_active_check_ctx; + /* ======================================================================== - * checkForUpdate — SYNCHRONOUS implementation + * checkForUpdate - ON-DEMAND WORKER THREAD implementation (Phase 1) * ======================================================================== */ /** - * @brief Check for firmware update — non-blocking, returns immediately + * @brief Check for firmware update - spawns on-demand worker thread * - * Sends CheckForUpdate(handle) to the daemon and returns immediately. - * The daemon will query the XConf server in the background (5-30 seconds) - * and emit a CheckForUpdateComplete signal when done. + * Allocates a CheckRequestContext, spawns a worker thread that connects + * to D-Bus, subscribes to CheckForUpdateComplete signal, sends the + * CheckForUpdate method call, and waits for the response. The caller + * blocks briefly (typically <100ms) until the worker signals "ready", + * then returns immediately. The callback fires asynchronously in the + * worker thread when the daemon responds (5s to 2min+). * - * The callback fires ONCE when the signal arrives with complete firmware info: - * - FwInfoData.status: FIRMWARE_AVAILABLE, FIRMWARE_NOT_AVAILABLE, etc. - * - FwInfoData.CurrFWVersion: Current firmware version - * - FwInfoData.UpdateDetails: Details about available update (if any) - * - * The callback is registered in the async registry before sending the D-Bus call - * to ensure the signal doesn't arrive before we're ready to receive it. + * INVARIANTS: + * - At most one checkForUpdate() in progress per process + * - Callback fires exactly once (on signal) or zero times (on timeout/error) + * - Worker thread is self-contained: creates and destroys all its resources + * - No interaction with the persistent background thread * * @param handle Valid FirmwareInterfaceHandle from registerProcess() * @param callback Invoked when CheckForUpdateComplete signal arrives @@ -75,11 +81,13 @@ CheckForUpdateResult checkForUpdate(FirmwareInterfaceHandle handle, UpdateEventCallback callback) { - /* [1] Validate */ + /* [1] Validate handle - must be non-NULL and non-empty (daemon would reject it anyway) */ if (handle == NULL || handle[0] == '\0') { FWUPMGR_ERROR("checkForUpdate: invalid handle (NULL or empty)\n"); return CHECK_FOR_UPDATE_FAIL; } + + /* [2] Validate callback - NULL callback means we'd have no way to deliver results */ if (callback == NULL) { FWUPMGR_ERROR("checkForUpdate: callback is NULL\n"); return CHECK_FOR_UPDATE_FAIL; @@ -87,66 +95,146 @@ CheckForUpdateResult checkForUpdate(FirmwareInterfaceHandle handle, FWUPMGR_INFO("checkForUpdate: handle='%s'\n", handle); - /* [2] Connect to D-Bus FIRST before registering callback + /* [3] Reject duplicate: only one checkForUpdate at a time per process. * - * This prevents stale registry entries if D-Bus connection fails. - * We only register the callback if we can successfully send the request. + * If a worker thread is already running, a second checkForUpdate() + * would create two threads both listening for the same D-Bus signal. + * wasteful and confusing (the app would get duplicate callbacks with + * identical data). So we reject it immediately. */ - GError *error = NULL; - GDBusConnection *conn = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error); - - if (conn == NULL) { - FWUPMGR_ERROR("checkForUpdate: D-Bus connect failed: %s\n", - error ? error->message : "unknown"); - if (error) g_error_free(error); + pthread_mutex_lock(&g_check_in_progress_mutex); + if (g_check_in_progress) { + pthread_mutex_unlock(&g_check_in_progress_mutex); + FWUPMGR_WARN("checkForUpdate: already in progress, rejecting. " + "handle='%s'\n", handle); return CHECK_FOR_UPDATE_FAIL; } + g_check_in_progress = true; + pthread_mutex_unlock(&g_check_in_progress_mutex); - /* [3] Register callback AFTER D-Bus connection succeeds + /* [4] Allocate per-request context on heap * - * Register immediately before sending to avoid race condition where - * the daemon responds before we're ready to receive the signal. + * The context struct holds everything the worker thread needs: + * the handle, callback pointer, condvar for handshake, and GLib objects. + * It's heap-allocated so it survives after checkForUpdate() returns. + * Ownership transfers to the worker thread after the condvar handshake. */ - if (!internal_register_callback(handle, callback)) { - FWUPMGR_ERROR("checkForUpdate: registry full, handle='%s'\n", handle); - g_object_unref(conn); + CheckRequestContext *ctx = calloc(1, sizeof(CheckRequestContext)); + if (ctx == NULL) { + FWUPMGR_ERROR("checkForUpdate: calloc failed for ctx\n"); + pthread_mutex_lock(&g_check_in_progress_mutex); + g_check_in_progress = false; + pthread_mutex_unlock(&g_check_in_progress_mutex); + return CHECK_FOR_UPDATE_FAIL; + } + + ctx->handle_key = strdup(handle); + if (ctx->handle_key == NULL) { + FWUPMGR_ERROR("checkForUpdate: strdup failed for handle\n"); + free(ctx); + pthread_mutex_lock(&g_check_in_progress_mutex); + g_check_in_progress = false; + pthread_mutex_unlock(&g_check_in_progress_mutex); return CHECK_FOR_UPDATE_FAIL; } - /* [4] Fire-and-forget D-Bus CheckForUpdate method call + ctx->callback = callback; + ctx->is_ready = false; + ctx->init_failed = false; + + if (pthread_mutex_init(&ctx->ready_mutex, NULL) != 0) { + FWUPMGR_ERROR("checkForUpdate: ready_mutex init failed\n"); + free(ctx->handle_key); + free(ctx); + pthread_mutex_lock(&g_check_in_progress_mutex); + g_check_in_progress = false; + pthread_mutex_unlock(&g_check_in_progress_mutex); + return CHECK_FOR_UPDATE_FAIL; + } + + if (pthread_cond_init(&ctx->ready_cond, NULL) != 0) { + FWUPMGR_ERROR("checkForUpdate: ready_cond init failed\n"); + pthread_mutex_destroy(&ctx->ready_mutex); + free(ctx->handle_key); + free(ctx); + pthread_mutex_lock(&g_check_in_progress_mutex); + g_check_in_progress = false; + pthread_mutex_unlock(&g_check_in_progress_mutex); + return CHECK_FOR_UPDATE_FAIL; + } + + /* [6] Track context for library-unload safety * - * Arguments: (s) - * s handle — identifies this app to the daemon + * Store the ctx pointer in g_active_check_ctx so the library + * destructor can find and cancel/join the worker thread. Without this, + * dlclose() would unmap our code while the worker is still running - might lead to crash. + */ + pthread_mutex_lock(&g_check_in_progress_mutex); + g_active_check_ctx = ctx; + pthread_mutex_unlock(&g_check_in_progress_mutex); + + /* [7] Spawn worker thread - ownership of ctx transfers to worker * - * Three trailing NULLs = fire and forget (no reply waited for). - * g_dbus_connection_call() returns immediately. - * Daemon will emit CheckForUpdateComplete signal when XConf query finishes. + * The worker thread will set up D-Bus, subscribe to signals, + * send the CheckForUpdate request, and wait for the daemon's response. + * If pthread_create fails, we undo everything and return FAIL. */ - FWUPMGR_INFO("checkForUpdate: calling CheckForUpdate on daemon, handle='%s'\n", - handle); + if (pthread_create(&ctx->thread, NULL, internal_check_worker_thread, ctx) != 0) { + FWUPMGR_ERROR("checkForUpdate: pthread_create failed\n"); + pthread_mutex_lock(&g_check_in_progress_mutex); + g_check_in_progress = false; + g_active_check_ctx = NULL; + pthread_mutex_unlock(&g_check_in_progress_mutex); + pthread_cond_destroy(&ctx->ready_cond); + pthread_mutex_destroy(&ctx->ready_mutex); + free(ctx->handle_key); + free(ctx); + return CHECK_FOR_UPDATE_FAIL; + } - g_dbus_connection_call( - conn, - DBUS_SERVICE_NAME, - DBUS_OBJECT_PATH, - DBUS_INTERFACE_NAME, - DBUS_METHOD_CHECK, /* method: CheckForUpdate */ - g_variant_new("(s)", handle), /* app's handler_id string */ - NULL, /* expected reply type: none */ - G_DBUS_CALL_FLAGS_NONE, - DBUS_TIMEOUT_MS, - NULL, /* GCancellable: none */ - NULL, /* reply callback: none */ - NULL /* user_data: none */ - ); + /* [8] Wait for worker to signal ready (no timeout — see §5.1) + * + * This blocks the caller for ~10-100ms while the worker sets up + * its D-Bus connection and signal subscription. The worker signals + * is_ready=true when it's either ready or has failed to init. + */ + pthread_mutex_lock(&ctx->ready_mutex); + while (!ctx->is_ready) { + pthread_cond_wait(&ctx->ready_cond, &ctx->ready_mutex); + } + bool failed = ctx->init_failed; + pthread_mutex_unlock(&ctx->ready_mutex); - g_object_unref(conn); + /* [9] Check if worker failed to initialize + * + * The worker tried to connect to D-Bus and subscribe to signals. + * If that failed (D-Bus dead, system error), init_failed is true. + * We join the worker (it's already exiting) and return FAIL to the app. + * The worker handles its own cleanup - we just wait for it to finish. + */ + if (failed) { + FWUPMGR_ERROR("checkForUpdate: worker thread failed to initialize. " + "handle='%s'\n", handle); + /* + * Worker thread will clean itself up (free ctx, reset g_check_in_progress). + * We just need to join it to avoid a zombie thread. + * But the worker signals ready BEFORE going to cleanup, so we must + * wait for it to actually exit. + */ + pthread_join(ctx->thread, NULL); + return CHECK_FOR_UPDATE_FAIL; + } - FWUPMGR_INFO("checkForUpdate: D-Bus call sent, returning SUCCESS. " - "Callback will fire when CheckForUpdateComplete signal arrives. " - "handle='%s'\n", handle); + /* [10] Worker is running and listening. Return success to caller. + * + * From this point, the caller NEVER touches ctx again. + * The worker thread is the sole owner and will free it after + * the callback fires or the timeout expires. + */ + FWUPMGR_INFO("checkForUpdate: worker thread started, returning SUCCESS. " + "Callback will fire when daemon responds. handle='%s'\n", + handle); - /* [5] Return immediately — app is unblocked */ return CHECK_FOR_UPDATE_SUCCESS; } @@ -173,12 +261,23 @@ static void rdkFwupdateMgr_lib_init(void) /** * @brief Library destructor — auto-called when .so is unloaded * - * Stops background thread and frees all resources cleanly. + * Stops any active checkForUpdate worker thread, then stops the + * persistent background thread and frees all resources cleanly. */ __attribute__((destructor)) static void rdkFwupdateMgr_lib_deinit(void) { FWUPMGR_INFO("=== rdkFwupdateMgr library unloading ===\n"); + + /* Cancel and join any active CheckForUpdate worker thread first. + * + * TL;DR: Must happen BEFORE internal_system_deinit() because the worker + * may be using the shared D-Bus connection. If we tore down the BG thread + * first, the worker could be left with a dangling connection reference. + * Order: (1) stop worker → (2) stop BG thread → (3) free resources. + */ + internal_cancel_all_active_check_threads(); + internal_system_deinit(); FWUPMGR_INFO("=== rdkFwupdateMgr library unloaded ===\n"); } diff --git a/librdkFwupdateMgr/src/rdkFwupdateMgr_async.c b/librdkFwupdateMgr/src/rdkFwupdateMgr_async.c index f1ffcc46..50723d78 100644 --- a/librdkFwupdateMgr/src/rdkFwupdateMgr_async.c +++ b/librdkFwupdateMgr/src/rdkFwupdateMgr_async.c @@ -12,13 +12,21 @@ /** * @file rdkFwupdateMgr_async.c - * @brief Internal engine: registry, background thread, signal dispatch + * @brief Internal engine: CheckForUpdate worker thread, Download/Update registries, + * background thread, signal dispatch * - * Owns: - * - Global callback registry (one slot per pending checkForUpdate call) - * - Background GLib event loop thread - * - D-Bus signal subscription and handler - * - Dispatch: signal arrives → find all PENDING → fire each callback + * PHASE 1 ARCHITECTURE: + * + * CheckForUpdate — ON-DEMAND WORKER THREAD: + * - internal_check_worker_thread(): spawned per checkForUpdate() call + * - on_check_signal_handler(): fires client callback directly + * - on_check_timeout(): 120s safety net + * - internal_is_check_in_progress(): query for session-state enforcement + * - internal_cancel_all_active_check_threads(): destructor cleanup + * + * Download / Update — PERSISTENT BG THREAD (unchanged): + * - background_thread_func(): subscribes to DownloadProgress + UpdateProgress + * - Registry-based dispatch (dispatch_all_dwnl_active, dispatch_all_update_active) * * Apps never interact with this file directly. * All entry points are through rdkFwupdateMgr_api.c. @@ -37,25 +45,22 @@ * GLOBAL STATE * ======================================================================== */ -static CallbackRegistry g_registry; static BackgroundThread g_bg_thread; static DwnlCallbackRegistry g_dwnl_registry; static UpdateCbRegistry g_update_registry; +/* ---- CheckForUpdate on-demand thread state ---- */ +/* Non-static: accessed by rdkFwupdateMgr_api.c via extern declarations */ +pthread_mutex_t g_check_in_progress_mutex = PTHREAD_MUTEX_INITIALIZER; +bool g_check_in_progress = false; +CheckRequestContext *g_active_check_ctx = NULL; + /* ======================================================================== * FORWARD DECLARATIONS * ======================================================================== */ static void *background_thread_func(void *arg); -static void on_check_complete_signal(GDBusConnection *conn, - const gchar *sender, - const gchar *object_path, - const gchar *interface_name, - const gchar *signal_name, - GVariant *parameters, - gpointer user_data); - static void on_download_progress_signal(GDBusConnection *conn, const gchar *sender, const gchar *object_path, @@ -72,11 +77,19 @@ static void on_update_progress_signal(GDBusConnection *conn, GVariant *parameters, gpointer user_data); -static void dispatch_all_pending(const InternalSignalData *signal_data); -static void registry_reset_slot(CallbackEntry *entry); static bool parse_update_details(const char *update_details_str, UpdateDetails *out_details); +/* Forward declarations — CheckForUpdate on-demand worker thread */ +static void on_check_signal_handler(GDBusConnection *conn, + const gchar *sender, + const gchar *object_path, + const gchar *interface_name, + const gchar *signal_name, + GVariant *parameters, + gpointer user_data); +static gboolean on_check_timeout(gpointer user_data); + /* Forward declaration for download status mapping function */ static DownloadStatus map_dwnl_status_string(const char *status_str); @@ -102,19 +115,10 @@ int internal_system_init(void) { FWUPMGR_INFO("internal_system_init: begin\n"); - /* Registry */ - memset(&g_registry, 0, sizeof(g_registry)); - if (pthread_mutex_init(&g_registry.mutex, NULL) != 0) { - FWUPMGR_ERROR("internal_system_init: registry mutex init failed\n"); - return -1; - } - g_registry.initialized = true; - - /* Background thread state */ + /* Background thread state (for Download/Update signals only) */ memset(&g_bg_thread, 0, sizeof(g_bg_thread)); if (pthread_mutex_init(&g_bg_thread.mutex, NULL) != 0) { FWUPMGR_ERROR("internal_system_init: bg thread mutex init failed\n"); - pthread_mutex_destroy(&g_registry.mutex); return -1; } @@ -131,7 +135,6 @@ int internal_system_init(void) g_main_loop_unref(g_bg_thread.main_loop); g_main_context_unref(g_bg_thread.context); pthread_mutex_destroy(&g_bg_thread.mutex); - pthread_mutex_destroy(&g_registry.mutex); return -1; } @@ -199,17 +202,6 @@ void internal_system_deinit(void) internal_dwnl_system_deinit(); internal_update_system_deinit(); - /* Free any leftover handle_key strings from CheckForUpdate registry */ - pthread_mutex_lock(&g_registry.mutex); - for (int i = 0; i < MAX_PENDING_CALLBACKS; i++) { - if (g_registry.entries[i].handle_key != NULL) { - free(g_registry.entries[i].handle_key); - g_registry.entries[i].handle_key = NULL; - } - } - pthread_mutex_unlock(&g_registry.mutex); - pthread_mutex_destroy(&g_registry.mutex); - FWUPMGR_INFO("internal_system_deinit: done\n"); } @@ -221,10 +213,11 @@ void internal_system_deinit(void) * @brief Background thread entry point * * Runs for the lifetime of the library. + * Handles Download/Update signals only (CheckForUpdate uses on-demand worker). * * 1. Push isolated GLib context for this thread * 2. Connect to system D-Bus - * 3. Subscribe to CheckForUpdateComplete signal + * 3. Subscribe to DownloadProgress and UpdateProgress signals * 4. Signal main thread: ready * 5. g_main_loop_run() — blocks until deinit calls g_main_loop_quit() * 6. Cleanup: unsubscribe, release connection, pop context @@ -245,36 +238,13 @@ static void *background_thread_func(void *arg) goto thread_exit; } - /* - * Subscribe to the CheckForUpdateComplete signal. - * - * sender = NULL → accept from any sender - * (daemon's well-known name may vary by deployment) - * arg0 = NULL → no filter on first argument - * - * GLib calls on_check_complete_signal() in THIS thread's context - * whenever the signal arrives. - */ - g_bg_thread.subscription_id = g_dbus_connection_signal_subscribe( - g_bg_thread.connection, - NULL, /* sender: any */ - DBUS_INTERFACE_NAME, /* interface */ - DBUS_SIGNAL_COMPLETE, /* signal: CheckForUpdateComplete */ - DBUS_OBJECT_PATH, /* object path */ - NULL, /* arg0 filter: none */ - G_DBUS_SIGNAL_FLAGS_NONE, - on_check_complete_signal, /* handler */ - NULL, /* user_data: not needed (use globals)*/ - NULL /* user_data destroy notify */ - ); - - FWUPMGR_INFO("background_thread: subscribed to CheckForUpdateComplete (id=%u)\n", - g_bg_thread.subscription_id); - /* * Subscribe to DownloadProgress and UpdateProgress signals. - * Must be done HERE in the background thread, not from main thread, - * because the connection belongs to this thread's GMainContext. + * + * TL;DR: The BG thread ONLY handles Download and Update signals now. + * CheckForUpdateComplete is handled by the on-demand worker thread (Phase 1). + * Previously, this thread also subscribed to CheckForUpdateComplete and + * used a registry to dispatch it — that code has been removed. */ guint dwnl_sub_id = g_dbus_connection_signal_subscribe( g_bg_thread.connection, @@ -313,10 +283,19 @@ static void *background_thread_func(void *arg) g_main_loop_run(g_bg_thread.main_loop); FWUPMGR_INFO("background_thread: event loop exited\n"); - if (g_bg_thread.subscription_id != 0) { - g_dbus_connection_signal_unsubscribe(g_bg_thread.connection, - g_bg_thread.subscription_id); + /* Unsubscribe from signals before releasing connection + * + * TL;DR: Must unsubscribe BEFORE g_object_unref(connection). If we unref + * first, the subscription callback could fire on a freed connection → crash. + * Order matters: unsubscribe → unref → pop context. + */ + if (dwnl_sub_id != 0) { + g_dbus_connection_signal_unsubscribe(g_bg_thread.connection, dwnl_sub_id); + } + if (update_sub_id != 0) { + g_dbus_connection_signal_unsubscribe(g_bg_thread.connection, update_sub_id); } + g_object_unref(g_bg_thread.connection); g_bg_thread.connection = NULL; @@ -327,253 +306,360 @@ static void *background_thread_func(void *arg) } /* ======================================================================== - * D-BUS SIGNAL HANDLER + * CHECKFORUPDATE — ON-DEMAND WORKER THREAD ENGINE (Phase 1) + * ======================================================================== + * + * Replaces the old registry-based signal dispatch for CheckForUpdate. + * Each checkForUpdate() call spawns a short-lived worker thread that: + * 1. Creates isolated GLib event loop + * 2. Subscribes to CheckForUpdateComplete signal + * 3. Sends CheckForUpdate D-Bus method call + * 4. Waits for signal (with 120s timeout) + * 5. Fires client callback directly + * 6. Cleans up and exits + * + * At most ONE worker thread per process (enforced by g_check_in_progress). * ======================================================================== */ /** - * @brief Called by GLib when CheckForUpdateComplete signal arrives + * @brief Query whether a checkForUpdate() is currently in progress. * - * Runs in the background thread context. + * Thread-safe: protected by g_check_in_progress_mutex. + */ +bool internal_is_check_in_progress(void) +{ + pthread_mutex_lock(&g_check_in_progress_mutex); + bool result = g_check_in_progress; + pthread_mutex_unlock(&g_check_in_progress_mutex); + return result; +} + +/** + * @brief Cancel all active checkForUpdate worker threads and join them. * - * 1. Parse GVariant payload → InternalSignalData - * 2. Dispatch to all PENDING registry entries - * 3. Free parsed signal data + * Called from library destructor. Quits the worker's event loop so it + * exits cleanly, then joins the thread to ensure no code is executing + * in library memory when dlclose() unmaps us. */ -static void on_check_complete_signal(GDBusConnection *conn, - const gchar *sender, - const gchar *object_path, - const gchar *interface_name, - const gchar *signal_name, - GVariant *parameters, - gpointer user_data) +void internal_cancel_all_active_check_threads(void) { - (void)conn; (void)sender; (void)object_path; - (void)interface_name; (void)signal_name; (void)user_data; + pthread_mutex_lock(&g_check_in_progress_mutex); + CheckRequestContext *ctx = g_active_check_ctx; + pthread_mutex_unlock(&g_check_in_progress_mutex); - FWUPMGR_INFO("on_check_complete_signal: received\n"); + if (ctx == NULL) { + FWUPMGR_INFO("internal_cancel_all_active_check_threads: no active worker\n"); + return; + } - InternalSignalData signal_data; - memset(&signal_data, 0, sizeof(signal_data)); + FWUPMGR_INFO("internal_cancel_all_active_check_threads: " + "stopping active worker thread\n"); - if (!internal_parse_signal_data(parameters, &signal_data)) { - FWUPMGR_ERROR("on_check_complete_signal: parse failed\n"); - return; + /* Quit the worker's event loop — this causes g_main_loop_run() to return */ + if (ctx->main_loop != NULL) { + g_main_loop_quit(ctx->main_loop); } - dispatch_all_pending(&signal_data); + /* Wait for worker thread to finish cleanup and exit */ + pthread_join(ctx->thread, NULL); - internal_cleanup_signal_data(&signal_data); + FWUPMGR_INFO("internal_cancel_all_active_check_threads: " + "worker thread joined\n"); } /** - * @brief Dispatch signal result to every PENDING callback - * - * TWO-PHASE DESIGN — avoids deadlock: - * - * PHASE 1 (mutex held): - * Scan registry → snapshot all PENDING entries into local array. - * Mark each found entry as DISPATCHED. - * Release mutex. + * @brief Timeout handler for the worker thread's GMainLoop. * - * PHASE 2 (mutex released): - * Build FwUpdateEventData from signal_data. - * Invoke each snapshot callback: callback(handle, &event_data) - * Re-acquire mutex briefly to reset each slot to IDLE. + * Fires after CHECK_SIGNAL_TIMEOUT_SECONDS if the daemon never sends + * the CheckForUpdateComplete signal. Quits the event loop so the worker + * can proceed to cleanup. * - * WHY RELEASE BEFORE CALLING CALLBACKS? - * If a callback called checkForUpdate() again, it would call - * internal_register_callback() which tries to lock the same mutex - * → deadlock. Releasing first makes re-entrant use safe. - * - * @param signal_data Parsed signal payload (shared across all callbacks) + * @param user_data CheckRequestContext* (NOT freed here — worker does it) + * @return G_SOURCE_REMOVE (fire once only) */ -static void dispatch_all_pending(const InternalSignalData *signal_data) +static gboolean on_check_timeout(gpointer user_data) { - /* Local snapshot — avoids holding mutex during callback invocations */ - typedef struct { - UpdateEventCallback callback; - char handle_copy[256]; - int slot_index; - } Snapshot; + CheckRequestContext *ctx = (CheckRequestContext *)user_data; - Snapshot snapshots[MAX_PENDING_CALLBACKS]; - int count = 0; + FWUPMGR_WARN("on_check_timeout: %ds timeout expired, " + "daemon did not respond. handle='%s'\n", + CHECK_SIGNAL_TIMEOUT_SECONDS, + ctx->handle_key ? ctx->handle_key : "(null)"); - /* ---- PHASE 1: collect under mutex ---- */ - pthread_mutex_lock(&g_registry.mutex); - - for (int i = 0; i < MAX_PENDING_CALLBACKS; i++) { - CallbackEntry *e = &g_registry.entries[i]; - if (e->state != CB_STATE_PENDING) continue; + if (ctx->main_loop != NULL) { + g_main_loop_quit(ctx->main_loop); + } - snapshots[count].callback = e->callback; - snapshots[count].slot_index = i; - snprintf(snapshots[count].handle_copy, - sizeof(snapshots[count].handle_copy), - "%s", e->handle_key ? e->handle_key : ""); + return G_SOURCE_REMOVE; +} - e->state = CB_STATE_DISPATCHED; - count++; +/** + * @brief Signal handler for CheckForUpdateComplete — fires client callback. + * + * Called by GLib in the worker thread's GMainContext when the daemon emits + * the CheckForUpdateComplete signal. Parses the payload, builds FwInfoData, + * invokes the client callback, then quits the event loop. + * + * @param user_data CheckRequestContext* (NOT freed here — worker does it) + */ +static void on_check_signal_handler(GDBusConnection *conn, + const gchar *sender, + const gchar *object_path, + const gchar *interface_name, + const gchar *signal_name, + GVariant *parameters, + gpointer user_data) +{ + (void)conn; (void)sender; (void)object_path; + (void)interface_name; (void)signal_name; - FWUPMGR_INFO("dispatch_all_pending: queued handle='%s'\n", - e->handle_key ? e->handle_key : "(null)"); - } + CheckRequestContext *ctx = (CheckRequestContext *)user_data; - pthread_mutex_unlock(&g_registry.mutex); + FWUPMGR_INFO("on_check_signal_handler: received CheckForUpdateComplete " + "for handle='%s'\n", + ctx->handle_key ? ctx->handle_key : "(null)"); - FWUPMGR_INFO("dispatch_all_pending: %d callback(s) to fire\n", count); + /* Parse signal payload */ + InternalSignalData signal_data; + memset(&signal_data, 0, sizeof(signal_data)); - /* ---- PHASE 2: invoke callbacks, no mutex held ---- */ + if (!internal_parse_signal_data(parameters, &signal_data)) { + FWUPMGR_ERROR("on_check_signal_handler: parse failed\n"); + /* Quit loop even on parse failure — don't hang forever */ + if (ctx->main_loop != NULL) { + g_main_loop_quit(ctx->main_loop); + } + return; + } - CheckForUpdateStatus status = internal_map_status_code(signal_data->status_code); + /* Build FwInfoData for the callback */ + CheckForUpdateStatus status = internal_map_status_code(signal_data.status_code); - /* - * Build FwInfoData with UpdateDetails for the callback. - * This matches the public API signature: UpdateEventCallback(const FwInfoData*) - * - * MEMORY MANAGEMENT: - * - FwInfoData is stack-allocated (valid during callback invocations) - * - CurrFWVersion is copied from signal_data (array, not pointer) - * - UpdateDetails is stack-allocated if needed - * - All data valid until end of this function - */ FwInfoData fwinfo_data; memset(&fwinfo_data, 0, sizeof(fwinfo_data)); /* Copy current firmware version */ - if (signal_data->current_version) { - strncpy(fwinfo_data.CurrFWVersion, signal_data->current_version, + if (signal_data.current_version) { + strncpy(fwinfo_data.CurrFWVersion, signal_data.current_version, sizeof(fwinfo_data.CurrFWVersion) - 1); fwinfo_data.CurrFWVersion[sizeof(fwinfo_data.CurrFWVersion) - 1] = '\0'; } - /* Set status */ fwinfo_data.status = status; - /* Parse and populate UpdateDetails if firmware is available */ + /* Parse UpdateDetails if firmware is available */ UpdateDetails update_details; - if (status == FIRMWARE_AVAILABLE && signal_data->update_details) { + if (status == FIRMWARE_AVAILABLE && signal_data.update_details) { memset(&update_details, 0, sizeof(update_details)); - - if (parse_update_details(signal_data->update_details, &update_details)) { - /* Point FwInfoData to our stack-allocated UpdateDetails */ + + if (parse_update_details(signal_data.update_details, &update_details)) { fwinfo_data.UpdateDetails = &update_details; - - FWUPMGR_INFO("dispatch_all_pending: UpdateDetails populated\n"); - FWUPMGR_INFO(" FwFileName: %s\n", update_details.FwFileName); - FWUPMGR_INFO(" FwVersion: %s\n", update_details.FwVersion); + FWUPMGR_INFO("on_check_signal_handler: UpdateDetails populated\n"); } else { - /* Parse failed - set to NULL to indicate no details available */ fwinfo_data.UpdateDetails = NULL; - FWUPMGR_ERROR("dispatch_all_pending: parse_update_details failed\n"); + FWUPMGR_ERROR("on_check_signal_handler: parse_update_details failed\n"); } } else { - /* Status is not FIRMWARE_AVAILABLE or no update_details string */ fwinfo_data.UpdateDetails = NULL; } - /* Invoke all callbacks with the same FwInfoData */ - for (int i = 0; i < count; i++) { - Snapshot *s = &snapshots[i]; + /* Fire the client's callback + * + * TL;DR: This is THE moment — deliver the firmware check result to the app. + * The callback runs in the worker thread, NOT the app's main thread. + * After this call returns, we quit the event loop and clean up. + */ + FWUPMGR_INFO("on_check_signal_handler: invoking callback for handle='%s'\n", + ctx->handle_key ? ctx->handle_key : "(null)"); - FWUPMGR_INFO("dispatch_all_pending: invoking callback for handle='%s'\n", - s->handle_copy); + ctx->callback(&fwinfo_data); - /* - * Invoke callback with proper signature: - * UpdateEventCallback(const FwInfoData *fwinfodata) - * - * handle_copy is passed but callback signature doesn't use it anymore. - * We pass it to maintain compatibility with 2-param callbacks if needed. - */ - s->callback(&fwinfo_data); + FWUPMGR_INFO("on_check_signal_handler: callback returned\n"); + + /* Cleanup parsed signal data — free strdup'd strings */ + internal_cleanup_signal_data(&signal_data); - /* Reset slot to IDLE */ - pthread_mutex_lock(&g_registry.mutex); - registry_reset_slot(&g_registry.entries[s->slot_index]); - pthread_mutex_unlock(&g_registry.mutex); + /* Quit the event loop — worker proceeds to cleanup. + * TL;DR: Break out of g_main_loop_run() in the worker thread. */ + if (ctx->main_loop != NULL) { + g_main_loop_quit(ctx->main_loop); } } -/* ======================================================================== - * REGISTRY OPERATIONS - * ======================================================================== */ - /** - * @brief Register a pending callback keyed by handle (no user_data) + * @brief Worker thread entry point for on-demand CheckForUpdate. * - * SAME HANDLE TWICE: - * If the same handle is still PENDING from a previous call, its slot - * is overwritten. Prevents ghost callbacks accumulating. + * LIFECYCLE: + * [A-C] Create isolated GLib event loop + * [D] Connect to D-Bus + * [E] Subscribe to CheckForUpdateComplete signal + * [F] Send CheckForUpdate D-Bus method call to daemon + * [G] Add 120s timeout source + * [H] Signal caller "ready" via condvar + * [I] g_main_loop_run() — wait for signal or timeout + * [J-K] Signal arrives → handler fires callback → loop quits + * [L] Cleanup: unsubscribe, unref GLib objects, free ctx + * [M] Thread exits * - * @param handle App's FirmwareInterfaceHandle (will be strdup'd) - * @param callback App's 2-param UpdateEventCallback - * @return true on success, false if registry is full + * OWNERSHIP: After condvar handshake, this thread solely owns ctx. + * Caller never touches ctx again. + * + * @param arg CheckRequestContext* (ownership transferred) + * @return NULL */ -bool internal_register_callback(FirmwareInterfaceHandle handle, - UpdateEventCallback callback) +void *internal_check_worker_thread(void *arg) { - pthread_mutex_lock(&g_registry.mutex); - - CallbackEntry *free_slot = NULL; - CallbackEntry *existing_slot = NULL; - - for (int i = 0; i < MAX_PENDING_CALLBACKS; i++) { - CallbackEntry *e = &g_registry.entries[i]; + CheckRequestContext *ctx = (CheckRequestContext *)arg; + GError *error = NULL; + GSource *timeout_source = NULL; - /* Existing pending entry for same handle → overwrite it */ - if (e->state == CB_STATE_PENDING && - e->handle_key != NULL && - strcmp(e->handle_key, handle) == 0) { - existing_slot = e; - break; - } + FWUPMGR_INFO("check_worker: starting for handle='%s'\n", + ctx->handle_key ? ctx->handle_key : "(null)"); - if (free_slot == NULL && e->state == CB_STATE_IDLE) { - free_slot = e; - } + /* [A] Create isolated GMainContext for this thread */ + ctx->context = g_main_context_new(); + if (ctx->context == NULL) { + FWUPMGR_ERROR("check_worker: g_main_context_new failed\n"); + goto init_failed; } - CallbackEntry *target = existing_slot ? existing_slot : free_slot; - - if (target == NULL) { - FWUPMGR_ERROR("internal_register_callback: registry full (max=%d)\n", - MAX_PENDING_CALLBACKS); - pthread_mutex_unlock(&g_registry.mutex); - return false; + /* [B] Create GMainLoop bound to our context */ + ctx->main_loop = g_main_loop_new(ctx->context, FALSE); + if (ctx->main_loop == NULL) { + FWUPMGR_ERROR("check_worker: g_main_loop_new failed\n"); + goto init_failed; } - if (existing_slot) { - FWUPMGR_INFO("internal_register_callback: overwriting existing for handle='%s'\n", - handle); - free(target->handle_key); - target->handle_key = NULL; + /* [C] Push as this thread's default context */ + g_main_context_push_thread_default(ctx->context); + + /* [D] Connect to D-Bus */ + ctx->connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error); + if (ctx->connection == NULL) { + FWUPMGR_ERROR("check_worker: D-Bus connect failed: %s\n", + error ? error->message : "unknown"); + if (error) g_error_free(error); + error = NULL; + goto init_failed_with_context; } - target->handle_key = strdup(handle); - target->callback = callback; - target->state = CB_STATE_PENDING; - target->registered_time = time(NULL); + /* [E] Subscribe to CheckForUpdateComplete signal */ + ctx->subscription_id = g_dbus_connection_signal_subscribe( + ctx->connection, + NULL, /* sender: any */ + DBUS_INTERFACE_NAME, /* interface */ + DBUS_SIGNAL_COMPLETE, /* signal: CheckForUpdateComplete */ + DBUS_OBJECT_PATH, /* object path */ + NULL, /* arg0 filter: none */ + G_DBUS_SIGNAL_FLAGS_NONE, + on_check_signal_handler, /* handler */ + ctx, /* user_data: per-request context */ + NULL /* user_data destroy notify */ + ); - pthread_mutex_unlock(&g_registry.mutex); + if (ctx->subscription_id == 0) { + FWUPMGR_ERROR("check_worker: signal subscribe failed\n"); + goto init_failed_with_connection; + } - FWUPMGR_INFO("internal_register_callback: registered handle='%s'\n", handle); - return true; -} + FWUPMGR_INFO("check_worker: subscribed to CheckForUpdateComplete (id=%u)\n", + ctx->subscription_id); + + /* [F] Send CheckForUpdate D-Bus method call (fire-and-forget) */ + FWUPMGR_INFO("check_worker: calling CheckForUpdate on daemon, handle='%s'\n", + ctx->handle_key); + + g_dbus_connection_call( + ctx->connection, + DBUS_SERVICE_NAME, + DBUS_OBJECT_PATH, + DBUS_INTERFACE_NAME, + DBUS_METHOD_CHECK, + g_variant_new("(s)", ctx->handle_key), + NULL, /* expected reply type: none */ + G_DBUS_CALL_FLAGS_NONE, + DBUS_TIMEOUT_MS, + NULL, /* GCancellable: none */ + NULL, /* reply callback: fire-and-forget */ + NULL /* user_data: none */ + ); -/** - * @brief Reset a registry slot to IDLE - * MUST be called with registry mutex held. - */ -static void registry_reset_slot(CallbackEntry *entry) -{ - if (entry->handle_key != NULL) { - free(entry->handle_key); - entry->handle_key = NULL; + /* [G] Add timeout source: CHECK_SIGNAL_TIMEOUT_SECONDS */ + timeout_source = g_timeout_source_new_seconds(CHECK_SIGNAL_TIMEOUT_SECONDS); + g_source_set_callback(timeout_source, on_check_timeout, ctx, NULL); + g_source_attach(timeout_source, ctx->context); + g_source_unref(timeout_source); /* context holds a ref now */ + + /* [H] Signal caller: "I'm ready" */ + pthread_mutex_lock(&ctx->ready_mutex); + ctx->init_failed = false; + ctx->is_ready = true; + pthread_cond_signal(&ctx->ready_cond); + pthread_mutex_unlock(&ctx->ready_mutex); + + /* [I] Run event loop — blocks until signal arrives or timeout fires */ + FWUPMGR_INFO("check_worker: entering event loop\n"); + g_main_loop_run(ctx->main_loop); + FWUPMGR_INFO("check_worker: event loop exited\n"); + + /* [L] Cleanup */ + if (ctx->subscription_id != 0) { + g_dbus_connection_signal_unsubscribe(ctx->connection, + ctx->subscription_id); } - entry->callback = NULL; - entry->registered_time = 0; - entry->state = CB_STATE_IDLE; + g_object_unref(ctx->connection); + ctx->connection = NULL; + + g_main_context_pop_thread_default(ctx->context); + g_main_loop_unref(ctx->main_loop); + g_main_context_unref(ctx->context); + ctx->main_loop = NULL; + ctx->context = NULL; + + goto cleanup_common; + +/* ---- Error paths ---- */ +init_failed_with_connection: + g_object_unref(ctx->connection); + ctx->connection = NULL; + +init_failed_with_context: + g_main_context_pop_thread_default(ctx->context); + if (ctx->main_loop) { + g_main_loop_unref(ctx->main_loop); + ctx->main_loop = NULL; + } + if (ctx->context) { + g_main_context_unref(ctx->context); + ctx->context = NULL; + } + +init_failed: + /* Signal caller: "I failed to init" */ + pthread_mutex_lock(&ctx->ready_mutex); + ctx->init_failed = true; + ctx->is_ready = true; + pthread_cond_signal(&ctx->ready_cond); + pthread_mutex_unlock(&ctx->ready_mutex); + +cleanup_common: + /* Reset global in-progress state and untrack this context */ + pthread_mutex_lock(&g_check_in_progress_mutex); + g_check_in_progress = false; + g_active_check_ctx = NULL; + pthread_mutex_unlock(&g_check_in_progress_mutex); + + /* Free per-request resources */ + free(ctx->handle_key); + ctx->handle_key = NULL; + pthread_mutex_destroy(&ctx->ready_mutex); + pthread_cond_destroy(&ctx->ready_cond); + free(ctx); + + FWUPMGR_INFO("check_worker: thread exiting\n"); + + /* [M] Thread exits */ + return NULL; } /* ======================================================================== diff --git a/librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h b/librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h index f12de154..31355892 100644 --- a/librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h +++ b/librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h @@ -14,32 +14,42 @@ * @file rdkFwupdateMgr_async_internal.h * @brief Internal types and declarations — NOT part of public API * - * ARCHITECTURE OVERVIEW: - * ====================== - * - * App A ──checkForUpdate(hdl_A, cb_A)──┐ - * App B ──checkForUpdate(hdl_B, cb_B)──┼──► Registry (keyed by handle) - * App C ──checkForUpdate(hdl_C, cb_C)──┘ │ - * │ background thread - * │ watches D-Bus - * ▼ - * Daemon emits CheckForUpdateComplete signal (ONCE) - * │ - * on_check_complete_signal() - * │ - * dispatch_all_pending() │ - * ├── cb_A(hdl_A, &event_data) - * ├── cb_B(hdl_B, &event_data) - * └── cb_C(hdl_C, &event_data) - * - * REGISTRY KEY: - * ============= - * Each entry keyed by FirmwareInterfaceHandle (string from registerProcess). - * One handle → one pending callback at a time. + * ARCHITECTURE OVERVIEW (Phase 1 — CheckForUpdate on-demand thread): + * ================================================================== + * + * CheckForUpdate (ON-DEMAND WORKER THREAD — new): + * + * App calls checkForUpdate(handle, callback) + * │ + * ├─ Allocate CheckRequestContext on heap + * ├─ pthread_create(internal_check_worker_thread, ctx) + * │ │ + * │ ├─ New GMainContext + GMainLoop (isolated) + * │ ├─ g_bus_get_sync() → D-Bus connection + * │ ├─ Subscribe to CheckForUpdateComplete signal + * │ ├─ Send CheckForUpdate D-Bus method call + * │ ├─ Signal caller "ready" via condvar + * │ ├─ g_main_loop_run() — waits for signal or 120s timeout + * │ ├─ Signal arrives → parse → callback(&fwinfo_data) + * │ └─ Cleanup everything, free(ctx), thread exits + * │ + * ├─ pthread_cond_wait() for ready signal + * └─ Return SUCCESS or FAIL + * + * Download / Update (PERSISTENT BG THREAD — unchanged): + * + * App ──downloadFirmware(hdl, req, cb)──► DwnlRegistry ─┐ + * App ──updateFirmware(hdl, req, cb)───► UpdateRegistry ─┼─► BG thread + * │ watches D-Bus + * ▼ + * Daemon emits DownloadProgress / UpdateProgress + * → dispatch to all ACTIVE callbacks * * THREAD SAFETY: * ============== - * Registry protected by pthread_mutex. + * CheckForUpdate: per-request ctx protected by ctx->ready_mutex (handshake), + * g_check_in_progress protected by g_check_in_progress_mutex. + * Download/Update: registries protected by their own pthread_mutex. * Callbacks invoked with mutex RELEASED (deadlock prevention). */ @@ -62,7 +72,6 @@ extern "C" { * ======================================================================== */ #define MAX_PENDING_CALLBACKS 30 /* Reduced from 64 to keep stack usage < 10KB - Need to discuss the max number ; for now kept to 30 to resolve coverity issues*/ -#define CALLBACK_TIMEOUT_SECONDS 60 #define DBUS_SERVICE_NAME "org.rdkfwupdater.Service" #define DBUS_OBJECT_PATH "/org/rdkfwupdater/Service" @@ -71,22 +80,43 @@ extern "C" { #define DBUS_SIGNAL_COMPLETE "CheckForUpdateComplete" #define DBUS_TIMEOUT_MS 5000 +/* Timeout for worker thread waiting for daemon signal (seconds) */ +#define CHECK_SIGNAL_TIMEOUT_SECONDS 120 + /* ======================================================================== - * CALLBACK ENTRY STATE + * CHECKFORUPDATE — ON-DEMAND WORKER THREAD CONTEXT (Phase 1) * ======================================================================== */ /** - * @brief Lifecycle of one registry slot + * @brief Per-request context for on-demand CheckForUpdate worker thread. * - * IDLE ──(register)──► PENDING ──(signal)──► DISPATCHED ──► IDLE - * └──(timeout)──► TIMED_OUT ──► IDLE + * Lifecycle: + * - Allocated by checkForUpdate() (caller thread) via calloc + * - Ownership transferred to worker thread after condvar handshake + * - Freed by worker thread after callback fires (or timeout/error) + * + * Memory: ~100 bytes (excluding GLib objects) */ -typedef enum { - CB_STATE_IDLE = 0, - CB_STATE_PENDING = 1, - CB_STATE_DISPATCHED = 2, - CB_STATE_TIMED_OUT = 3 -} CallbackEntryState; +typedef struct { + /* Condvar handshake: worker signals "I'm ready" to caller */ + pthread_mutex_t ready_mutex; + pthread_cond_t ready_cond; + bool is_ready; /**< true = worker finished setup */ + bool init_failed; /**< true = D-Bus connect/subscribe failed */ + + /* GLib event loop (isolated, per-thread) */ + GMainContext *context; + GMainLoop *main_loop; + GDBusConnection *connection; + guint subscription_id; + + /* Request data */ + char *handle_key; /**< strdup of FirmwareInterfaceHandle */ + UpdateEventCallback callback; /**< Client's callback function ptr */ + + /* Thread handle (for join in destructor) */ + pthread_t thread; +} CheckRequestContext; /* ======================================================================== * INTERNAL SIGNAL DATA @@ -108,40 +138,7 @@ typedef struct { } InternalSignalData; /* ======================================================================== - * CALLBACK REGISTRY ENTRY - * ======================================================================== */ - -/** - * @brief One slot in the callback registry - * - * Keyed by handle_key (strdup of app's FirmwareInterfaceHandle). - * No user_data — aligned to 2-param callback signature. - * - * MEMORY: - * handle_key is strdup'd on registration, freed on slot reset to IDLE. - */ -typedef struct { - CallbackEntryState state; /**< Current lifecycle state */ - char *handle_key; /**< strdup of app's handle */ - UpdateEventCallback callback; /**< App's 2-param callback */ - time_t registered_time; /**< For timeout detection */ -} CallbackEntry; - -/* ======================================================================== - * CALLBACK REGISTRY - * ======================================================================== */ - -/** - * @brief Global registry — one instance per library load - */ -typedef struct { - CallbackEntry entries[MAX_PENDING_CALLBACKS]; - pthread_mutex_t mutex; - bool initialized; -} CallbackRegistry; - -/* ======================================================================== - * BACKGROUND THREAD + * BACKGROUND THREAD (for Download/Update only — Phase 1) * ======================================================================== */ /** @@ -165,7 +162,7 @@ typedef struct { * ======================================================================== */ /** - * @brief Initialize registry and start background thread + * @brief Initialize download/update registries and start background thread * Called from library __attribute__((constructor)). * @return 0 on success, -1 on error */ @@ -178,21 +175,43 @@ int internal_system_init(void); void internal_system_deinit(void); /** - * @brief Register a pending callback keyed by handle + * @brief Worker thread entry point for on-demand CheckForUpdate. * - * No user_data — matches the 2-param UpdateEventCallback signature. + * Creates an isolated GLib event loop, connects to D-Bus, subscribes to + * CheckForUpdateComplete signal, sends CheckForUpdate D-Bus method call, + * then waits for the signal (with 120s timeout). Fires the client's + * callback when signal arrives, then cleans up all resources and exits. + * + * @param arg CheckRequestContext* (ownership transferred from caller) + * @return NULL + */ +void *internal_check_worker_thread(void *arg); + +/** + * @brief Query whether a checkForUpdate() operation is currently in progress. + * + * Used by unregisterProcess() to enforce the session-state invariant: + * a client cannot unregister while it has outstanding operations. + * + * Thread-safe: protected by internal mutex. + * + * @return true if a checkForUpdate worker thread is active, false otherwise. + */ +bool internal_is_check_in_progress(void); + +/** + * @brief Cancel all active checkForUpdate worker threads and join them. * - * @param handle App's FirmwareInterfaceHandle (will be strdup'd) - * @param callback App's UpdateEventCallback (2-param) - * @return true on success, false if registry is full + * Called from library destructor to ensure no threads are running + * when library code is unmapped. */ -bool internal_register_callback(FirmwareInterfaceHandle handle, - UpdateEventCallback callback); +void internal_cancel_all_active_check_threads(void); /** * @brief Parse GVariant signal into InternalSignalData * - * Expected GVariant signature: (iissss) + * Expected GVariant signature: (tiissss) + * t handler_id (uint64) * i result_code * i status_code * s current_version diff --git a/librdkFwupdateMgr/src/rdkFwupdateMgr_process.c b/librdkFwupdateMgr/src/rdkFwupdateMgr_process.c index 8e3c27ae..a76733f1 100755 --- a/librdkFwupdateMgr/src/rdkFwupdateMgr_process.c +++ b/librdkFwupdateMgr/src/rdkFwupdateMgr_process.c @@ -70,6 +70,7 @@ #include "rdkFwupdateMgr_client.h" #include "rdkFwupdateMgr_log.h" +#include "rdkFwupdateMgr_async_internal.h" /* for internal_is_check_in_progress() */ #include #include #include @@ -96,7 +97,7 @@ #define MAX_LIB_VERSION_LEN 64 /** Default D-Bus call timeout in milliseconds (10 seconds) */ -#define DBUS_TIMEOUT_MS 10000 +#define DBUS_TIMEOUT_MSEC 10000 /* ======================================================================== * INTERNAL CONTEXT STRUCTURE @@ -248,7 +249,7 @@ static bool validate_lib_version(const char *libVersion) * IMPLEMENTATION NOTES: * - Creates D-Bus proxy on-demand (no persistent connection) * - Synchronous D-Bus call (blocks until daemon responds) - * - Timeout: 10 seconds (configurable via DBUS_TIMEOUT_MS) + * - Timeout: 10 seconds (configurable via DBUS_TIMEOUT_MSEC) * - Returns string handle (handler_id as decimal string) * * ERROR HANDLING: @@ -297,7 +298,7 @@ FirmwareInterfaceHandle registerProcess(const char *processName, const char *lib "RegisterProcess", g_variant_new("(ss)", processName, libVersion), G_DBUS_CALL_FLAGS_NONE, - DBUS_TIMEOUT_MS, + DBUS_TIMEOUT_MSEC, NULL, // GCancellable &error ); @@ -337,7 +338,7 @@ FirmwareInterfaceHandle registerProcess(const char *processName, const char *lib "UnregisterProcess", g_variant_new("(t)", handler_id), G_DBUS_CALL_FLAGS_NONE, - DBUS_TIMEOUT_MS, + DBUS_TIMEOUT_MSEC, NULL, &cleanup_error ); @@ -390,6 +391,28 @@ void unregisterProcess(FirmwareInterfaceHandle handler) guint64 handler_id = 0; gboolean success = FALSE; + /* Session state validation: reject if checkForUpdate() is active. + * + * You can't hang up the phone while waiting for an answer. + * registerProcess() = start session, checkForUpdate() = ask a question, + * unregisterProcess() = end session. If we let the app end the session + * while the daemon is still processing the firmware check, the daemon- + * client relationship enters an undefined state. So we reject the call + * and tell the app to wait for the callback first, then unregister. + * + * We return without freeing the handle - caller still owns it and can + * retry after the checkForUpdate callback fires (bounded by 120s timeout). + * + * Note: void return type means we can't return an error code. The app + * must check logs. A future API revision will add a return type. + */ + if (internal_is_check_in_progress()) { + FWUPMGR_ERROR("unregisterProcess: REJECTED - checkForUpdate() is in " + "progress. Wait for the callback to fire, then retry " + "unregisterProcess().\n"); + return; + } + // NULL check: Safe to unregister NULL handle (no-op) if (!handler) { FWUPMGR_INFO("unregisterProcess() called with NULL handle (no-op)\n"); @@ -457,7 +480,7 @@ void unregisterProcess(FirmwareInterfaceHandle handler) "UnregisterProcess", g_variant_new("(t)", handler_id), G_DBUS_CALL_FLAGS_NONE, - DBUS_TIMEOUT_MS, + DBUS_TIMEOUT_MSEC, NULL, // GCancellable &error );