[adrojaankur]

No description provided.

[Copilot]

Pull request overview

Copilot reviewed 71 out of 71 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.

[Copilot]

Pull request overview

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

---

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

[Copilot]

rdk_logger_deinit() closes the dynamic logger socket but never resets isLogInited (and doesn't call any logger/log4c deinit). After a deinit(), subsequent rdk_logger_init() calls will be a no-op, leaving dynamic logging disabled and preventing reconfiguration. Either (a) make deinit symmetrical (call rdk_dbg_priv_deinit(), set isLogInited=false, and clean up resources), or (b) if deinit is intentionally a no-op, avoid closing the dynamic logger socket and/or document that deinit does not permit re-init.

[Copilot]

The rdk_logger_ext_config_t example here uses fields like .fileName, .logdir, .maxSize, .maxCount that don't exist in the current public struct (it now uses output, format, and pFilePolicy). This example won't compile and will mislead users—please update it to match the actual API.

[Copilot]

This test case uses ~NONE as a log level for rdklogctrl, but rdklogctrl no longer supports ~-prefixed levels. As written, the command will fail (and the test currently ignores the system() return code), so it isn't exercising the intended behavior. Update the test to use supported levels and assert on the command exit status/output (or reintroduce ~ support if it's required).

[Copilot]

Header guard macro is inconsistent: #ifndef _RDK_DEBUG_H but #define _RDK_DEBUG_H_. This defeats the include guard and can cause multiple-definition / redefinition issues on repeated includes. Use the same macro name in both directives (and in the closing #endif comment).

Suggested change

	#ifndef _RDK_DEBUG_H
	#ifndef _RDK_DEBUG_H_

[Copilot]

RDK_LOGGER_INIT() macro includes a trailing semicolon, which breaks common usage patterns like if (RDK_LOGGER_INIT() != RDK_SUCCESS) { ... } (as shown in the docs). Define it as a pure expression (no trailing ;) and consider wrapping the whole ternary in parentheses for safe use in larger expressions.

Suggested change

	#define RDK_LOGGER_INIT() (0 == access(DEBUG_INI_OVERRIDE_PATH_1, F_OK)) \
	? rdk_logger_init(DEBUG_INI_OVERRIDE_PATH_1) \
	: (0 == access(DEBUG_INI_OVERRIDE_PATH_2, F_OK)) \
	? rdk_logger_init(DEBUG_INI_OVERRIDE_PATH_2) \
	: rdk_logger_init(DEBUG_INI_NAME);
	#define RDK_LOGGER_INIT() ((0 == access(DEBUG_INI_OVERRIDE_PATH_1, F_OK)) \
	? rdk_logger_init(DEBUG_INI_OVERRIDE_PATH_1) \
	: ((0 == access(DEBUG_INI_OVERRIDE_PATH_2, F_OK)) \
	? rdk_logger_init(DEBUG_INI_OVERRIDE_PATH_2) \
	: rdk_logger_init(DEBUG_INI_NAME)))

[Copilot]

isLogInited is a non-static global symbol. Since it is only used to protect internal initialization state, keeping it static avoids exporting an unnecessary symbol from the shared library and prevents accidental external linkage/ABI coupling.

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/rdk_logger/45/rdk/components/generic/rdk_logger

• Commit: 5ccaad2

Report detail: gist'

[Copilot]

Pull request overview

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

Suppressed comments (12)

include/rdk_debug.h:26

• Header guard macro is inconsistent: the file checks _RDK_DEBUG_H but defines _RDK_DEBUG_H_, so the guard will not work as intended.

#ifndef _RDK_DEBUG_H
#define _RDK_DEBUG_H_

src/rdk_logger_init.c:121

• rdk_logger_deinit() closes the dynamic logger socket but never resets isLogInited or deinitializes the underlying debug subsystem. After a deinit, subsequent rdk_logger_init() calls will not reconfigure or restart dynamic logging, and the logger state is left partially initialized.
  utils/rdklogctrl.c:113
• app_len and comp_len are written into a fixed 128-byte packet buffer without validating bounds. Long argv values can overflow buf and also truncate lengths into a single byte, producing malformed packets.
  docs/QUICK_REFERENCE.md:19
• The rdk_logger_ext_config_t example uses fields (fileName, logdir, maxSize, maxCount) that do not exist in the new public struct. This example won’t compile for users following the quick reference.

// Extended initialization with file logging
rdk_logger_ext_config_t config = {
    .fileName = "app.log",
    .logdir = "/var/log/",
    .maxSize = 1024*1024,    // 1MB

docs/QUICK_REFERENCE.md:88

• The quick reference documents rdklogctrl ... ~DEBUG to disable a specific log level, but utils/rdklogctrl.c no longer accepts the ~ prefix. This will mislead users.

# Disable specific level (~ prefix)
rdklogctrl myapp LOG.RDK.NETWORK ~DEBUG

unittests/rdkLoggerDynamic.cpp:117

• This test generates the log level string ~NONE, but rdklogctrl no longer supports the ~ prefix. The system() call will fail and the test won't actually exercise the intended dynamic logger behavior.
  src/rdk_dynamic_logger.c:42
• rdk_dyn_log_logLevelToString returns string literals; the return type should be const char * to avoid discarding const qualifiers.
  src/rdk_logger_init.c:66
• When debugConfigFile is NULL, the implementation falls back to DEBUG_CONF_FILE (currently built as a relative debug.ini). This is inconsistent with RDK_LOGGER_INIT() in include/rdk_logger.h (which selects /opt, /nvram, then /etc) and makes rdk_logger_init(NULL) unreliable in production and tests.
  src/rdk_debug.c:118
• rdk_logger_enable_logLevel() ignores the enableLogLvl parameter, so callers cannot actually disable a log level as the API contract implies. This is a behavioral change for legacy consumers and should be either implemented consistently or the API/docs updated to reflect the new semantics.
  utils/rdklogctrl.c:43
• validate_loglevel() uses strncmp() with fixed lengths, so values like ERROR123 are accepted as valid. It also doesn't accept the common spelling WARNING even though other parts of the code/docs use it.
  unittests/test_utils.h:20
• After fork(), the child should use _exit() rather than exit() to avoid running parent atexit handlers / flushing shared stdio buffers twice. This can cause flaky test behavior.
  unittests/rdkLoggerConfigTest.cpp:132
• This test expects rdk_logger_init(NULL) to succeed, but unittests/rdkloggerInit.cpp now asserts the opposite and the current implementation falls back to a relative debug.ini which typically won’t exist in the test working directory. The tests are inconsistent and this one will likely fail.

[rdkcmf-jenkins]

b'## WARNING: A Blackduck scan failure has been waived

A prior failure has been upvoted

• Upvote reason: OK

• Commit: 5ccaad2
  '

[Copilot]

🔵 Needs a closer look

Public initialization and level-control APIs are broken, lifecycle reinitialization fails, and the test harness currently masks failures.

2 open findings

• Packet-derived lengths are used to copy/compare strings without bounds checks: `memcmp(...,…
• Adding -Werror will turn existing warnings in compiled sources into build failures (e.g.,…

Previously missed (5)

In code that hasn't changed since last review

enableLogLvl parameter is ignored

src/​rdk_debug.c:116

enableLogLvl is ignored, so passing FALSE still sets the requested level instead of disabling it. This breaks the retained public API and the examples that rely on enable/disable behavior; either implement both states in the backend or remove/migrate this API and its callers.

RDK_LOG_NONE maps to the wrong Log4C priority

src/​rdk_dynamic_logger.c:75

RDK_LOG_NONE is accepted here and forwarded to rdk_dbg_priv_log_reconfig, where it maps to LOG4C_PRIORITY_UNKNOWN; configuration parsing uses LOG4C_PRIORITY_NONE instead. Consequently the advertised rdklogctrl ... NONE path does not disable the category and can enable valid severities under Log4C's priority ordering.

Test runner always reports success

unittests/​main.cpp:35

The test runner always returns success even when RUN_ALL_TESTS() reports failures, so the workflow cannot fail on a broken test suite. Return the GoogleTest result after printing the diagnostic.

Child test failures are not propagated

unittests/​test_utils.h:19

The child always exits with status 0, so nonfatal GoogleTest failures in test_body are discarded and the parent assertion passes. Fatal ASSERT_* calls are also unsafe here because they can return before this exit; use GoogleTest death/subprocess support or explicitly propagate child failures.

Priority list contradicts logger initialization fallback order

docs/​QUICK_REFERENCE.md:72

This priority list contradicts RDK_LOGGER_INIT(), which checks /opt/debug.ini first, then /nvram/debug.ini, then /etc/debug.ini. A custom path passed directly to rdk_logger_init() is not the third fallback in that chain.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Unresolved CI failures, runtime-control defects, API compilation errors, unreliable tests, and incorrect documentation block approval.

13 open issues

• Create /opt/logs with sudo before writing logs · New
• Make runtime RDK_LOG_NONE suppress logging · New
• Make child PID rebinding mutex fork-safe · New
• Adding -Werror will turn existing warnings in compiled sources into build failures (e.g.,…
• Preserve enableLogLvl compatibility behavior · New
• Unlink pathname sockets during logger cleanup · New
• Reset logger state between configuration parsing tests · New
• Assert the configured file appender attachment · New
• Propagate child GoogleTest failures to the parent · New
• Use numeric PIDs with rdklogctrl commands · New
• Document correct logger configuration search precedence · New
• Update CLI guidance for disabling DEBUG and TRACE · New
• Avoid logging and cleanup from signal handlers · New

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unresolved build, test-registration/install, CI, runtime-control, API, test-reliability, and documentation defects block approval.

16 open issues

• Retain -pthread in target-specific Automake flags · New
• Install configuration using an absolute source path · New
• Restore GTest discovery with the correct working directory · New
• Make child PID rebinding mutex fork-safe
• Make runtime RDK_LOG_NONE suppress logging
• Create /opt/logs with sudo before writing logs
• Adding -Werror will turn existing warnings in compiled sources into build failures (e.g.,…
• Propagate child GoogleTest failures to the parent
• Assert the configured file appender attachment
• Reset logger state between configuration parsing tests
• Unlink pathname sockets during logger cleanup
• Preserve enableLogLvl compatibility behavior
• Avoid logging and cleanup from signal handlers
• Update CLI guidance for disabling DEBUG and TRACE
• Document correct logger configuration search precedence
• Use numeric PIDs with rdklogctrl commands

2 previously missed issues

In code that hasn't changed since last review

Isolate tests from shared logger singleton state

unittests/​rdkLoggerErrorTest.cpp:45

This cleanup does not reset the logger singleton: rdk_logger_deinit() leaves isLogInited and the existing categories intact. The fixture's subsequent rdk_logger_init(test.ini) calls therefore return success without loading the INFO/DEBUG configuration created in SetUp, and the full runner inherits configuration and level changes from earlier suites. Isolate these cases before logger initialization so the error paths run under their intended configuration rather than shared, order-dependent state.

Verify the attached console appender

unittests/​test_rdkloggerExtInit.cpp:70

Extended initialization attaches LOG.RDK.print, not LOG.RDK.stdout. Since log4c_appender_get() creates missing appenders, this assertion succeeds on a new, unattached object and cannot detect a missing console appender. Read the appender attached to category_name rather than creating one with the expected name.

_{Review effort: Balanced}