Rebase

[adrojaankur]

No description provided.

[Copilot]

Pull request overview

This pull request performs a significant refactoring of the RDK Logger library, modernizing the codebase and improving its architecture.

Changes:

• Refactored logging infrastructure to use log4c more effectively with improved category management and thread safety
• Added extensive unit test coverage including rotation, performance, error handling, and configuration tests
• Removed deprecated dynamic logger test and simplified build configuration

Reviewed changes

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

Show a summary per file

File	Description
utils/rdklogctrl.c	Fixed spelling error "Reciever" → "Receiver", simplified log level validation, removed obsolete module validation
utils/rdk_logger_onboard_main.c	Simplified onboard logging by removing intermediate buffer and passing format string directly
utils/Makefile.am	Updated build flags, added LOG4C_LIBS, changed all programs to use += operator, fixed library paths
unittests/*	Added comprehensive test suites for ext_init, rotation, performance, error handling, and configuration
src/rdk_logger_init.c	Major refactor with thread safety (pthread mutex), constructor initialization, and ext_init support
src/rdk_debug_priv.c	Complete rewrite of logging infrastructure with new layout/appender system and improved formatting
src/rdk_dynamic_logger.c	Simplified dynamic logging with removal of negation support and cleaner log level handling
src/rdk_debug.c	Simplified public API, removed complex module number tracking, added level_from_string converter
test/*	Removed deprecated dynamic logger test, updated test program with new ext_init API usage

---

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

[Copilot]

This build script downloads and executes third-party source code (log4c-1.2.3.tar.gz from SourceForge) without any integrity verification, which exposes the CI/build environment to a supply-chain attack if the remote artifact or distribution channel is compromised. An attacker who can tamper with that tarball could gain code execution in your build environment (with access to build secrets and artifacts) when ./configure, make, and make install are run. To mitigate this, pin the dependency to a specific immutable artifact and verify its integrity before building (for example, by checking a known-good cryptographic hash or signature) instead of trusting the remote download implicitly.

[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 doc claims rdklogctrl can disable a level using a ~ prefix (e.g. ~DEBUG), but the updated rdklogctrl implementation no longer accepts ~ and will treat it as an invalid log level. Please either restore ~ handling in the tool/protocol or remove/update this documentation.

Suggested change

	# Disable specific level (~ prefix)
	rdklogctrl myapp LOG.RDK.NETWORK ~DEBUG
	# Disable logging for a module (set level to NONE)
	rdklogctrl myapp LOG.RDK.NETWORK NONE

[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
  '