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.
5 out of 6 committers have signed the CLA.

✅ AravindanNC
✅ nhanasi
✅ Abhinavpv28
✅ Alan-Ryan
✅ venkat0557
❌ naveenkumarhanasi
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[Copilot]

Pull request overview

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

[Copilot]

The cmdData cleanup (free(cmdData->rfcvalue), free(cmdData->command), free(cmdData)) is inside #if !defined(GTEST_ENABLE). When GTEST_ENABLE is set, this function returns true without freeing those heap allocations, leaking memory in unit tests. Keep the systemctl stop guarded, but free cmdData regardless of GTEST_ENABLE.

[Copilot]

rrd_upload_cleanup_source_dir builds a shell command (rm -rf %s) from dir_path and executes it via system(). Since dir_path can ultimately be derived from RFC/issue-type content, this is a command-injection risk and can also break on spaces/shell metacharacters. Please replace this with a non-shell recursive delete (e.g., nftw() / unlinkat() traversal) and validate that dir_path is within the expected /tmp/rrd/ subtree before deleting.

[Copilot]

The lock handling here differs from the existing scripts/uploadRRDLogs.sh (which checks /tmp/.log-upload.pid) and also creates /tmp/.log-upload.lock unconditionally via O_CREAT. Creating the lock file may change behavior for components that rely on file non-existence. Consider aligning with the existing lock convention (or checking both .pid and .lock for backward compatibility) and avoid O_CREAT so a missing lock file is treated as unlocked without side effects.

[Copilot]

--enable-L2support currently sets L2_SUPPORT_FLAG/L2_SUPPORT_ENABLE but neither is used later (no AC_SUBST, no AM_CONDITIONAL, and src/Makefile.am doesn’t reference the flag). As-is, the option has no effect on the build. Please either wire L2_SUPPORT_FLAG into the relevant CFLAGS via AC_SUBST/Makefile.am (and/or add an AM_CONDITIONAL), or remove the option to avoid a misleading configure interface.

Suggested change

	AC_ARG_ENABLE([L2support],
	AS_HELP_STRING([--enable-L2support],[enable L2support (default is no)]),
	[
	case "${enableval}" in
	yes) L2_SUPPORT_ENABLE=true
	L2_SUPPORT_FLAG="-DUSE_L2_SUPPORT"
	m4_if(m4_sysval,[0],[SUBDIRS_L2_SUPPORT="src"]) ;;
	no) L2_SUPPORT_ENABLE=false AC_MSG_ERROR([L2_SUPPORT is disabled]) ;;
	*) AC_MSG_ERROR([bad value ${enableval} for --enable-L2support]) ;;
	esac
	],
	[echo "L2support is disabled"])
	# Note: legacy --enable-L2support option removed because it had no effect on the build.

[Copilot]

This test mutates /etc/rrd/remote_debugger.json at import time (module top-level). That runs before pytest collection/fixtures, makes the test order-dependent, and can fail in environments where /etc is read-only. Please move this JSON update into a test/fixture (e.g., setup/teardown that backs up and restores the original file), or write to a temporary config path and point the component/test to that path.

[Copilot]

The create_json.sh invocation swallows failures: on CalledProcessError it only prints stderr and the test still passes. This can hide setup failures and make the test results misleading. Please fail the test on error (e.g., re-raise or assert with stderr) and/or assert on expected file output from the script.

[Copilot]

SCRIPT_SUCCESS/SCRIPT_FAILURE constants are swapped: SCRIPT_SUCCESS is set to the "...upload Failed" log string and then prints "Script execution success" when that failure string is found (and vice versa). This makes the test output misleading. Please rename or swap the constants/branches so success maps to the success log message.

[Copilot]

pIssueNode (and its Node/subNode fields allocated by getIssueInfo()) is no longer freed after processing. This introduces a per-event memory leak. Please restore cleanup (free pIssueNode->Node, pIssueNode->subNode, then pIssueNode) once processing is complete.

[Copilot]

Pull request overview

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

[Copilot]

Under GTEST_ENABLE, this path returns without freeing cmdData / cmdData->rfcvalue / cmdData->command, which will leak in unit tests (and can hide real leaks). Only the systemctl calls should be conditional; the frees should run unconditionally.

[Copilot]

The daemon log emitted by uploadDebugoutput is "Starting Upload Debug output via API..." (with ellipsis). This assertion uses "Starting Upload Debug output via API" and will never match, causing false failures. Update the expected string to match the actual log message (or loosen the match to a stable substring).

[Copilot]

The daemon log emitted by uploadDebugoutput is "Starting Upload Debug output via API..." (with ellipsis). This assertion uses "Starting Upload Debug output via API" and will not match. Update the expected string (or match a stable substring) to avoid failing the test due to punctuation differences.

[Copilot]

The daemon log emitted by uploadDebugoutput is "Starting Upload Debug output via API..." (with ellipsis). This assertion expects the string without the ellipsis and will not match. Update the expected log string (or search for a stable substring) to prevent false failures.

[Copilot]

If remotedebugger fails to start, pid1 can be an empty list and pid1[0] will raise IndexError, obscuring the real failure. Add an explicit assertion that pid1 is non-empty after starting (with a helpful message) before indexing it.

[Copilot]

rrd_upload_cleanup_source_dir builds an rm -rf command and executes it via system(). Even if dir_path is expected to be internal, this is still command-injection prone and risky for production code. Prefer a direct recursive delete implementation (e.g., nftw()/unlinkat() traversal) and/or strict validation that dir_path is under the expected /tmp/rrd/ prefix before deletion.

[Copilot]

The daemon log emitted by uploadDebugoutput is "Starting Upload Debug output via API..." (with ellipsis). This assertion uses "Starting Upload Debug output via API" and will not match, making the test flaky/always-failing depending on implementation. Align the expected string with the actual log output.

[Copilot]

Spelling: "shopuld" should be "should" in the scenario step text.

[Copilot]

Spelling: "shopuld" should be "should" in the scenario step text.

[Copilot]

Adding -fpermissive to unit-test builds will downgrade many C++ compile errors to warnings and can hide real type/safety issues. It would be better to fix the underlying compilation problems so tests compile cleanly without -fpermissive.

[Copilot]

Pull request overview

Copilot reviewed 83 out of 84 changed files in this pull request and generated 15 comments.

Comments suppressed due to low confidence (1)

src/rrdEventProcess.c:345

• The previous fix that freed pIssueNode here has been removed, leaving only the dangling comment //CID-336989: Resource leak. pIssueNode is malloced at line 188 and there is no other code path in this function that frees it, so every call to processIssueType() now leaks one issueNodeData allocation (plus any internal buffers populated by getIssueInfo). If the removal was needed because a callee (e.g., processIssueTypeInStaticProfile, processIssueTypeInDynamicProfile, checkIssueNodeInfo) takes ownership of the node, that needs to be done consistently across all branches; otherwise restore the free here.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 89 out of 90 changed files in this pull request and generated 18 comments.

Comments suppressed due to low confidence (1)

src/rrdRunCmdThread.c:423

• Wrapping the free() calls for cmdData->rfcvalue, cmdData->command and cmdData itself in #if !defined(GTEST_ENABLE) means these allocations leak under the gtest build. If the intent was only to prevent double-free interactions with the mocked systemd path in tests, prefer freeing unconditionally and instead make the test harness not allocate ownership-transferring cmdData here, or guard only the systemd v_secure_system call. As written, gtest runs accumulate leaks that valgrind/CI coverage will report.