Skip to content

RDKEMW-15498:Implement software update service layer library (Fix review comments) - #225

Open
mkadinti wants to merge 38 commits into
developfrom
topic/RDKEMW-15498
Open

RDKEMW-15498:Implement software update service layer library (Fix review comments)#225
mkadinti wants to merge 38 commits into
developfrom
topic/RDKEMW-15498

Conversation

@mkadinti

Copy link
Copy Markdown
Contributor

No description provided.

@mkadinti
mkadinti requested a review from a team as a code owner March 17, 2026 08:52
Copilot AI review requested due to automatic review settings March 17, 2026 08:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR redesigns the checkForUpdate() client-side implementation in librdkFwupdateMgr to use an on-demand worker thread (per request) instead of the legacy registry + persistent signal subscription model, and adds a session-state guard to prevent unregisterProcess() while a check is active.

Changes:

  • Replaced CheckForUpdate registry-based dispatch with a per-request worker thread that subscribes to CheckForUpdateComplete, fires the callback, and self-terminates (with a 120s timeout).
  • Added process-level “check in progress” state and destructor cleanup to cancel/join an active worker on library unload.
  • Added a session-state guard in unregisterProcess() to reject unregister while checkForUpdate() is active; updated/added design & progress documentation.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Rewrites checkForUpdate() to spawn/handshake with a worker thread; destructor now cancels/joins active workers before deinit.
librdkFwupdateMgr/src/rdkFwupdateMgr_async.c Removes legacy CheckForUpdate registry dispatch; adds worker thread engine, signal/timeout handlers, and cancellation/join support.
librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h Introduces CheckRequestContext and new internal APIs; removes legacy registry types; updates architecture docs (with one doc mismatch noted).
librdkFwupdateMgr/src/rdkFwupdateMgr_process.c Adds session-state validation to reject unregisterProcess() while a check is in progress; renames timeout macro locally.
docs/DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md Adds a detailed design document for the Phase 1 redesign.
docs/CHECKFORUPDATE_PROGRESS.md Adds a progress tracker / next steps doc for the redesign.
docs/TRACKING_CHECKFORUPDATE_REDESIGN.md Present but currently empty.
Comments suppressed due to low confidence (1)

librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h:148

  • This BackgroundThread struct comment still says the background thread “Subscribes to CheckForUpdateComplete signal”, but in Phase 1 the background thread now only subscribes to DownloadProgress/UpdateProgress (CheckForUpdate is handled by on-demand workers). Please update this documentation to match the new behavior to avoid misleading maintainers.
/**
 * @brief State for the background GLib event loop thread
 *
 * Started at library load. Subscribes to CheckForUpdateComplete signal.
 * Runs until library unload.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_process.c
Copilot AI review requested due to automatic review settings March 25, 2026 06:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR continues the firmware update manager library refactor by moving checkForUpdate() and downloadFirmware() to an on-demand worker-thread model, tightening session-state handling, and updating design documentation to match the new architecture.

Changes:

  • Switched checkForUpdate() and downloadFirmware() to per-request worker threads with condvar “ready” handshakes and bounded GLib-loop timeouts.
  • Added session-state rejection in unregisterProcess() while checkForUpdate() / downloadFirmware() are in progress.
  • Added/updated extensive design docs for the on-demand thread approach (Phase 1+2).

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
librdkFwupdateMgr/src/rdkFwupdateMgr_process.c Adds session-state guards to reject unregister during active operations; renames process-layer D-Bus timeout macro.
librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h Updates internal architecture overview and introduces/extends internal worker-thread context APIs for Phase 1+2.
librdkFwupdateMgr/src/rdkFwupdateMgr_async.c Implements on-demand worker threads and state tracking for CheckForUpdate and DownloadFirmware; trims BG thread scope to Update only.
librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Rewrites public checkForUpdate() and downloadFirmware() to spawn/join worker threads via internal accessors and handshakes.
docs/DESIGN_CHECKFORUPDATE_ON_DEMAND_THREAD.md Adds/updates detailed Phase 1 design documentation.
docs/DESIGN_DOWNLOAD_FIRMWARE_ON_DEMAND_THREAD.md Adds detailed Phase 2 design documentation for DownloadFirmware on-demand thread model.
docs/CHECKFORUPDATE_PROGRESS.md Tracks implementation status and next steps for Phase 1.
docs/TRACKING_CHECKFORUPDATE_REDESIGN.md Present as an (empty) tracking artifact in the docs set.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_process.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c
Copilot AI review requested due to automatic review settings March 26, 2026 09:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 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.

Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_process.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread kwib_src/KnowWhereItBreaks.c Outdated
Copilot AI review requested due to automatic review settings March 30, 2026 21:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c Outdated
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_process.c
Comment thread docs/KnowWhereItBreaks.md Outdated
Comment thread kwib_src/KnowWhereItBreaks.c Outdated
Comment thread kwib_src/KnowWhereItBreaks.c Outdated
Comment thread kwib_src/KnowWhereItBreaks.c Outdated
Comment thread Makefile.am Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 31, 2026 07:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread kwib_src/USAGE_KWIB.md Outdated
Comment thread kwib_src/KnowWhereItBreaks_README.md Outdated
Comment thread kwib_src/KnowWhereItBreaks.c Outdated
Comment thread docs/KnowWhereItBreaks.md Outdated
Comment thread docs/KnowWhereItBreaks.md Outdated
Copilot AI review requested due to automatic review settings April 1, 2026 07:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_api.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async.c
Comment thread librdkFwupdateMgr/src/rdkFwupdateMgr_async_internal.h
Copilot AI review requested due to automatic review settings April 9, 2026 04:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +261 to +265
if (failed) {
FWUPMGR_ERROR("checkForUpdate: worker thread failed to initialize. "
"handle='%s'\n", handle);
pthread_join(worker_thread, NULL);

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failed conflates ctx->init_failed with pthread_cond_timedwait() returning ETIMEDOUT, then unconditionally pthread_join()s the worker and destroys/frees ctx. If the timed-wait expires while the worker is still on the normal (success) path (where caller_owns_cleanup stays false), the worker will eventually free ctx itself before exiting; after pthread_join() returns, this path can double-free / use-after-free ctx. Please handle ETIMEDOUT as a separate path: only destroy/free ctx in the caller when the worker explicitly transferred ownership (caller_owns_cleanup==true), and otherwise cancel/detach the worker (or extend the ready timeout to cover the worker’s longest synchronous init step) without freeing ctx here.

Copilot uses AI. Check for mistakes.
Comment on lines +572 to +607
if (wait_rc == ETIMEDOUT) {
FWUPMGR_ERROR("downloadFirmware: worker thread did not signal ready "
"within %ds — treating as init failure. handle='%s'\n",
WORKER_READY_TIMEOUT_SEC, handle);
}

/* [8] Check if worker failed to initialize or daemon rejected
*
* If init_failed is true, either D-Bus setup failed or the daemon
* rejected the download request. The worker thread is already
* cleaning itself up. We join it to avoid a zombie thread.
*
* IMPORTANT: We use worker_thread (local copy), NOT ctx->thread.
* After the condvar wake, the worker may have already freed ctx.
*/
if (failed) {
FWUPMGR_ERROR("downloadFirmware: worker init failed or daemon rejected. "
"handle='%s'\n", handle);
pthread_join(worker_thread, NULL);

/* After join, the worker has exited. If caller_owns_cleanup is set,
* the worker left the mutex/cond/ctx alive for us to clean up safely.
* The worker already freed its own resources (daemon_reject_message, etc.)
* but left our strdup'd strings, mutex/cond, and ctx for us. */
pthread_mutex_destroy(&ctx->ready_mutex);
pthread_cond_destroy(&ctx->ready_cond);
free(ctx->handle_key);
free(ctx->firmware_name);
free(ctx->firmware_url);
free(ctx->firmware_type);
free(ctx);

return RDKFW_DWNL_FAILED;
}

/* [9] Worker is running and listening for DownloadProgress signals.

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue in the downloadFirmware failure path: failed treats ETIMEDOUT the same as init_failed and then pthread_join()s and frees ctx. If the timed-wait expires while the worker is still in its normal init (e.g., blocked in the 30s synchronous D-Bus call) the worker will later run the success cleanup path (caller_owns_cleanup==false) and free ctx itself; after join returns, this path can double-free / UAF. Please split the timeout path from init-failure/daemon-reject and avoid freeing ctx here unless ownership was transferred.

Suggested change
if (wait_rc == ETIMEDOUT) {
FWUPMGR_ERROR("downloadFirmware: worker thread did not signal ready "
"within %ds — treating as init failure. handle='%s'\n",
WORKER_READY_TIMEOUT_SEC, handle);
}
/* [8] Check if worker failed to initialize or daemon rejected
*
* If init_failed is true, either D-Bus setup failed or the daemon
* rejected the download request. The worker thread is already
* cleaning itself up. We join it to avoid a zombie thread.
*
* IMPORTANT: We use worker_thread (local copy), NOT ctx->thread.
* After the condvar wake, the worker may have already freed ctx.
*/
if (failed) {
FWUPMGR_ERROR("downloadFirmware: worker init failed or daemon rejected. "
"handle='%s'\n", handle);
pthread_join(worker_thread, NULL);
/* After join, the worker has exited. If caller_owns_cleanup is set,
* the worker left the mutex/cond/ctx alive for us to clean up safely.
* The worker already freed its own resources (daemon_reject_message, etc.)
* but left our strdup'd strings, mutex/cond, and ctx for us. */
pthread_mutex_destroy(&ctx->ready_mutex);
pthread_cond_destroy(&ctx->ready_cond);
free(ctx->handle_key);
free(ctx->firmware_name);
free(ctx->firmware_url);
free(ctx->firmware_type);
free(ctx);
return RDKFW_DWNL_FAILED;
}
/* [9] Worker is running and listening for DownloadProgress signals.
/* [8] Handle worker-ready wait timeout separately from true init failure.
*
* A timeout only means the worker did not signal readiness within the
* caller's wait window. The worker may still be in its normal init path
* (for example, blocked in a synchronous D-Bus call) and may later take
* the normal success/self-cleanup path. In that case, ownership of ctx
* was never transferred back to the caller, so we must not join/free it.
*/
if (wait_rc == ETIMEDOUT) {
FWUPMGR_ERROR("downloadFirmware: worker thread did not signal ready "
"within %ds. Leaving worker ownership of context intact. "
"handle='%s'\n",
WORKER_READY_TIMEOUT_SEC, handle);
return RDKFW_DWNL_FAILED;
}
/* [9] Check if worker failed to initialize or daemon rejected.
*
* In this path the worker has signaled readiness/result and exited or is
* exiting the init phase. We join it to avoid a zombie thread, but only
* destroy/free ctx if the worker explicitly transferred cleanup ownership
* back to the caller.
*
* IMPORTANT: We use worker_thread (local copy), NOT ctx->thread.
*/
if (failed) {
FWUPMGR_ERROR("downloadFirmware: worker init failed or daemon rejected. "
"handle='%s'\n", handle);
pthread_join(worker_thread, NULL);
/* After join, the worker has exited. Only clean up ctx here when the
* worker left the mutex/cond/ctx alive for caller-side cleanup. */
if (ctx->caller_owns_cleanup) {
pthread_mutex_destroy(&ctx->ready_mutex);
pthread_cond_destroy(&ctx->ready_cond);
free(ctx->handle_key);
free(ctx->firmware_name);
free(ctx->firmware_url);
free(ctx->firmware_type);
free(ctx);
}
return RDKFW_DWNL_FAILED;
}
/* [10] Worker is running and listening for DownloadProgress signals.

Copilot uses AI. Check for mistakes.
Comment on lines +406 to +442
/* 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;
}

/* Session state validation: reject if downloadFirmware() is active.
*
* Same rationale as checkForUpdate: you can't end the session while a
* firmware download is in progress. Downloads can take 1-30 minutes,
* but the app should wait for the DWNL_COMPLETED or DWNL_ERROR callback
* before unregistering. If the app receives SIGTERM, it should just exit()
* — the daemon detects the D-Bus peer disconnect and cleans up.
*
* We return without freeing the handle — caller still owns it and can
* retry after the download callback fires with a terminal status.
*/
if (internal_is_dwnl_in_progress()) {
FWUPMGR_ERROR("unregisterProcess: REJECTED - downloadFirmware() is in "
"progress. Wait for the DWNL_COMPLETED or DWNL_ERROR "
"callback, then retry unregisterProcess().\n");

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unregisterProcess() now returns early when an operation is in progress and explicitly does not free handler (see comment at lines 415-416). This breaks the public API contract in rdkFwupdateMgr_client.h (“The library owns this string… It becomes invalid after you call unregisterProcess()”), and can also leak the handle if the caller follows that contract and never frees it. Consider either (a) keeping the contract: always free the handle on return and perform best-effort daemon cleanup (or cancel/join active worker threads before unregistering), or (b) if changing ownership is intended, update the public header/API to state the caller must retain/free/retry and ideally return an error code instead of void.

Suggested change
/* 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;
}
/* Session state validation: reject if downloadFirmware() is active.
*
* Same rationale as checkForUpdate: you can't end the session while a
* firmware download is in progress. Downloads can take 1-30 minutes,
* but the app should wait for the DWNL_COMPLETED or DWNL_ERROR callback
* before unregistering. If the app receives SIGTERM, it should just exit()
*the daemon detects the D-Bus peer disconnect and cleans up.
*
* We return without freeing the handlecaller still owns it and can
* retry after the download callback fires with a terminal status.
*/
if (internal_is_dwnl_in_progress()) {
FWUPMGR_ERROR("unregisterProcess: REJECTED - downloadFirmware() is in "
"progress. Wait for the DWNL_COMPLETED or DWNL_ERROR "
"callback, then retry unregisterProcess().\n");
/* Session state validation: reject daemon-side unregister 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.
*
* However, the public API contract still requires unregisterProcess()
* to invalidate the library-owned handle on return. So we skip the
* daemon call, but still free the local handle as best-effort cleanup.
*/
if (internal_is_check_in_progress()) {
FWUPMGR_ERROR("unregisterProcess: checkForUpdate() is in progress; "
"skipping daemon unregister and freeing local handle "
"per API contract.\n");
free(handler);
return;
}
/* Session state validation: reject daemon-side unregister if
* downloadFirmware() is active.
*
* Same rationale as checkForUpdate: you can't end the session while a
* firmware download is in progress. Downloads can take 1-30 minutes,
* and daemon-side cleanup is deferred until the operation completes or
* the daemon observes disconnect.
*
* As above, unregisterProcess() must still invalidate the library-owned
* handle on return, so free it before leaving this early path.
*/
if (internal_is_dwnl_in_progress()) {
FWUPMGR_ERROR("unregisterProcess: downloadFirmware() is in progress; "
"skipping daemon unregister and freeing local handle "
"per API contract.\n");
free(handler);

Copilot uses AI. Check for mistakes.
Comment thread librdkFwupdateMgr/examples/example_app.c Outdated
…iew comments)- adding pdri support to example_plugin
selected_fw_name = g_peripheral_firmwares;
selected_fw_url = ""; /* Daemon resolves URL for peripherals */
break;
default:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverity Issue - Dead default in switch

Execution cannot reach this statement: "default:".

Low Impact, CWE-561
DEADCODE

mkadinti and others added 3 commits April 13, 2026 11:26
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…(Fix review comments)- adding pdri support to example_plugin"

This reverts commit 930bb98.
Copilot AI review requested due to automatic review settings April 15, 2026 00:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +590 to +604
FWUPMGR_INFO("on_download_signal_handler: handler=%" PRIu64
" firmware='%s' progress=%u%% status='%s' handle='%s'\n",
signal_data.handler_id,
signal_data.firmware_name ? signal_data.firmware_name : "(null)",
signal_data.progress_percent,
signal_data.status_string ? signal_data.status_string : "(null)",
ctx->handle_key ? ctx->handle_key : "(null)");

/* Map status string to enum */
DownloadStatus status = map_dwnl_status_string(signal_data.status_string);

/* Fire the client's callback with progress and status */
if (ctx->callback != NULL) {
ctx->callback((int)signal_data.progress_percent, status);
}
Comment on lines +821 to +835
FWUPMGR_INFO("on_update_signal_handler: handler=%" PRIu64
" firmware='%s' progress=%d%% status=%d handle='%s'\n",
signal_data.handler_id,
signal_data.firmware_name ? signal_data.firmware_name : "(null)",
signal_data.progress_percent,
signal_data.status_code,
ctx->handle_key ? ctx->handle_key : "(null)");

internal_cleanup_signal_data(&signal_data);
/* Map status code to enum */
UpdateStatus status = internal_map_update_status_code(signal_data.status_code);

/* Fire the client's callback with progress and status */
if (ctx->callback != NULL) {
ctx->callback(signal_data.progress_percent, status);
}
Comment thread Makefile.am
Comment on lines +187 to +188
librdkFwupdateMgr_la_CFLAGS = -fPIC -I${top_srcdir}/librdkFwupdateMgr/include -I${top_srcdir}/librdkFwupdateMgr/src -I${top_srcdir}/common_utilities/utils $(AM_CFLAGS) $(GLIB_CFLAGS)
librdkFwupdateMgr_la_CPPFLAGS = -fPIC -I${top_srcdir}/librdkFwupdateMgr/include -I${top_srcdir}/librdkFwupdateMgr/src -I${top_srcdir}/common_utilities/utils $(GLIB_CFLAGS)
Comment on lines +421 to +426
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;
}
Comment on lines +334 to 349
FWUPMGR_INFO("on_check_signal_handler: received CheckForUpdateComplete "
"for handle='%s'\n",
ctx->handle_key ? ctx->handle_key : "(null)");

/* Parse signal payload */
InternalSignalData signal_data;
memset(&signal_data, 0, sizeof(signal_data));

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;
}
Copilot AI review requested due to automatic review settings April 28, 2026 00:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/dbus/rdkFwupdateMgr_handlers.c:488

  • local_immed_reboot_flag is used by isThrottleEnabled(), which only enables throttling when reboot_immediate_flag is "false" (non-critical update). Setting the default to "true" disables throttling even when the RFC throttle flag is enabled and video is playing/background mode. If the intent is to enable throttling by default (or preserve previous behavior), this should remain "false" or be derived from the actual XConf cloudImmediateRebootFlag instead of hardcoding "true" here.
                    Rfc_t local_rfc_list = {0};
                    getRFCSettings(&local_rfc_list);  // Read actual RFC settings from system
                    
                    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;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +44 to 54

/* ========================================================================
* 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__)
#include <stdio.h>
Comment on lines 336 to 340
* t handlerId (uint64 - handler ID)
* s firmwareName (string - firmware filename)
* u progress (uint32 - 0-100 percent)
* s status (string - "INPROGRESS", "COMPLETED", "NOTSTARTED")
* s status (string - "INPROGRESS", "COMPLETED", "ERROR")
* s message (string - human-readable message)
Comment on lines +54 to +78
#if defined(RDK_LOGGER)
#include "rdk_debug.h"

/* ========================================================================
* LOGGING MACROS - Same pattern as SWLOG_* in daemon
* ======================================================================== */
/* Generic base macro callers provide their own module name */
#define FWUPMGR_LOG(level, module, format, ...) \
RDK_LOG(level, module, format, ##__VA_ARGS__)

/**
* @brief Log informational message
*
* Usage: FWUPMGR_INFO("Registered with handler: %s\n", handler_id);
*/
#define FWUPMGR_INFO(format, ...) \
fwupmgr_log_internal("INFO", "[%s:%d] " format, __FUNCTION__, __LINE__, ##__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__)

/**
* @brief Log error message
*
* Usage: FWUPMGR_ERROR("Registration failed: %s\n", error_msg);
*/
#define FWUPMGR_ERROR(format, ...) \
fwupmgr_log_internal("ERROR", "[%s:%d] " format, __FUNCTION__, __LINE__, ##__VA_ARGS__)
#else

/**
* @brief Log debug message
*
* Usage: FWUPMGR_DEBUG("D-Bus proxy created: %p\n", proxy);
*/
#define FWUPMGR_DEBUG(format, ...) \
fwupmgr_log_internal("DEBUG", "[%s:%d] " format, __FUNCTION__, __LINE__, ##__VA_ARGS__)

/**
* @brief Log warning message
*
* Usage: FWUPMGR_WARN("Daemon not responding, retry recommended\n");
*/
#define FWUPMGR_WARN(format, ...) \
fwupmgr_log_internal("WARN", "[%s:%d] " format, __FUNCTION__, __LINE__, ##__VA_ARGS__)
/* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants