rebase

[Abhinavpv28]

No description provided.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
6 out of 8 committers have signed the CLA.

✅ Saranya2421
✅ nhanasi
✅ AravindanNC
✅ Abhinavpv28
✅ venkat0557
✅ Alan-Ryan
❌ Saranya
❌ naveenkumarhanasi

---

Saranya seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

Pull request overview

This PR performs a rebase containing substantial enhancements to the Remote Debugger component, including new test coverage for deepsleep and dynamic profile scenarios, code quality improvements addressing Coverity issues, and comprehensive documentation additions.

Key Changes:

• Added L2 test coverage for dynamic subcategory reports, deepsleep scenarios, and append functionality
• Fixed multiple Coverity issues including resource leaks and buffer handling improvements
• Added extensive documentation for uploadRRDLogs component migration to C
• Updated CI/CD workflows and build configurations

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
test/functional-tests/tests/test_rrd_single_instance.py	Enhanced PID handling for instance checking with better parsing
test/functional-tests/tests/test_rrd_dynamic_subcategory_report.py	New L2 tests for dynamic subcategory report validation
test/functional-tests/tests/test_rrd_deepsleep_static_report.py	New L2 tests for deepsleep static profile handling
test/functional-tests/tests/test_rrd_append_report.py	New L2 tests for append request functionality
test/functional-tests/tests/power_controller.h	New header defining PowerController API interfaces
test/functional-tests/tests/deepsleep_main.c	New C test file for deepsleep event handling
test/functional-tests/tests/create_json.sh	Updated JSON generation with additional test entries
test/functional-tests/tests/Makefile	New makefile for building deepsleep test binary
test/functional-tests/features/*.feature	New feature files for BDD test scenarios
src/rrdRunCmdThread.c	Fixed systemd service handling and conditional compilation
src/rrdJsonParser.c	Fixed resource leak and added deepsleep validation logic
src/rrdInterface.h/c	Updated IARM support conditionals and buffer handling fix
src/rrdIarmEvents.c	Refactored WebCfg and IARM event handling logic
src/rrdEventProcess.c	Fixed memory management and added null checks
src/rrdDynamic.c	Fixed buffer size calculation for deep sleep events
src/unittest/rrdUnitTestRunner.cpp	Extensive unit test updates with new test cases
src/unittest/mocks/*.h/cpp	Updated mock implementations for RBus and IARM
remote_debugger.json	Added DeepSleep configuration entries
docs/uploadRRDLogs_*.md	New comprehensive documentation for C migration
run_l2.sh	Added new L2 test executions
cov_build.sh	Updated build configuration with new flags
configure.ac	Added L2 support configuration option
CHANGELOG.md	Updated with recent release information
.github/workflows/*.yml	Updated CI/CD workflows and permissions
.github/CODEOWNERS	Updated code ownership configuration

---

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

[Copilot]

The success and failure message constants are swapped. SCRIPT_SUCCESS is set to "upload Failed" and SCRIPT_FAILURE is set to "upload Success", which is backwards and will cause confusing log messages.

[Copilot]

The success and failure message constants are swapped. SCRIPT_SUCCESS is set to "upload Failed" and SCRIPT_FAILURE is set to "upload Success", which is backwards and will cause confusing log messages.

[Copilot]

Typo in comment: "shopuld" should be "should".

Suggested change

	And Command output shopuld be added to the output file
	And Command output should be added to the output file

[Copilot]

Typo in comment: "shopuld" should be "should".

Suggested change

	And Command output shopuld be added to the output file
	And Command output should be added to the output file

[Copilot]

Variable issue_string is not used.

[Copilot]

Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.

[Copilot]

Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.

[Copilot]

Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.

[Copilot]

Import of 'json' is not used.

[Copilot]

Pull request overview

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

---

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

[Copilot]

Variable node is not used.

[Copilot]

Variable subnode is not used.

[Copilot]

Variable ISSUE_FOUND is not used.

[Copilot]

Import pollutes the enclosing namespace, as the imported module helper_functions does not define 'all'.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (10)

test/functional-tests/tests/test_rrd_deepsleep_static_report.py:1

• subprocess is used but never imported in this file, which will raise NameError at runtime. Add import subprocess (and any other required stdlib imports) at the top of the test module.
  test/functional-tests/tests/test_rrd_c_api_upload.py:1
• subprocess is used but not imported in this new test file, which will fail at runtime. Add import subprocess at the top of the file.
  test/functional-tests/tests/test_rrd_append_report.py:1
• This test module mutates /etc/rrd/remote_debugger.json at import time (before pytest collects/runs tests), which can cause non-deterministic failures and pollute the test environment. Move this mutation into a test setup/fixture with a clear teardown (restore the original file), or write to a test-scoped copy (e.g., under /tmp) and point the system-under-test at that copy.
  test/functional-tests/tests/deepsleep_main.c:1
• RRDEventThreadFunc loops forever and only breaks on msgrcv error, but main() unconditionally pthread_joins the thread. As written, the test harness can hang indefinitely. Add a termination mechanism (e.g., a shared atomic “stop” flag + msgrcv with timeout / non-blocking + periodic check, or cancel/detach the thread and exit cleanly after the events are injected).
  src/rrd_upload.c:1
• UploadSTBLogsParams/TRIGGER_ONDEMAND come from uploadstblogs.h, but src/rrd_upload.h excludes that include when GTEST_ENABLE is defined. If rrd_upload.c is compiled with -DGTEST_ENABLE, this will not compile. Consider providing a small local typedef/enum shim under #ifdef GTEST_ENABLE (in a C header used by rrd_upload.c), or include uploadstblogs.h unconditionally for builds that compile this file.
  src/rrd_upload.c:1
• Calling system("rm -rf %s") with an unquoted path is shell-injection prone if dir_path ever contains whitespace or shell metacharacters. Replace this with a safe recursive delete implementation (e.g., nftw + unlink/rmdir, or openat/unlinkat walking), or at minimum use an execv-style call to /bin/rm with -- and an argument vector (no shell).
  src/uploadRRDLogs.c:1
• This PR introduces a substantial new orchestration surface with multiple distinct error codes (config load, MAC/timestamp, dir validation, sanitize, archive create, upload, cleanup). Add gtest unit coverage that asserts the returned error codes and key log/behavior for the main failure paths (e.g., invalid params → 1, config failure → 3, empty dir → 6, archive failure → 10, upload failure → 11) using mocks for sysinfo/upload and temp directories for filesystem behavior.
  src/rrd_sysinfo.h:1
• The header comment says the MAC buffer needs “min 18 bytes”, but the implementation strips colons and only requires 12 hex chars + terminator (13 bytes). Update the documentation (or the implementation) so the required minimum buffer size is consistent.
  test/functional-tests/tests/power_controller.h:1
• Correct the spelling in the enum comment: 'Critial' → 'Critical'.
  test/functional-tests/features/rrd_dynamic_profile_subcategory_report.feature:1
• Fix typo in the scenario step: 'shopuld' → 'should'.

[Copilot]

When GTEST_ENABLE is defined, cmdData->rfcvalue, cmdData->command, and cmdData are no longer freed, introducing a guaranteed memory leak on the gtest path. Keep memory cleanup outside the #if !defined(GTEST_ENABLE) guard and only gate the systemctl calls.

[Copilot]

This strncpy call can produce a non-null-terminated string when truncated, and the computed bound (msgDataStringSize - strlen(RDM_PKG_SUFFIX)) can underflow if msgDataStringSize is smaller than strlen(RDM_PKG_SUFFIX). Switch to snprintf (or ensure the bound is validated and always explicitly null-terminate msgDataString).

Suggested change

	strncpy(msgDataString, paramString, (msgDataStringSize - strlen(RDM_PKG_SUFFIX)));
	{
	size_t paramLen = strlen(paramString);
	size_t suffixLen = strlen(RDM_PKG_SUFFIX);
	if (paramLen > suffixLen && msgDataStringSize > 0)
	{
	size_t baseLen = paramLen - suffixLen;
	if (baseLen >= msgDataStringSize)
	{
	baseLen = msgDataStringSize - 1;
	}
	/* Copy paramString without the RDM_PKG_SUFFIX, ensuring null termination */
	snprintf(msgDataString, msgDataStringSize, "%.*s", (int)baseLen, paramString);
	}
	else if (msgDataStringSize > 0)
	{
	msgDataString[0] = '\0';
	}
	}

[Copilot]

USTAR prefix/name splitting must occur on a directory separator boundary (and prefix should contain the directory portion). Splitting purely by character count can generate invalid headers that some tar implementations won’t extract correctly. Split on the last '/' that keeps prefix <= 155 and name <= 100, and fail cleanly if no valid split exists.

Suggested change

	if (name_len <= 100) {
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name */
	size_t prefix_len = name_len - 100 - 1; /* leave room for null */
	if (prefix_len > sizeof(hdr.prefix)) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s] File name too long: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	strncpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len < sizeof(hdr.prefix) ? prefix_len : sizeof(hdr.prefix) - 1] = '\0';
	strncpy(hdr.name, name + prefix_len + 1, sizeof(hdr.name) - 1);
	if (name_len <= sizeof(hdr.name)) {
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name on a directory separator boundary */
	size_t prefix_len = 0;
	const char *name_part = NULL;
	/* search for the last '/' that yields a valid prefix and name */
	for (size_t i = name_len; i > 0; --i) {
	if (name[i - 1] == '/') {
	size_t p_len = i - 1; /* directory portion, without trailing '/' */
	size_t n_len = name_len - i; /* name portion after '/' */
	if (p_len > 0 &&
	p_len <= sizeof(hdr.prefix) &&
	n_len > 0 &&
	n_len <= sizeof(hdr.name)) {
	prefix_len = p_len;
	name_part = name + i;
	break;
	}
	}
	}
	if (name_part == NULL) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s] File name too long: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	/* copy prefix (directory part) */
	memcpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len] = '\0';
	/* copy name (final path component) */
	strncpy(hdr.name, name_part, sizeof(hdr.name) - 1);

[Copilot]

Pull request overview

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

[Copilot]

This test module performs file I/O and mutates /etc/rrd/remote_debugger.json at import time. Pytest will execute this on collection, which can make test runs order-dependent and can affect other tests (and local environments) even if these tests are skipped/xfail. Move this JSON read/update/write into a dedicated test/fixture (e.g., setup function) and ensure the original file contents are restored in teardown/finalizer.

[Copilot]

SCRIPT_SUCCESS / SCRIPT_FAILURE constants are swapped: SCRIPT_SUCCESS is set to the "...upload Failed" message and SCRIPT_FAILURE to the "...upload Success" message, and the printed status strings are inverted as well. This will report success on failure and vice versa. Swap the constants (or adjust the condition/print statements) so the labels match the expected log lines.

[Copilot]

The CalledProcessError from running create_json.sh is caught and only printed, so the test will continue and potentially pass even when the script fails. Re-raise the exception or fail the test (e.g., assert False) in the except block so environment setup failures are not silently ignored.

[Copilot]

rrd_upload_cleanup_source_dir builds an "rm -rf" shell command with an unquoted dir_path and executes it via system(). Since dir_path is derived from issue/category values that ultimately come from external input, this is vulnerable to command injection and also breaks paths containing spaces/shell metacharacters. Prefer a safe recursive delete using nftw()/unlinkat() (or, at minimum, execv("rm", {"rm","-rf","--",dir_path,...}) without invoking a shell).

[Copilot]

In the GTEST_ENABLE build, this block skips freeing cmdData->rfcvalue/cmdData->command/cmdData entirely, which introduces a leak in unit tests and makes behavior differ between test and production builds. Consider guarding only the systemctl calls with GTEST_ENABLE, but keep the memory cleanup outside the conditional.

[Copilot]

The ustar name/prefix split logic for paths 101–255 chars is incorrect: it splits at an arbitrary offset (name_len - 101) rather than at the last '/' boundary. This can produce invalid tar headers and lose path information. Split the path at the last '/' such that prefix <= 155 and name <= 100 per ustar rules, or reject names that can’t be represented correctly.

Suggested change

	if (name_len <= 100) {
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name */
	size_t prefix_len = name_len - 100 - 1; /* leave room for null */
	if (prefix_len > sizeof(hdr.prefix)) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "[%s] File name too long: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	strncpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len < sizeof(hdr.prefix) ? prefix_len : sizeof(hdr.prefix) - 1] = '\0';
	strncpy(hdr.name, name + prefix_len + 1, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	if (name_len <= sizeof(hdr.name)) {
	/* simple case: whole path fits in name field */
	strncpy(hdr.name, name, sizeof(hdr.name) - 1);
	hdr.name[sizeof(hdr.name) - 1] = '\0';
	} else if (name_len <= 255) {
	/* split into prefix and name according to POSIX ustar rules:
	* - prefix contains the directory path (no trailing '/')
	* - name contains the final path component
	* - both fields must fit in their respective header buffers
	*/
	size_t prefix_len = 0;
	size_t name_part_len = 0;
	int split_found = 0;
	/* search for last '/' that yields valid prefix and name lengths */
	for (size_t i = name_len; i > 0; --i) {
	if (name[i - 1] == '/') {
	prefix_len = i - 1; /* up to but not including this '/' */
	name_part_len = name_len - i; /* after '/' to end */
	if (prefix_len > 0 &&
	prefix_len <= sizeof(hdr.prefix) &&
	name_part_len > 0 &&
	name_part_len <= sizeof(hdr.name)) {
	split_found = 1;
	break;
	}
	}
	}
	if (!split_found) {
	RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG,
	"[%s] File name too long or invalid for ustar: %s (length %zu)\n",
	__FUNCTION__, name, name_len);
	return -1;
	}
	/* copy prefix (directory path) */
	memcpy(hdr.prefix, name, prefix_len);
	hdr.prefix[prefix_len] = '\0';
	/* copy final path component */
	memcpy(hdr.name, name + prefix_len + 1, name_part_len);
	hdr.name[name_part_len] = '\0';

[Copilot]

rrd_upload.h declares rrd_upload_invoke_logupload_api() and rrd_upload_cleanup(), but there are no implementations in the source tree. Since this header is installed as part of the public include set, this creates an API contract that will fail at link time for consumers. Either implement these functions or remove them from the public header until they exist.

[Copilot]

rrd_log.h declares rrd_log_init(), but there is no implementation in the repository. If this is intended as a public API, it should be implemented; otherwise, remove the declaration to avoid unresolved symbol/link-time failures for any consumer that includes this installed header.

[Copilot]

Typo in step text: "shopuld" should be "should".

[Copilot]

Typo in step text: "shopuld" should be "should".