RDKEMW-17052:Refactor librdkFwupdateMgr from on-demand threading to a single-threaded design#239
RDKEMW-17052:Refactor librdkFwupdateMgr from on-demand threading to a single-threaded design#239mkadinti wants to merge 42 commits into
Conversation
Release For 1.6.2
Release For 1.6.2
There was a problem hiding this comment.
Pull request overview
Refactors librdkFwupdateMgr initialization so the async engine/background listener thread is started/stopped via registerProcess()/unregisterProcess() rather than at library load/unload, and updates the project changelog.
Changes:
- Start the async engine (
internal_system_init) duringregisterProcess()and stop it (internal_system_deinit) duringunregisterProcess(). - Centralize
DBUS_TIMEOUT_MSby sourcing it fromrdkFwupdateMgr_async_internal.h. - Add
1.6.2entries and a release date note inCHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| librdkFwupdateMgr/src/rdkFwupdateMgr_process.c | Moves async engine init/deinit into process registration lifecycle and pulls in async internal header for shared constants. |
| librdkFwupdateMgr/src/rdkFwupdateMgr_api.c | Disables constructor/destructor lifecycle via #if 0. |
| CHANGELOG.md | Adds a new 1.6.2 section and updates 1.6.1 date metadata. |
Comments suppressed due to low confidence (1)
librdkFwupdateMgr/src/rdkFwupdateMgr_process.c:92
- Including rdkFwupdateMgr_async_internal.h introduces DBUS_SERVICE_NAME/DBUS_OBJECT_PATH/DBUS_INTERFACE_NAME/etc macros that are also defined locally in this file (lines 84-91). With AM_CFLAGS including -Werror, some toolchains warn on macro redefinition; consider removing the local DBUS_* macro definitions here (or guarding the header with #ifndef) and rely on a single source of truth.
#include "rdkFwupdateMgr_client.h"
#include "rdkFwupdateMgr_async_internal.h"
#include "rdkFwupdateMgr_log.h"
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <gio/gio.h>
#include <inttypes.h>
/* ========================================================================
* CONSTANTS
* ======================================================================== */
/** D-Bus service name for firmware update daemon */
#define DBUS_SERVICE_NAME "org.rdkfwupdater.Service"
/** D-Bus object path */
#define DBUS_OBJECT_PATH "/org/rdkfwupdater/Service"
/** D-Bus interface name */
#define DBUS_INTERFACE_NAME "org.rdkfwupdater.Interface"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** Default D-Bus call timeout in milliseconds (10 seconds) */ | ||
| #define DBUS_TIMEOUT_MS 10000 | ||
| /* DBUS_TIMEOUT_MS is defined in rdkFwupdateMgr_async_internal.h */ |
There was a problem hiding this comment.
DBUS_TIMEOUT_MS is now sourced from rdkFwupdateMgr_async_internal.h (currently 5000ms), but this file's documentation still references a 10-second timeout elsewhere and the constant is shared with async APIs. Consider either keeping a dedicated process-registration timeout (if intended to remain 10s) or updating the documentation/usage to reflect the new 5s value consistently.
| /* DBUS_TIMEOUT_MS is defined in rdkFwupdateMgr_async_internal.h */ | |
| /* | |
| * D-Bus timeout for process registration/unregistration calls. | |
| * | |
| * DBUS_TIMEOUT_MS is defined in rdkFwupdateMgr_async_internal.h and is | |
| * intentionally shared with the async APIs. The current value is 5000 ms, | |
| * so this file must not document or assume a separate 10-second timeout. | |
| */ |
| Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). | ||
|
|
||
| #### [1.6.2](https://github.com/rdkcentral/rdkfwupdater/compare/1.6.1...1.6.2) | ||
|
|
There was a problem hiding this comment.
The new 1.6.2 changelog section is missing the release date line (> <date>) that other entries include. Add the date to keep the auto-changelog format consistent.
| > <date> |
| if (internal_system_init() != 0) { | ||
| FWUPMGR_ERROR("rdkFwupdateMgr_lib_init: internal_system_init FAILED\n"); | ||
| } | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); |
There was a problem hiding this comment.
Spelling in log message: "successfull" → "successful".
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successful ===\n"); |
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thred unloading ===\n"); | ||
| internal_system_deinit(); | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr destory thread ===\n"); | ||
|
|
There was a problem hiding this comment.
unregisterProcess() calls internal_system_deinit() for every handle. Because the async engine is global per library load, deinitializing it on the first unregister will tear down the shared background thread and registries while other handles (or in-flight callbacks) may still be active. Make deinit conditional (e.g., refcount and only deinit when the last registration is removed) and consider ordering it after the D-Bus UnregisterProcess call succeeds (or at least after the local handle is removed from any registries).
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thred unloading ===\n"); | |
| internal_system_deinit(); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr destory thread ===\n"); | |
| // Do not deinitialize the shared async/global subsystem here. | |
| // unregisterProcess() is per-handle, while internal_system_deinit() | |
| // tears down library-wide state and can break other active handles or | |
| // in-flight callbacks if called on the first unregister. |
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thred unloading ===\n"); | ||
| internal_system_deinit(); | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr destory thread ===\n"); |
There was a problem hiding this comment.
Spelling in log messages: "thred" → "thread" and "destory" → "destroy".
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thred unloading ===\n"); | |
| internal_system_deinit(); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr destory thread ===\n"); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thread unloading ===\n"); | |
| internal_system_deinit(); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thread ===\n"); |
| @@ -182,7 +182,7 @@ static void rdkFwupdateMgr_lib_deinit(void) | |||
| internal_system_deinit(); | |||
| FWUPMGR_INFO("=== rdkFwupdateMgr library unloaded ===\n"); | |||
| } | |||
|
|
|||
| #endif | |||
There was a problem hiding this comment.
Wrapping the constructor/destructor-based lifecycle in #if 0 leaves dead code in the file and changes the initialization contract (the async engine is no longer guaranteed to be started on library load / stopped on unload). Prefer removing this block entirely or gating it behind a named build-time macro (e.g., RDKFWUPDATEMGR_ENABLE_CONSTRUCTORS) and ensure there is a single, well-defined init/deinit path (especially for dlclose scenarios).
|
|
||
| - RDKEMW-10029 : Syncing of Gerrit commits that are required for security components [`#209`](https://github.com/rdkcentral/rdkfwupdater/pull/209) | ||
| - RDKEMW-15146 : Introduce mechanism to unlock debug service - read /opt/swupdate.conf [`#205`](https://github.com/rdkcentral/rdkfwupdater/pull/205) | ||
| - RDKTV-39830:Xumo TV devices got stuck with old firmware with http eror code 405 [`#213`](https://github.com/rdkcentral/rdkfwupdater/pull/213) |
There was a problem hiding this comment.
Typo in changelog entry: "http eror" → "http error".
| - RDKTV-39830:Xumo TV devices got stuck with old firmware with http eror code 405 [`#213`](https://github.com/rdkcentral/rdkfwupdater/pull/213) | |
| - RDKTV-39830:Xumo TV devices got stuck with old firmware with http error code 405 [`#213`](https://github.com/rdkcentral/rdkfwupdater/pull/213) |
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen ===\n"); | ||
| if (internal_system_init() != 0) { | ||
| FWUPMGR_ERROR("rdkFwupdateMgr_lib_init: internal_system_init FAILED\n"); | ||
| } | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); |
There was a problem hiding this comment.
This file’s header comment describes the implementation as having “No background threads / Stateless design”, but registerProcess()/unregisterProcess() now start/stop the async background thread via internal_system_init/deinit. Please update the file-level documentation (architecture + thread safety notes) so it matches the new behavior and lifecycle.
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen ===\n"); | ||
| if (internal_system_init() != 0) { | ||
| FWUPMGR_ERROR("rdkFwupdateMgr_lib_init: internal_system_init FAILED\n"); | ||
| } | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); |
There was a problem hiding this comment.
internal_system_init() is called unconditionally on every registerProcess() invocation. internal_system_init() is not idempotent (it memset()s globals, re-inits mutexes, and spawns a thread), so multiple registrations (or concurrent registrations) can lead to double-init, leaked threads, or undefined behavior. Add a one-time init guard / reference counting (init on first register, deinit on last unregister), and make it thread-safe.
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen ===\n"); | ||
| if (internal_system_init() != 0) { | ||
| FWUPMGR_ERROR("rdkFwupdateMgr_lib_init: internal_system_init FAILED\n"); | ||
| } | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); | ||
|
|
||
| return (FirmwareInterfaceHandle)handle_str; |
There was a problem hiding this comment.
If internal_system_init() fails, registerProcess() still returns a valid handle and logs a "success" message. That leaves callers thinking async callbacks will work when the background listener thread may not be running (and internal_system_init() failure paths may also leak partial resources). Consider treating init failure as fatal for registration: perform best-effort UnregisterProcess cleanup + free(handle_str) and return NULL (or otherwise propagate the failure), and only log success when init succeeds.
…kfwupdater into topic/RDKEMW-17052
… single-threaded design- bring defects'fixes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/dbus/rdkFwupdateMgr_handlers.c:490
local_immed_reboot_flagdefaulting to "true" disables throttling based on the currentisThrottleEnabled()logic (it only enables throttle whenreboot_immediate_flagis "false"). This also changeschunk_dwnl_retry_timebehavior indownloadFile()("false" → 10 retries, otherwise 0). If the intent is to allow throttling for daemon XCONF fetch, the default here should remain "false" (or updateisThrottleEnabled()semantics consistently).
const char *local_immed_reboot_flag = "true"; // Making true as default setting to make it work in Throttle enable mode.
int local_delay_dwnl = 0; // Default daemon setting
const char *local_lastrun = "0"; // Default daemon setting
char *local_disableStatsUpdate = "false"; // Default daemon setting
int local_force_exit = 0; // Default daemon setting
int local_trigger_type = 1; // Default daemon setting
xconf_context.immed_reboot_flag = local_immed_reboot_flag;
xconf_context.delay_dwnl = local_delay_dwnl;
xconf_context.lastrun = local_lastrun;
xconf_context.disableStatsUpdate = local_disableStatsUpdate;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * The file has been cleaned up by initialValidation(). | ||
| * In the daemon mode, we transition to IDLE and wait | ||
| * initialValidation() is also responsible for emitting | ||
| * the MAINT_FWDOWNLOAD_COMPLETE event for this case. | ||
| * In the daemon mode, we transition to IDLE and wait | ||
| * for the pending reboot or next D-Bus request. |
There was a problem hiding this comment.
The added comment block has duplicated/unfinished sentences ("In the daemon mode, we transition to IDLE and wait" appears twice and one occurrence is missing a period). Please de-duplicate and clarify the comment so it matches the actual control flow.
| * The file has been cleaned up by initialValidation(). | |
| * In the daemon mode, we transition to IDLE and wait | |
| * initialValidation() is also responsible for emitting | |
| * the MAINT_FWDOWNLOAD_COMPLETE event for this case. | |
| * In the daemon mode, we transition to IDLE and wait | |
| * for the pending reboot or next D-Bus request. | |
| * The file has been cleaned up by initialValidation(), | |
| * which is also responsible for emitting the | |
| * MAINT_FWDOWNLOAD_COMPLETE event for this case. | |
| * In daemon mode, transition to IDLE and wait for | |
| * the pending reboot or the next D-Bus request. |
| if (internal_system_init() != 0) { | ||
| FWUPMGR_ERROR("rdkFwupdateMgr_lib_init: internal_system_init FAILED\n"); | ||
| } | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); |
There was a problem hiding this comment.
Log messages contain typos (e.g., "successfull", "thred", "destory"), which makes log searching/alerting harder. Please correct the spelling in these strings.
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successfull ===\n"); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen successful ===\n"); |
| #if defined(RDK_LOGGER) | ||
| #include "rdk_debug.h" | ||
|
|
||
| /* Generic base macro callers provide their own module name */ | ||
| #define FWUPMGR_LOG(level, module, format, ...) \ | ||
| RDK_LOG(level, module, format, ##__VA_ARGS__) | ||
|
|
||
| /* Default library macros use LOG.RDK.FWUPMGR */ | ||
| #define FWUPMGR_TRACE(format, ...) FWUPMGR_LOG(RDK_LOG_TRACE1, "LOG.RDK.FWUPMGR", format, ##__VA_ARGS__) | ||
| #define FWUPMGR_DEBUG(format, ...) FWUPMGR_LOG(RDK_LOG_DEBUG, "LOG.RDK.FWUPMGR", format, ##__VA_ARGS__) | ||
| #define FWUPMGR_INFO(format, ...) FWUPMGR_LOG(RDK_LOG_INFO, "LOG.RDK.FWUPMGR", format, ##__VA_ARGS__) | ||
| #define FWUPMGR_WARN(format, ...) FWUPMGR_LOG(RDK_LOG_WARN, "LOG.RDK.FWUPMGR", format, ##__VA_ARGS__) | ||
| #define FWUPMGR_ERROR(format, ...) FWUPMGR_LOG(RDK_LOG_ERROR, "LOG.RDK.FWUPMGR", format, ##__VA_ARGS__) | ||
| #define FWUPMGR_FATAL(format, ...) FWUPMGR_LOG(RDK_LOG_FATAL, "LOG.RDK.FWUPMGR", format, ##__VA_ARGS__) | ||
|
|
||
| #else | ||
|
|
||
|
|
||
| /* Default library macros */ | ||
| #define FWUPMGR_TRACE(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) | ||
| #define FWUPMGR_DEBUG(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) | ||
| #define FWUPMGR_INFO(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) | ||
| #define FWUPMGR_WARN(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) | ||
| #define FWUPMGR_ERROR(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) | ||
| #define FWUPMGR_FATAL(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) | ||
|
|
There was a problem hiding this comment.
When RDK_LOGGER is not defined, the fallback branch still calls FWUPMGR_LOG(...) and references FWUPMGR_LOG_INFO, but neither is defined in this header. This will fail to compile in non-RDK_LOGGER builds (including unit-test builds that use the stub rdkv_cdl_log_wrapper.h). Define a proper non-RDK_LOGGER fallback (e.g., map to SWLOG_INFO/SWLOG_ERROR or fprintf(stderr, ...)) and avoid relying on undefined symbols.
| * library logs and [FWUPG] daemon logs. | ||
| * ======================================================================== */ | ||
| #define EXAMPLE_DEBUG(format, ...) FWUPMGR_LOG(RDK_LOG_DEBUG, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | ||
| #define EXAMPLE_INFO(format, ...) FWUPMGR_LOG(RDK_LOG_INFO, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | ||
| #define EXAMPLE_WARN(format, ...) FWUPMGR_LOG(RDK_LOG_WARN, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | ||
| #define EXAMPLE_ERROR(format, ...) FWUPMGR_LOG(RDK_LOG_ERROR, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) |
There was a problem hiding this comment.
The example's EXAMPLE_* macros unconditionally use RDK_LOG_* constants and FWUPMGR_LOG(...). If RDK_LOGGER is not defined (or rdk_debug.h isn't available), this will not compile. Consider guarding these macros with #if defined(RDK_LOGGER) and providing a printf/stderr fallback for non-RDK_LOGGER builds so the example remains buildable across configurations.
| * library logs and [FWUPG] daemon logs. | |
| * ======================================================================== */ | |
| #define EXAMPLE_DEBUG(format, ...) FWUPMGR_LOG(RDK_LOG_DEBUG, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #define EXAMPLE_INFO(format, ...) FWUPMGR_LOG(RDK_LOG_INFO, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #define EXAMPLE_WARN(format, ...) FWUPMGR_LOG(RDK_LOG_WARN, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #define EXAMPLE_ERROR(format, ...) FWUPMGR_LOG(RDK_LOG_ERROR, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| * library logs and [FWUPG] daemon logs. | |
| * For non-RDK_LOGGER builds, fall back to stderr so this example remains | |
| * buildable even when RDK logging headers/constants are unavailable. | |
| * ======================================================================== */ | |
| #if defined(RDK_LOGGER) | |
| #define EXAMPLE_DEBUG(format, ...) FWUPMGR_LOG(RDK_LOG_DEBUG, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #define EXAMPLE_INFO(format, ...) FWUPMGR_LOG(RDK_LOG_INFO, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #define EXAMPLE_WARN(format, ...) FWUPMGR_LOG(RDK_LOG_WARN, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #define EXAMPLE_ERROR(format, ...) FWUPMGR_LOG(RDK_LOG_ERROR, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | |
| #else | |
| #define EXAMPLE_DEBUG(format, ...) fprintf(stderr, "[EXAMPLE][DEBUG] " format "\n", ##__VA_ARGS__) | |
| #define EXAMPLE_INFO(format, ...) fprintf(stderr, "[EXAMPLE][INFO] " format "\n", ##__VA_ARGS__) | |
| #define EXAMPLE_WARN(format, ...) fprintf(stderr, "[EXAMPLE][WARN] " format "\n", ##__VA_ARGS__) | |
| #define EXAMPLE_ERROR(format, ...) fprintf(stderr, "[EXAMPLE][ERROR] " format "\n", ##__VA_ARGS__) | |
| #endif |
… single-threaded design- bring defects'fixes
… single-threaded design- bring defects'fixes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NOTE: Sender-ID ownership check is intentionally skipped for UnregisterProcess. | ||
| // The single-thread library model creates a new D-Bus connection per API call, | ||
| // resulting in different sender IDs for Register vs Unregister. | ||
| // The handler_id itself serves as the authorization token. |
There was a problem hiding this comment.
UnregisterProcess now skips validating the caller against the original registrant and relies on handler_id as the only authorization token. Since handler_ids are predictable (monotonic starting at 1), any local D-Bus client can unregister other clients by guessing IDs (DoS). Consider restoring an ownership check using stable credentials (e.g., Unix UID/PID from the bus for the sender) or returning a cryptographically-random token at RegisterProcess and requiring it for UnregisterProcess.
| * In the daemon mode, we transition to IDLE and wait | ||
| * initialValidation() is also responsible for emitting | ||
| * the MAINT_FWDOWNLOAD_COMPLETE event for this case. | ||
| * In the daemon mode, we transition to IDLE and wait | ||
| * for the pending reboot or next D-Bus request. |
There was a problem hiding this comment.
This comment block repeats the sentence "In the daemon mode, we transition to IDLE and wait" twice, which makes the intent harder to read. Please remove the duplicate line(s) and keep a single clear explanation of the daemon behavior for INITIAL_VALIDATION_DWNL_COMPLETED.
| Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). | ||
|
|
||
| #### [1.6.2](https://github.com/rdkcentral/rdkfwupdater/compare/1.6.1...1.6.2) | ||
|
|
There was a problem hiding this comment.
CHANGELOG.md states that dates are displayed in UTC, and other versions include a date line (e.g., "> 11 March 2026" for 1.6.1). The new 1.6.2 section is missing its release date; please add it for consistency.
| > 12 March 2026 |
| const char *local_immed_reboot_flag = "true"; // Making true as default setting to make it work in Throttle enable mode. | ||
| int local_delay_dwnl = 0; // Default daemon setting | ||
| const char *local_lastrun = "0"; // Default daemon setting | ||
| char *local_disableStatsUpdate = "false"; // Default daemon setting |
There was a problem hiding this comment.
Setting local_immed_reboot_flag to "true" will disable throttling in isThrottleEnabled(), which only enables throttling when reboot_immediate_flag == "false" (see src/device_status_helper.c:497-502). This change appears to invert the intended behavior and contradicts the new comment. Please revert to "false" (or adjust isThrottleEnabled()/callers consistently) so throttle-enable mode can actually engage.
| char *local_disableStatsUpdate = "false"; // Default daemon setting | |
| char local_disableStatsUpdate[] = "false"; // Default daemon setting |
| #include "rdkFwupdateMgr_client.h" | ||
| #include "rdkFwupdateMgr_async_internal.h" | ||
| #include "rdkFwupdateMgr_log.h" |
There was a problem hiding this comment.
rdkFwupdateMgr_process.c now includes the internal header rdkFwupdateMgr_async_internal.h to get DBUS_TIMEOUT_MS. This couples the public process registration API to async internals (pulling in pthread/GLib types and increasing ABI/compile-time coupling). Prefer defining DBUS_TIMEOUT_MS in a shared public header (or locally in this file) so process registration doesn't depend on internal async implementation details.
| FWUPMGR_INFO(" handler_id: %"G_GUINT64_FORMAT"\n", handler_id); | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thred unloading ===\n"); | ||
| internal_system_deinit(); | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr destory thread ===\n"); |
There was a problem hiding this comment.
Typo in log message: "destory" should be "destroy".
| FWUPMGR_INFO("=== rdkFwupdateMgr destory thread ===\n"); | |
| FWUPMGR_INFO("=== rdkFwupdateMgr destroy thread ===\n"); |
| example_plugin_CFLAGS = \ | ||
| -I${top_srcdir}/librdkFwupdateMgr/include \ | ||
| -I${top_srcdir}/librdkFwupdateMgr/src \ | ||
| -I${top_srcdir}/common_utilities/utils \ |
There was a problem hiding this comment.
example_plugin_CFLAGS now adds -I${top_srcdir}/common_utilities/utils, but this repository doesn't contain a common_utilities/ directory. This makes building from a standalone source tree brittle (headers like rdkv_cdl_log_wrapper.h may still be missing unless common_utilities is checked out adjacent to the repo). Prefer relying on installed headers via pkg-config / compiler include paths, or add a configure-time check to locate the dependency rather than hardcoding a relative source path.
| -I${top_srcdir}/common_utilities/utils \ |
… single-threaded design- bring defects'fixes- UnregisterProcess
… single-threaded design- bring defects'fixes- Design docs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/dbus/rdkv_dbus_server.c:419
- remove_process_from_tracking() now ignores sender_id and no longer validates that the caller owns handler_id. Since handler_id is a sequential counter (see next_process_id++ in add_process_to_tracking), any authorized D-Bus client can unregister other clients by guessing IDs, which is a security/robustness regression. If sender-id ownership can't be used due to per-call ephemeral connections, consider storing and validating caller credentials that are stable across connections (e.g., uid/pid from D-Bus peer credentials) or return an unguessable token and require it for UnregisterProcess.
* @param sender_id D-Bus sender ID of the requesting client (unused - kept for API compatibility)
* @return TRUE if found and removed, FALSE if not found
*/
static gboolean remove_process_from_tracking(guint64 handler_id, const gchar *sender_id)
{
(void)sender_id; // Not used - single-thread library model creates new D-Bus connection per API call
ProcessInfo *info = g_hash_table_lookup(registered_processes, GINT_TO_POINTER(handler_id));
if (!info) {
SWLOG_INFO("[PROCESS_TRACKING] Handler %"G_GUINT64_FORMAT" not found\n", handler_id);
return FALSE;
}
SWLOG_INFO("[PROCESS_TRACKING] Removing: %s (handler: %"G_GUINT64_FORMAT")\n", info->process_name, handler_id);
g_hash_table_remove(registered_processes, GINT_TO_POINTER(handler_id));
SWLOG_INFO("[PROCESS_TRACKING] Total registered: %d\n", g_hash_table_size(registered_processes));
return TRUE;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Remove from tracking system | ||
| SWLOG_INFO("[UNREGISTER] Attempting to remove process '%s' from tracking...\n", process_name); | ||
| if (remove_process_from_tracking(handler, rdkv_req_caller_id)) { | ||
| SWLOG_INFO("[UNREGISTER] SUCCESS: Process '%s' unregistered successfully!\n", process_name); |
There was a problem hiding this comment.
Now that ownership checks were removed, the failure path/logging for UnregisterProcess should no longer reference "access denied" / "unauthorized" (it can only be "handler not found"). Consider updating the failure messages and return error semantics accordingly so operators aren’t misled during troubleshooting.
| * In the daemon mode, we transition to IDLE and wait | ||
| * initialValidation() is also responsible for emitting | ||
| * the MAINT_FWDOWNLOAD_COMPLETE event for this case. | ||
| * In the daemon mode, we transition to IDLE and wait | ||
| * for the pending reboot or next D-Bus request. | ||
| */ |
There was a problem hiding this comment.
This comment block repeats the same sentence twice ("In the daemon mode, we transition to IDLE and wait"). Please deduplicate to keep the rationale clear and avoid drift in future edits.
… single-threaded design- bring defects'fixes- Design docs
… single-threaded design- bring defects'fixes- Documentation for Register and Unregister process APIs
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… single-threaded design- bring defects'fixes- Addressing copilot reviews
…kfwupdater into topic/RDKEMW-17052
… single-threaded design- bring defects'fixes- Addressing copilot reviews
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
librdkFwupdateMgr/src/rdkFwupdateMgr_process.c:525
- registerProcess(): on g_dbus_proxy_call_sync() failure, the code logs error->message and unconditionally calls g_error_free(error). GLib may return NULL without setting a GError in some cases (e.g., extreme OOM), which would make this dereference crash. Guard against error == NULL (log an "unknown" message) and only free when non-NULL.
FWUPMGR_ERROR("RegisterProcess D-Bus call failed: %s\n",
error->message);
g_error_free(error);
g_object_unref(proxy);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #else | ||
|
|
||
|
|
||
| /* Default library macros */ | ||
| #define FWUPMGR_TRACE(FORMAT...) FWUPMGR_LOG(FWUPMGR_LOG_INFO, "FWUPMGR", FORMAT) |
There was a problem hiding this comment.
In the non-RDK_LOGGER build, the FWUPMGR_* macros expand to FWUPMGR_LOG(...), but FWUPMGR_LOG (and FWUPMGR_LOG_INFO) are only defined in the RDK_LOGGER branch. This will fail to compile when RDK_LOGGER is not set (and also breaks example_app which relies on FWUPMGR_LOG). Define a fallback FWUPMGR_LOG macro (e.g., to fprintf(stderr, ...) or to SWLOG_*) and appropriate level constants in the #else branch, or avoid referencing FWUPMGR_LOG there.
| // Look up process info to validate ownership and get process name for logging | ||
| ProcessInfo *process_info = g_hash_table_lookup(registered_processes, GINT_TO_POINTER(handler)); | ||
| const gchar *process_name = process_info ? process_info->process_name : "UNKNOWN"; | ||
|
|
||
| // Validate ownership before attempting removal | ||
| if (process_info && g_strcmp0(process_info->sender_id, rdkv_req_caller_id) != 0) { | ||
| SWLOG_ERROR("[UNREGISTER] Access denied: Handler %"G_GUINT64_FORMAT" (process: %s) owned by '%s', but '%s' attempted to unregister\n", | ||
| handler, process_name, process_info->sender_id, rdkv_req_caller_id); | ||
| g_dbus_method_invocation_return_error(resp_ctx, | ||
| G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED, | ||
| "Unregistration denied: Handler owned by different client"); | ||
| return; | ||
| } | ||
|
|
||
| // Remove from tracking system (this will double-check ownership) | ||
| // Remove from tracking system | ||
| SWLOG_INFO("[UNREGISTER] Attempting to remove process '%s' from tracking...\n", process_name); | ||
| if (remove_process_from_tracking(handler, rdkv_req_caller_id)) { |
There was a problem hiding this comment.
UnregisterProcess handling no longer performs any ownership/authorization check before removing a registration; it relies entirely on handler_id existence. With sequential handler IDs this allows one authorized caller to unregister other clients by guessing handler IDs. If sender_id validation is not viable with the current connection model, please add a different unguessable per-registration secret/token and validate it here (or adjust the client library to use a stable sender for unregister).
|
|
||
| - RDKEMW-10029 : Syncing of Gerrit commits that are required for security components [`#209`](https://github.com/rdkcentral/rdkfwupdater/pull/209) | ||
| - RDKEMW-15146 : Introduce mechanism to unlock debug service - read /opt/swupdate.conf [`#205`](https://github.com/rdkcentral/rdkfwupdater/pull/205) | ||
| - RDKTV-39830:Xumo TV devices got stuck with old firmware with http eror code 405 [`#213`](https://github.com/rdkcentral/rdkfwupdater/pull/213) |
There was a problem hiding this comment.
Typo in the changelog entry: "http eror code 405" should be "http error code 405" for consistency with the other entries.
| - RDKTV-39830:Xumo TV devices got stuck with old firmware with http eror code 405 [`#213`](https://github.com/rdkcentral/rdkfwupdater/pull/213) | |
| - RDKTV-39830:Xumo TV devices got stuck with old firmware with http error code 405 [`#213`](https://github.com/rdkcentral/rdkfwupdater/pull/213) |
… single-threaded design- bring defects'fixes- Addressing copilot reviews
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…kfwupdater into topic/RDKEMW-17052
… single-threaded design- bring defects'fixes- modified code comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/dbus/rdkv_dbus_server.c:421
- remove_process_from_tracking() no longer validates that the caller owns the handler_id. With sender validation removed, any permitted D-Bus client can unregister any other client by guessing/obtaining its handler_id (which is sequential via next_process_id++). If sender-id validation can't work with the per-call connection model, consider storing/verifying a stable credential (uid/pid from D-Bus, or a random capability token returned at registration) or update the D-Bus API to include an unguessable secret.
* @brief Remove a process from the tracking system.
*
* Called when client invokes UnregisterProcess. Frees associated ProcessInfo.
*
* @param handler_id Handler ID to remove
* @param sender_id D-Bus sender ID of the requesting client; currently unused but
* retained to keep this internal helper aligned with its caller context and to
* allow future sender-based validation or auditing.
* @return TRUE if found and removed, FALSE if not found
*/
static gboolean remove_process_from_tracking(guint64 handler_id, const gchar *sender_id)
{
(void)sender_id; // Intentionally unused for now; reserved for possible sender-aware checks
ProcessInfo *info = g_hash_table_lookup(registered_processes, GINT_TO_POINTER(handler_id));
if (!info) {
SWLOG_INFO("[PROCESS_TRACKING] Handler %"G_GUINT64_FORMAT" not found\n", handler_id);
return FALSE;
}
SWLOG_INFO("[PROCESS_TRACKING] Removing: %s (handler: %"G_GUINT64_FORMAT")\n", info->process_name, handler_id);
g_hash_table_remove(registered_processes, GINT_TO_POINTER(handler_id));
SWLOG_INFO("[PROCESS_TRACKING] Total registered: %d\n", g_hash_table_size(registered_processes));
return TRUE;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * EXAMPLE_* logging macros use FWUPMGR_LOG with LOG.RDK.EXAMPLE module. | ||
| * Keeps example_plugin logs as [EXAMPLE], distinguishable from [FWUPMGR] | ||
| * library logs and [FWUPG] daemon logs. | ||
| * ======================================================================== */ | ||
| #define EXAMPLE_DEBUG(format, ...) FWUPMGR_LOG(RDK_LOG_DEBUG, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | ||
| #define EXAMPLE_INFO(format, ...) FWUPMGR_LOG(RDK_LOG_INFO, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | ||
| #define EXAMPLE_WARN(format, ...) FWUPMGR_LOG(RDK_LOG_WARN, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) | ||
| #define EXAMPLE_ERROR(format, ...) FWUPMGR_LOG(RDK_LOG_ERROR, "LOG.RDK.EXAMPLE", format, ##__VA_ARGS__) |
| ProcessInfo *process_info = g_hash_table_lookup(registered_processes, GINT_TO_POINTER(handler)); | ||
| const gchar *process_name = process_info ? process_info->process_name : "UNKNOWN"; | ||
|
|
||
| // Validate ownership before attempting removal | ||
| if (process_info && g_strcmp0(process_info->sender_id, rdkv_req_caller_id) != 0) { | ||
| SWLOG_ERROR("[UNREGISTER] Access denied: Handler %"G_GUINT64_FORMAT" (process: %s) owned by '%s', but '%s' attempted to unregister\n", | ||
| handler, process_name, process_info->sender_id, rdkv_req_caller_id); | ||
| g_dbus_method_invocation_return_error(resp_ctx, | ||
| G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED, | ||
| "Unregistration denied: Handler owned by different client"); | ||
| return; | ||
| } | ||
|
|
||
| // Remove from tracking system (this will double-check ownership) | ||
| // Remove from tracking system | ||
| SWLOG_INFO("[UNREGISTER] Attempting to remove process '%s' from tracking...\n", process_name); | ||
| if (remove_process_from_tracking(handler, rdkv_req_caller_id)) { | ||
| SWLOG_INFO("[UNREGISTER] SUCCESS: Process '%s' unregistered successfully!\n", process_name); |
| getRFCSettings(&local_rfc_list); // Read actual RFC settings from system | ||
|
|
||
| const char *local_immed_reboot_flag = "false"; // Default daemon setting | ||
| const char *local_immed_reboot_flag = "NA"; // Keeping it as NA as default setting to make it work in Throttle enable mode. |
| /* Initialize async engine: registries, mutexes, BG thread */ | ||
| FWUPMGR_INFO("=== rdkFwupdateMgr Creating thread for listen ===\n"); | ||
| if (internal_system_init() != 0) { | ||
| FWUPMGR_ERROR("rdkFwupdateMgr_lib_init: internal_system_init FAILED\n"); | ||
| GError *cleanup_error = NULL; | ||
| GDBusProxy *cleanup_proxy = create_dbus_proxy(&cleanup_error); | ||
| if (cleanup_proxy) { | ||
| GVariant *cleanup_result = g_dbus_proxy_call_sync( | ||
| cleanup_proxy, | ||
| "UnregisterProcess", | ||
| g_variant_new("(t)", handler_id), | ||
| G_DBUS_CALL_FLAGS_NONE, | ||
| DBUS_SYNC_TIMEOUT_MS, | ||
| NULL, | ||
| &cleanup_error | ||
| ); | ||
| if (cleanup_result) { | ||
| FWUPMGR_INFO("Cleanup successful: process unregistered\n"); | ||
| g_variant_unref(cleanup_result); | ||
| } else { | ||
| FWUPMGR_ERROR("Cleanup failed: %s (registration may be leaked)\n", | ||
| cleanup_error ? cleanup_error->message : "unknown"); | ||
| if (cleanup_error) g_error_free(cleanup_error); | ||
| } | ||
| g_object_unref(cleanup_proxy); | ||
| } else { | ||
| FWUPMGR_ERROR("Cleanup proxy creation failed (registration leaked)\n"); | ||
| if (cleanup_error) g_error_free(cleanup_error); | ||
| } | ||
|
|
||
| free(handle_str); | ||
| return NULL; | ||
| } |
… single-threaded design- bring defects'fixes- modified code comments
… single-threaded design- bring defects'fixes- modified code comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/dbus/rdkv_dbus_server.c:421
- UnregisterProcess no longer validates that the requesting client is allowed to remove the given handler_id. With the current implementation, any D-Bus client that can call this method can unregister (and disrupt) another client's session by guessing/obtaining its handler_id.
If sender-name validation was removed because clients use ephemeral D-Bus connections, consider validating on a stable property instead (e.g., compare the caller's Unix UID via org.freedesktop.DBus.GetConnectionUnixUser/GetConnectionCredentials, or store a per-registration secret/token and require it on unregister).
static gboolean remove_process_from_tracking(guint64 handler_id, const gchar *sender_id)
{
(void)sender_id; // Intentionally unused for now; reserved for possible sender-aware checks
ProcessInfo *info = g_hash_table_lookup(registered_processes, GINT_TO_POINTER(handler_id));
if (!info) {
SWLOG_INFO("[PROCESS_TRACKING] Handler %"G_GUINT64_FORMAT" not found\n", handler_id);
return FALSE;
}
SWLOG_INFO("[PROCESS_TRACKING] Removing: %s (handler: %"G_GUINT64_FORMAT")\n", info->process_name, handler_id);
g_hash_table_remove(registered_processes, GINT_TO_POINTER(handler_id));
SWLOG_INFO("[PROCESS_TRACKING] Total registered: %d\n", g_hash_table_size(registered_processes));
return TRUE;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.