RDKEMW-14991:[Xione UK] - Firmware download should not happen when throttle speed is set to 0 by mkadinti · Pull Request #202 · rdkcentral/rdkfwupdater

mkadinti · 2026-03-05T11:28:47Z

No description provided.

… UnregisterProcess

… CheckForUpdate- adding signals

…- testClient and unittest changes

…- testClient modified

… updating testClient for - signals option

… updating testClient for - signals option - core debug

… updating testClient for - signals option - core debug - tral fix

… Enhanced testClient

… img validation enhancement

… unittests

… PR Clean up

… l2 unittests

… Coverity fixes

…copilot reviews

…rottle speed is set to 0

Copilot

Pull request overview

This PR updates the firmware download/upgrade flow to avoid terminating the process when throttling is enabled with a max speed of 0, and introduces structured negative “library error” return codes so callers (CLI vs daemon) can decide how to handle fatal conditions.

Changes:

Replace several exit(1) paths in download/upgrade code with negative error returns (e.g., throttle speed 0, “force exit” / curl error 23) and add rdkv_upgrade_strerror() for logging.
Update daemon/CLI call sites and unit-test mocks for the new checkAndEnterStateRed() return type, and add D-Bus DownloadFirmware logic to allow an empty URL (resolve via XConf cache).
Add initial build system plumbing for a new librdkFwupdateMgr client library header/target.

Reviewed changes

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

Show a summary per file

File	Description
unittest/test_dbus_fake.cpp	Updates copyright year.
unittest/miscellaneous_mock.cpp	Updates mocks/stubs to match new return types and adds extra stubs behind `#ifndef GTEST_BASIC`.
unittest/Makefile.am	Adjusts unit-test sources (adds/removes real sources vs mocks).
test/functional-tests/tests/test_dbus_UpdateFirmware.py	Minor refactors; changes daemon startup usage in one test.
test/functional-tests/tests/test_dbus_DownloadFirmware.py	Switches helper import style; adjusts a few test variables/exception handling.
test/functional-tests/tests/test_dbus_CheckForUpdate.py	Switches helper import style; minor refactor.
src/rdkv_upgrade.c	Introduces negative library error codes, removes process exits for throttle/force-exit, and logs library errors.
src/rdkv_main.c	Adds handling/logging for negative library errors in the CLI path.
src/rdkFwupdateMgr.c	Adds handling/logging for negative library errors in the daemon path.
src/include/rdkv_upgrade.h	Defines negative library error codes + declares `rdkv_upgrade_strerror()`.
src/include/device_status_helper.h	Changes `checkAndEnterStateRed()` signature to return `int`.
src/device_status_helper.c	Makes `checkAndEnterStateRed()` return status instead of exiting on TLS/SSL errors.
src/dbus/rdkv_dbus_server.c	Allows empty `download_url` and resolves via XConf cache; removes disabled code blocks.
src/dbus/rdkFwupdateMgr_handlers.h	Adds prototypes/docs for XConf cache helpers and opt-out response helper.
src/dbus/rdkFwupdateMgr_handlers.c	Adds an in-memory XConf cache + opt-out response helper; logs library errors.
src/chunk.c	Replaces `exit(1)` force-exit paths with negative error returns.
run_l2.sh	Removes `--enable-extended-logger` from configure invocation.
cov_build.sh	Removes `--enable-extended-logger` from configure invocation.
librdkFwupdateMgr/include/rdkFwupdateMgr_client.h	Adds a new public client header/API definitions.
Makefile.am	Adds `librdkFwupdateMgr.la` target and installs its public header.

Comments suppressed due to low confidence (2)

test/functional-tests/tests/test_dbus_DownloadFirmware.py:110

The file switches to import rdkfw_test_helper, but it still calls helper functions like remove_file() / initial_rdkfw_setup() / write_on_file() as bare names. This will raise NameError at runtime. Either revert to from rdkfw_test_helper import * or update all helper calls to use the rdkfw_test_helper. prefix (or import specific symbols explicitly).

import rdkfw_test_helper

# D-Bus Configuration
DBUS_SERVICE_NAME = "org.rdkfwupdater.Service"
DBUS_OBJECT_PATH = "/org/rdkfwupdater/Service"
DBUS_INTERFACE = "org.rdkfwupdater.Interface"
DAEMON_BINARY = "/usr/local/bin/rdkFwupdateMgr"

# Daemon files
STATUS_FILE = "/tmp/dnldmgr_status.txt"
PROGRESS_FILE = "/opt/curl_progress"
XCONF_CACHE_FILE = "/tmp/xconf_response_thunder.txt"
SWUPDATE_LOG_FILE_0 = "/opt/logs/swupdate.txt.0"

# Result codes
#DOWNLOAD_SUCCESS = 0
#DOWNLOAD_ALREADY_EXISTS = 1
#DOWNLOAD_NETWORK_ERROR = 2
#DOWNLOAD_NOT_FOUND = 3
#DOWNLOAD_ERROR = 4

#DownloadResult
RDKFW_DWNL_SUCCESS = 0 #Firmware download initiated successfully.
RDKFW_DWNL_FAILED = 1  #Firmware download initiation failed.

def write_device_prop():
    file_path = "/etc/device.properties"
    data = """DEVICE_NAME=DEV_CONTAINER
DEVICE_TYPE=mediaclient
DIFW_PATH=/opt/CDL
ENABLE_MAINTENANCE=false
MODEL_NUM=ABCD
ENABLE_SOFTWARE_OPTOUT=false
BUILD_TYPE=VBN
ESTB_INTERFACE=eth0
PDRI_ENABLED=true
"""
    try:
        with open(file_path, "w") as file:
            file.write(data)
    except Exception as e:
        print(f"Error creating device.properties: {e}")


def start_daemon():
    """Start D-Bus daemon"""
    subprocess.run(['pkill', '-9', '-f', 'rdkFwupdateMgr'], capture_output=True)
    time.sleep(0.5)
    proc = subprocess.Popen([DAEMON_BINARY, "0", "1"])
    time.sleep(3)
    return proc


def stop_daemon(proc):
    """Stop daemon"""
    proc.terminate()
    proc.wait()


def iface():
    """Get D-Bus interface"""
    bus = dbus.SystemBus()
    proxy = bus.get_object(DBUS_SERVICE_NAME, DBUS_OBJECT_PATH)
    return dbus.Interface(proxy, DBUS_INTERFACE)


def cleanup_daemon_files():
    """Clean daemon-specific files including flash indicators"""
    remove_file(STATUS_FILE)
    remove_file(PROGRESS_FILE)
    remove_file(XCONF_CACHE_FILE)
    # Clean flash indicator files to ensure test isolation
    remove_file("/tmp/fw_preparing_to_reboot")
    remove_file("/tmp/currently_running_image_name")
    remove_file("/opt/cdl_flashed_file_name")

def wait_for_file(filepath, timeout=15.0):
    """Wait for file to exist"""
    start = time.time()
    while time.time() - start < timeout:
        if os.path.exists(filepath):
            time.sleep(0.5)
            return True
        time.sleep(0.2)

test/functional-tests/tests/test_dbus_CheckForUpdate.py:90

The file switches to import rdkfw_test_helper, but it still uses helper functions (rename_file, write_on_file, remove_file, initial_rdkfw_setup, etc.) without qualifying them. This will fail at runtime with NameError. Update call sites to rdkfw_test_helper.<fn>() (or import the required symbols explicitly).

import rdkfw_test_helper

# D-Bus Configuration
DBUS_SERVICE_NAME = "org.rdkfwupdater.Service"
DBUS_OBJECT_PATH = "/org/rdkfwupdater/Service"
DBUS_INTERFACE = "org.rdkfwupdater.Interface"
DAEMON_BINARY = "/usr/local/bin/rdkFwupdateMgr"

# Cache files
XCONF_CACHE_FILE = "/tmp/xconf_response_thunder.txt"
XCONF_HTTP_CODE_FILE = "/tmp/xconf_httpcode_thunder.txt"
SWUPDATE_CONF_FILE = "/opt/swupdate.conf"
SWUPDATE_LOG_FILE_0 = "/opt/logs/swupdate.txt.0"

# Mock XConf URLs
XCONF_NORMAL_URL = "https://mockxconf:50052/firmwareupdate/getfirmwaredata"
XCONF_404_URL = "https://mockxconf:50052/firmwareupdate404/getfirmwaredata"
XCONF_INVALID_JSON_URL = "https://mockxconf:50052/firmwareupdate/getinvalidfirmwaredata"
XCONF_UNRESOLVED_URL = "https://unmockxconf:50052/featureControl/getSettings"
XCONF_INVALIDPCI_URL = "https://mockxconf:50052/firmwareupdate/getinvalidpcifirmwaredata"
XCONF_DELAY_URL = "https://mockxconf:50052/firmwareupdate/delaydwnlfirmwaredata"
XCONF_REBOOT_URL = "https://mockxconf:50052/firmwareupdate/getreboottruefirmwaredata"

# Backup conf file
BKUP_SWUPDATE_CONF_FILE = "/opt/bk_swupdate.conf"


# Result codes
CHECK_FOR_UPDATE_SUCCESS = 0  # API call succeeded
CHECK_FOR_UPDATE_FAIL = 1     # API call failed

# Status codes
FIRMWARE_AVAILABLE = 0
FIRMWARE_NOT_AVAILABLE = 1
UPDATE_NOT_ALLOWED = 2
FIRMWARE_CHECK_ERROR = 3
IGNORE_OPTOUT = 4
BYPASS_OPTOUT = 5

def set_xconf_url(url):
    """
    Set XConf URL in swupdate.conf
    This simulates different XConf server behaviors
    """
    # Backup original if exists
    if os.path.exists(SWUPDATE_CONF_FILE) and not os.path.exists(BKUP_SWUPDATE_CONF_FILE):
        rename_file(SWUPDATE_CONF_FILE, BKUP_SWUPDATE_CONF_FILE)
    
    # Write new URL
    write_on_file(SWUPDATE_CONF_FILE, url)
    print(f"[SETUP] XConf URL set to: {url}")

def restore_xconf_url():
    """Restore original XConf URL"""
    if os.path.exists(BKUP_SWUPDATE_CONF_FILE):
        remove_file(SWUPDATE_CONF_FILE)
        rename_file(BKUP_SWUPDATE_CONF_FILE, SWUPDATE_CONF_FILE)

def wait_for_cache_creation(timeout=30):
    """
    Wait for XConf cache to be created
    """
    print(f"[INFO] Waiting for XConf query and cache creation (max {timeout}s)...")
    elapsed = 0
    wait_interval = 5

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

Copilot · 2026-03-05T11:34:18Z

+    start_daemon()
    initial_rdkfw_setup()
    write_device_prop()
    cleanup_daemon_files()

    # Create mock firmware file
    firmware_name = "ABCD_PCI_test.bin"
-    firmware_path = create_mock_firmware_file(firmware_name)
+    create_mock_firmware_file(firmware_name)


This test now starts the daemon but never stops it (and doesn’t use a try/finally to restore files). This can leak a running rdkFwupdateMgr process and break isolation for later functional tests. Please capture the returned proc and call stop_daemon(proc) (and restore/cleanup any mock files) in a finally block, consistent with other tests in this suite.

Copilot · 2026-03-05T11:34:18Z

    // HTTP CODE 495 - Expired client certificate not in servers allow list
    if( http_code == 495 ) {
        SWLOG_INFO("%s : Calling checkAndEnterStateRed() with code:%d\n", __FUNCTION__, http_code);
-        checkAndEnterStateRed(http_code, disableStatsUpdate);
+        if (checkAndEnterStateRed(http_code, disableStatsUpdate) != 0) {
+            SWLOG_ERROR("%s : State red entered due to HTTP error %d\n", __FUNCTION__, http_code);
+        }
    }else {
        SWLOG_INFO("%s : Calling checkAndEnterStateRed() with code:%d\n", __FUNCTION__, curl_code);
-        checkAndEnterStateRed(curl_code, disableStatsUpdate);
+        if (checkAndEnterStateRed(curl_code, disableStatsUpdate) != 0) {
+            SWLOG_ERROR("%s : State red entered due to curl error %d\n", __FUNCTION__, curl_code);
+        }


checkAndEnterStateRed() now returns an error code instead of exiting on TLS/SSL failures, but dwnlError() only logs the non-zero return and then continues. This likely changes CLI behavior (previously it would terminate on TLS/SSL fatal errors). Consider propagating the non-zero return up to the caller (or explicitly documenting that state-red entry is non-fatal here) so callers can decide whether to abort.

Copilot · 2026-03-05T11:34:19Z

+     ${top_srcdir}/librdkFwupdateMgr/src/
+    #${top_srcdir}/librdkFwupdateMgr/src/handle_mgr.c \
+    #${top_srcdir}/librdkFwupdateMgr/src/handle_registry.c \
+    #${top_srcdir}/librdkFwupdateMgr/src/dbus_client.c \
+    #${top_srcdir}/librdkFwupdateMgr/src/api_impl.c


librdkFwupdateMgr_la_SOURCES is set to a directory path (.../librdkFwupdateMgr/src/), which Automake will treat as a missing source file and fail the build. Replace this with an explicit list of .c/.cpp sources for the client library (or remove the library target until sources are ready).

Suggested change

${top_srcdir}/librdkFwupdateMgr/src/

#${top_srcdir}/librdkFwupdateMgr/src/handle_mgr.c \

#${top_srcdir}/librdkFwupdateMgr/src/handle_registry.c \

#${top_srcdir}/librdkFwupdateMgr/src/dbus_client.c \

#${top_srcdir}/librdkFwupdateMgr/src/api_impl.c

${top_srcdir}/librdkFwupdateMgr/src/handle_mgr.c \

${top_srcdir}/librdkFwupdateMgr/src/handle_registry.c \

${top_srcdir}/librdkFwupdateMgr/src/dbus_client.c \

${top_srcdir}/librdkFwupdateMgr/src/api_impl.c

Copilot · 2026-03-05T11:34:19Z

+    int url_len = snprintf(imageHTTPURL, sizeof(imageHTTPURL), "%s/%s", effective_download_url, ctx->firmware_name);
    if (url_len < 0 || url_len >= sizeof(imageHTTPURL)) {
 	    SWLOG_ERROR("[DOWNLOAD_WORKER] ERROR: URL too long or snprintf failed (len=%d, max=%zu)\n",
 			    url_len, sizeof(imageHTTPURL));
-	    SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", ctx->download_url, ctx->firmware_name);
+	    SWLOG_ERROR("[DOWNLOAD_WORKER] URL would be: %s/%s\n", effective_download_url, ctx->firmware_name);
+	    g_free(effective_download_url);
 	    g_task_return_boolean(task, FALSE);


The worker constructs the final URL as "%s/%s" (base URL + firmware name). Existing callers/tests pass download_url as a full URL including the filename (e.g. .../test_progress.bin), which would yield an invalid doubled path (.../test_progress.bin/test_progress.bin). Consider treating a non-empty download_url as the full URL when it already ends with the firmware filename (or enforce/document that download_url must be a directory/base path).

rdkcmf-jenkins · 2026-03-05T11:38:42Z

Coverity Issue - Identical code for different branches

Ternary expression on condition "isinstance(result, tuple)" has identical then and else expressions: "result[0]". Should one of the expressions be modified, or the entire ternary expression replaced?