RDKB-66348: Migrate Crashupload to compiled code in RDKB

[gomathishankar37]

No description provided.

[Copilot]

Pull request overview

This PR continues the migration of the Crashupload workflow from script-driven behavior toward a compiled C implementation for RDKB, aligning behavior (paths, checks, and markers) and improving structured error handling/logging.

Changes:

• Refactors prerequisites dump-detection logic (including broadband/extender parity) and standardizes prerequisite return codes.
• Improves operational logging by adding device-type context and consistent “SUCCESS” messaging for archive/upload paths.
• Hardens initialization and configuration flow with explicit failure handling and broadband log-directory preparation.

Reviewed changes

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

Show a summary per file

File	Description
c_sourcecode/src/utils/prerequisites.c	Adds helper-based dump presence detection and adjusts prerequisite error/return handling.
c_sourcecode/src/utils/logger.h	Removes DEBUG_INI_NAME macro from the header.
c_sourcecode/src/utils/logger.c	Moves debug.ini path usage into the init path and adjusts logger initialization behavior.
c_sourcecode/src/upload/upload.c	Enhances upload success logs with device type + dump type context.
c_sourcecode/src/main.c	Improves prerequisite failure messaging and adds broadband /tmp/crash_reboot marker creation on exit.
c_sourcecode/src/init/system_init.c	Converts initialization failures to explicit error codes and adds failure checks for config/platform init.
c_sourcecode/src/config/config_manager.c	Adds broadband directory creation support and applies broadband/extender /minidumps path parity.
c_sourcecode/src/archive/archive.c	Enhances archive creation success logs with device type + dump type context.
c_sourcecode/common/types.h	Adds device_type_to_str() helper and extends config_t with comm_interface.
c_sourcecode/common/errors.h	Introduces additional error codes for prerequisites and system initialization.

---

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

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (6)

c_sourcecode/src/init/system_init.c:61

• If open() fails when creating core_log_file, system_initialize() returns immediately but leaves telemetry initialized (it was started via t2Init() earlier). Clean up before returning to keep partial-init failures from leaking process state.

        if (fd < 0)
        {
            CRASHUPLOAD_ERROR("open failed\n");
            return ERR_SYSTEM_INIT_FAILED;

c_sourcecode/src/init/system_init.c:75

• On platform_initialize() failure, system_initialize() returns ERR_PLATFORM_INIT_FAILED but does not undo the earlier t2Init(). Consider uninitializing telemetry on this failure path as well.

    if (platform_initialize(config, platform) != PLATFORM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("platform_initialize failed\n");
        return ERR_PLATFORM_INIT_FAILED;
    }

c_sourcecode/src/utils/prerequisites.c:134

• has_required_dumps() no longer respects config->dump_type for non-broadband/extender devices. This can cause prerequisites to succeed when only the other dump type exists (e.g., minidump present but running in coredump mode), which contradicts existing dump-type selection behavior and can lead to later scan/upload doing extra work and returning different results.

    /* Script parity: for non-broadband/extender, continue if either minidump or coredump exists. */
    if (directory_has_pattern(config->minidump_path, ".dmp") == 1)
    {
        return 1;
    }

c_sourcecode/src/init/system_init.c:52

• system_initialize() calls t2Init() but returns early on config_init_load() failure without uninitializing telemetry. Since this function owns the initialization, it should also clean up on failure to avoid requiring every caller (including unit tests) to do it correctly.

This issue also appears in the following locations of the same file:

• line 58
• line 71

    if (config_init_load(config, argc, argv) != CONFIG_SUCCESS)
    {
        CRASHUPLOAD_ERROR("config_init_load failed\n");
        return ERR_SYSTEM_INIT_FAILED;
    }

c_sourcecode/src/config/config_manager.c:73

• ensure_directory_exists() treats errno == EEXIST as success without verifying the existing path is actually a directory. If a regular file exists at that path, later logging writes will still fail but this function will report success.

            if (mkdir(path, 0777) != 0 && errno != EEXIST)
            {
                return -1;
            }

c_sourcecode/src/main.c:191

• prerequisites_wait() currently returns ERR_INVALID_ARGUMENT, NO_DUMPS_FOUND, or PREREQUISITES_SUCCESS (0). Since it never returns ERR_PREREQUISITE_FAILED today, the first branch here is dead and the log severity is inconsistent between failure types.

        if(prereq_ret == ERR_PREREQUISITE_FAILED)
        {
            CRASHUPLOAD_ERROR("Prerequisites check failed\n");
        }
        else if(prereq_ret == NO_DUMPS_FOUND)

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

c_sourcecode/src/config/config_manager.c:74

• The final mkdir() in ensure_directory_exists() also uses mode 0777, which makes the target directory world-writable. Prefer 0755 (or another least-privilege mode consistent with the platform’s logging directory expectations).

    if (mkdir(path, 0777) != 0)

c_sourcecode/src/main.c:394

• /tmp/crash_reboot is created with 0600 permissions, while the legacy script path uses touch (typically resulting in 0644 subject to umask). If other processes/users need to detect/read this flag, 0600 can cause operational issues; consider using 0644-equivalent permissions.

        int fp = open("/tmp/crash_reboot", O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);

unittest/mainapp_gtest.cpp:167

• system_initialize() now returns ERR_SYSTEM_INIT_FAILED on open() failure (as asserted here), but there’s still an earlier inline comment in this test describing the old return -1 behavior, and the SystemInitialize_ConfigInitLoadFailure / SystemInitialize_PlatformInitializeFailure tests above no longer match the new return-code semantics. Please update those comments/assertions to reflect the new behavior so the suite actually validates the failure paths.

    // open() on a directory fails -> system_initialize returns ERR_SYSTEM_INIT_FAILED
    EXPECT_EQ(result, ERR_SYSTEM_INIT_FAILED);

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

unittest/mainapp_gtest.cpp:167

• This test’s description still says system_initialize() returns -1 on open() failure, but the implementation now returns ERR_SYSTEM_INIT_FAILED. Updating the comment will keep the test intent accurate.

    // open() on a directory fails -> system_initialize returns ERR_SYSTEM_INIT_FAILED
    EXPECT_EQ(result, ERR_SYSTEM_INIT_FAILED);

c_sourcecode/src/init/system_init.c:52

• system_initialize() now has a dedicated failure branch when config_init_load() != CONFIG_SUCCESS, but the unit tests don’t assert the returned error code for this path (only a printf). Add/adjust a test to expect ERR_SYSTEM_INIT_FAILED for the config-init failure case.

    if (config_init_load(config, argc, argv) != CONFIG_SUCCESS)
    {
        CRASHUPLOAD_ERROR("config_init_load failed\n");
        t2Uninit();
        return ERR_SYSTEM_INIT_FAILED;

c_sourcecode/src/init/system_init.c:77

• system_initialize() now returns ERR_PLATFORM_INIT_FAILED when platform_initialize() fails, but there’s no unit test assertion covering this return code. Add/adjust a test to expect ERR_PLATFORM_INIT_FAILED for the platform-init failure case.

    if (platform_initialize(config, platform) != PLATFORM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("platform_initialize failed\n");
        t2Uninit();
        return ERR_PLATFORM_INIT_FAILED;

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (3)

c_sourcecode/src/init/system_init.c:78

• system_initialize() calls t2Uninit() on platform_initialize failure, but main_test()/main.c also calls t2Uninit() when system_initialize() returns failure (main.c:151-153). This duplicates teardown and risks double-uninit side effects. Choose a single teardown owner for telemetry on init failures.

    if (platform_initialize(config, platform) != PLATFORM_INIT_SUCCESS)
    {
        CRASHUPLOAD_ERROR("platform_initialize failed\n");
        t2Uninit();
        return ERR_PLATFORM_INIT_FAILED;
    }

c_sourcecode/src/init/system_init.c:64

• system_initialize() calls t2Uninit() when open() fails, but the caller (main_test()/main.c) also calls t2Uninit() on system_initialize() failure (main.c:151-153). This can lead to a double-uninit of telemetry. Consolidate telemetry teardown in one place (either in system_initialize() or in the caller), but not both.

        if (fd < 0)
        {
            CRASHUPLOAD_ERROR("open failed\n");
            t2Uninit();
            return ERR_SYSTEM_INIT_FAILED;
        }

c_sourcecode/src/init/system_init.c:53

• system_initialize() calls t2Uninit() on config_init_load failure, but main_test()/main.c also calls t2Uninit() when system_initialize() returns failure (main.c:151-153). This creates an unbalanced/double-uninit path that can break telemetry state depending on the implementation of t2Uninit(). Prefer having exactly one owner for telemetry teardown on this failure path.

This issue also appears in the following locations of the same file:

• line 59
• line 73

    if (config_init_load(config, argc, argv) != CONFIG_SUCCESS)
    {
        CRASHUPLOAD_ERROR("config_init_load failed\n");
        t2Uninit();
        return ERR_SYSTEM_INIT_FAILED;
    }