Skip to content

RDKB-63306: cujo-agent agent.txt - add log rotation #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sowmiyachelliah wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

RDKB-63306: cujo-agent agent.txt - add log rotation #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
8a494a1
cujo-agent agent.txt - add log rotation
sowmiyachelliah Mar 2, 2026
8e88ea3
Updating ssp_main.c
sowmiyachelliah Mar 2, 2026
9750e24
Updating ssp_main.c
sowmiyachelliah Mar 2, 2026
0ffc625
Updating ssp_main.c
sowmiyachelliah Mar 3, 2026
0080884
Updating ssp_main.c
sowmiyachelliah Mar 4, 2026
9d5c66a
Updating ssp_main.c
sowmiyachelliah Mar 4, 2026
1868a67
Update ssp_main.c
sowmiyachelliah Mar 5, 2026
ffd7edb
Adding log rotate code in cosa_adv_security_internal.c
sowmiyachelliah Apr 4, 2026
0034e57
Adding libraries in Makefile.am
sowmiyachelliah Apr 4, 2026
62357f5
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 4, 2026
942b1a0
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 4, 2026
f6a1256
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 6, 2026
73fd38c
Update cosa_adv_security_internal.c
sowmiyachelliah Apr 6, 2026
2136965
cosa_adv_security_internal.c
sowmiyachelliah Apr 6, 2026
e8a8fa6
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 6, 2026
760dd0b
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 7, 2026
6b6d962
Update cosa_adv_security_internal.c
sowmiyachelliah Apr 8, 2026
9f9b455
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 10, 2026
bb8ad44
Updating Makefile.am to resolve unit test case failure
sowmiyachelliah Apr 10, 2026
d1d8fff
Updating cosa_adv_security_internal.c and Makefile.am
sowmiyachelliah Apr 10, 2026
eb9ffe4
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 10, 2026
e263014
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 14, 2026
116f39e
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 14, 2026
5529ea6
Updating cosa_adv_security_internal.c
sowmiyachelliah Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/AdvSecurityDml/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ hardware_platform = i686-linux-gnu
lib_LTLIBRARIES = libdmlasecurity.la
libdmlasecurity_la_CPPFLAGS = -I$(top_srcdir)/include -I$(top_srcdir)/source/AdvSecurityDml -I$(top_srcdir)/source/AdvSecuritySsp $(CPPFLAGS) -I$(top_srcdir)/../Utopia/source/include/sysevent -I${PKG_CONFIG_SYSROOT_DIR}$(includedir)/rbus
libdmlasecurity_la_SOURCES = plugin_main.c cosa_adv_security_internal.c cosa_adv_security_dml.c cosa_adv_security_webconfig.c advsecurity_helpers.c advsecurity_param.c
libdmlasecurity_la_LDFLAGS = -lccsp_common -lsyscfg -lsysevent -lwebconfig_framework -lmsgpackc -ltrower-base64 -lsecure_wrapper -lrbus $(SSP_LDFLAGS)
libdmlasecurity_la_LDFLAGS = -lccsp_common -lsyscfg -lsysevent -lwebconfig_framework -lmsgpackc -ltrower-base64 -lsecure_wrapper -lrbus -lev -lpthread $(SSP_LDFLAGS)
Comment on lines 27 to +29
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding -lev in the library link flags introduces a new build dependency on libev. If libev isn’t guaranteed in all build environments, this may break the build; consider adding proper dependency detection (e.g., via configure/pkg-config) and using discovered CFLAGS/LIBS instead of a fixed -lev.

Copilot uses AI. Check for mistakes.

if WIFI_DATA_COLLECTION
libdmlasecurity_la_CPPFLAGS += -DDML_SUPPORT -DNON_PRIVILEGED -DWIFI_DATA_COLLECTION
Expand Down
100 changes: 100 additions & 0 deletions source/AdvSecurityDml/cosa_adv_security_internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
#include "safec_lib_common.h"
#include "secure_wrapper.h"
#include <rbus/rbus.h>
#include <sys/stat.h>
#include <ev.h>
#include <pthread.h>
#if defined(_COSA_BCM_MIPS_)
#include <ccsp/dpoe_hal.h>
#else
Expand Down Expand Up @@ -91,6 +94,13 @@
#define SAFEBRO_CONFIG_FILE_PATH "/tmp/safebro.json"
#define ADVSEC_PRIMARY_WAN_IF_NAME "erouter0"

/* Logrotate configuration for agent.txt */
#define ADVSEC_AGENT_LOG_FILE "/rdklogs/logs/agent.txt"
#define ADVSEC_AGENT_LOG_MAX_SIZE (2 * 1024 * 1024) /* 2MB */
#define ADVSEC_AGENT_LOG_INTERVAL 5.0
#define LOGROTATE_BINARY "/usr/sbin/logrotate"
#define ADVSEC_AGENT_LOGROTATE_CONF "/etc/logrotate.d/advsec-agent"

Comment on lines +97 to +103
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ADVSEC_AGENT_LOGROTATE_CONF points to /etc/logrotate.d/advsec-agent, but no such config appears to be added/installed by this change. Without ensuring this file exists on target devices, rotation will consistently fail. Consider shipping/installing the config as part of this component (or add an existence check with a clear error log).

Copilot uses AI. Check for mistakes.
#ifdef CONFIG_CISCO
#define CONFIG_VENDOR_NAME "Cisco"
#endif
Expand Down Expand Up @@ -153,6 +163,8 @@ static char prevWanIfname[MAX_INTERFACE_SIZE] = {0};

void advsec_handle_sysevent_async(void);
static void advsec_start_logger_thread(void);
static void* agent_log_monitor_thread(void* arg);
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent_log_monitor_thread is forward-declared as static, but its definition later is not static. That’s conflicting linkage in C and typically fails compilation with a “static declaration follows non-static”/“conflicting linkage” error. Make the definition static as well (and consider making the other helper functions in this block static too since they’re file-local).

Suggested change
static void* agent_log_monitor_thread(void* arg);
void* agent_log_monitor_thread(void* arg);

Copilot uses AI. Check for mistakes.
static void advsec_start_log_monitor_thread(void);
static BOOL WaitForLoggerTimeout(ULONG period);
enum advSysEvent_e{
SYSEVENT_BRIDGE_MODE_EVENT,
Expand Down Expand Up @@ -1398,6 +1410,7 @@ CosaSecurityInitialize
rc = strcpy_s(prevWanIfname, sizeof(prevWanIfname), ADVSEC_PRIMARY_WAN_IF_NAME);
ERR_CHK(rc);
advsec_start_logger_thread();
advsec_start_log_monitor_thread();
advsec_handle_sysevent_async();
Comment on lines 1411 to 1414
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log monitor thread is detached and runs ev_run() indefinitely, but there’s no corresponding shutdown path (e.g., in CosaSecurityRemove/COSA_Unload). This can leave a background thread running after the component is unloaded, causing undefined behavior. Add a stop mechanism (ev_break + thread join/cancel) tied to the component lifecycle.

Copilot uses AI. Check for mistakes.
Comment on lines +1413 to 1414
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log monitor thread is started unconditionally during initialization, even when Is_Device_Finger_Print_Enabled() is false (the logger thread is gated, but the monitor thread is not). This creates a detached thread running an event loop for the lifetime of the process even when the feature is disabled; consider gating this start on the same enable condition (or stopping it when disabled).

Suggested change
advsec_start_log_monitor_thread();
advsec_handle_sysevent_async();
if(Value == 1)
{
advsec_start_log_monitor_thread();
advsec_handle_sysevent_async();
}

Copilot uses AI. Check for mistakes.

#ifdef WAN_FAILOVER_SUPPORTED
Expand Down Expand Up @@ -1747,6 +1760,93 @@ static void advsec_start_logger_thread(void)
}
}

/* Log rotation function for agent.txt using logrotate binary */
void rotate_agent_log(void)
{
struct stat st;
int result;
Comment on lines +1763 to +1767
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New log rotation behavior is introduced here but there are existing gtest-based unit tests for this module; consider adding coverage for rotation triggering (size threshold) and the logrotate command execution path (success/failure) to prevent regressions.

Copilot uses AI. Check for mistakes.
Comment on lines +1763 to +1767
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New log-rotation behavior (rotate_agent_log + libev watcher) isn’t covered by the existing gtest suite for this module. Adding unit tests that verify the logrotate command is invoked only when the file exceeds the threshold (with mocks for v_secure_system and stat/wrapper) would help prevent regressions.

Copilot uses AI. Check for mistakes.

if (stat(ADVSEC_AGENT_LOG_FILE, &st) != 0)
{
return;
}
Comment on lines +1769 to +1772
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ADVSEC_AGENT_LOG_FILE is used here but is not defined anywhere in the repo (no #define/declaration found), which will fail compilation. Define the log path constant (e.g., to agent.txt) in an appropriate header or in this file before using it in stat()/ev_stat_init().

Copilot uses AI. Check for mistakes.

if (st.st_size < ADVSEC_AGENT_LOG_MAX_SIZE)
{
return;
}
Comment on lines +1763 to +1777
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New log-rotation behavior (size check + logrotate command execution) is introduced here, but there are existing unit tests for this module under source/test/CcspAdvSecurityDmlTest. Consider adding tests for rotate_agent_log() to verify it only invokes v_secure_system once the size threshold is exceeded and that failures are handled/logged correctly.

Copilot uses AI. Check for mistakes.

CcspTraceInfo(("Agent log reached %ld bytes, calling logrotate...\n", st.st_size));
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

st.st_size is an off_t and isn’t guaranteed to fit %ld (it may be 64-bit on some platforms). This log line should use a format specifier/cast that is correct for off_t (e.g., cast to intmax_t and print with %jd, or use a platform-safe macro) to avoid undefined behavior or incorrect output.

Copilot uses AI. Check for mistakes.

Comment on lines +1778 to +1780
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

st.st_size is an off_t; logging it with %ld is not portable and can be undefined on platforms where off_t is not long. Use an appropriate format (e.g., cast to intmax_t and print with %jd) to avoid incorrect output/UB.

Copilot uses AI. Check for mistakes.
result = v_secure_system("%s %s",
LOGROTATE_BINARY, ADVSEC_AGENT_LOGROTATE_CONF);
if (result != 0)
Comment on lines +1779 to +1783
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new logrotate invocation references ADVSEC_AGENT_LOGROTATE_CONF, but that macro isn't defined anywhere in the repo, so this won’t compile. Also, the command line passed to logrotate appears malformed: /tmp/logrotate-advsec.status is being passed as a positional argument rather than via -s <statefile>, which logrotate expects for the state file. Define the config path macro and update the logrotate command to pass a config file and state file using the correct flags.

Copilot uses AI. Check for mistakes.
{
CcspTraceError(("Logrotate failed with return code: %d\n", result));
}
else
{
CcspTraceInfo(("Logrotate completed successfully\n"));
}
}

/* Callback function for libev stat watcher */
void agent_log_stat_cb(EV_P_ ev_stat *w, int revents)
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent_log_stat_cb is only used within this translation unit but is defined with external linkage. Mark it static to avoid exporting the symbol from the library.

Suggested change
void agent_log_stat_cb(EV_P_ ev_stat *w, int revents)
static void agent_log_stat_cb(EV_P_ ev_stat *w, int revents)

Copilot uses AI. Check for mistakes.
{
(void)loop;
(void)revents;

if (w->attr.st_nlink)
{
rotate_agent_log();
}
}

/* Thread function to run libev event loop for log monitoring */
void* agent_log_monitor_thread(void* arg)
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent_log_monitor_thread is forward-declared as static but defined without static here. This is a storage-class mismatch that can fail compilation under strict flags; make the definition static (or drop static from the prototype) so they match.

Suggested change
void* agent_log_monitor_thread(void* arg)
static void* agent_log_monitor_thread(void* arg)

Copilot uses AI. Check for mistakes.
{
(void)arg;
Comment on lines +1805 to +1808
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new forward declaration marks agent_log_monitor_thread as static (internal linkage), but the definition is non-static. With -Wall -Werror this can fail to compile due to conflicting linkage. Make the definition static (and consider making the other helper functions in this block static as well since they’re file-local).

Copilot uses AI. Check for mistakes.

struct ev_loop *loop = NULL;
static ev_stat stat_watcher;
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stat_watcher is declared static inside agent_log_monitor_thread, which makes it shared across calls/threads and can cause hard-to-debug behavior if the monitor thread is ever started more than once. Prefer a non-static watcher allocated on the thread stack (or heap) so each thread has its own instance.

Suggested change
static ev_stat stat_watcher;
ev_stat stat_watcher;

Copilot uses AI. Check for mistakes.

CcspTraceDebug(("Starting agent log monitor thread\n"));

loop = ev_loop_new(0);
if (!loop)
{
CcspTraceError(("Failed to create libev event loop\n"));
return NULL;
}

ev_stat_init(&stat_watcher, agent_log_stat_cb, ADVSEC_AGENT_LOG_FILE, ADVSEC_AGENT_LOG_INTERVAL);

ev_stat_start(loop, &stat_watcher);

CcspTraceDebug(("Agent log monitoring started on %s\n", ADVSEC_AGENT_LOG_FILE));

ev_run(loop, 0);
ev_loop_destroy(loop);
return NULL;
}

static void advsec_start_log_monitor_thread(void)
{
int err;
pthread_t log_monitor_tid;

err = pthread_create(&log_monitor_tid, NULL, agent_log_monitor_thread, NULL);
if (err != 0)
{
CcspTraceError(("%s: Failed to create agent log monitor thread\n", __FUNCTION__));
}
else
{
pthread_detach(log_monitor_tid);
CcspTraceDebug(("%s: Agent log monitor thread created successfully\n", __FUNCTION__));
}
}

ANSC_STATUS CosaAdvSecStartFeatures(advsec_feature_type type)
{
ANSC_STATUS returnStatus = ANSC_STATUS_SUCCESS;
Expand Down
2 changes: 1 addition & 1 deletion source/test/CcspAdvSecurityDmlTest/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ CcspAdvSecurityDmlTest_gtest_bin_SOURCES = CcspAdvSecurityMock.cpp \
${top_builddir}/source/AdvSecurityDml/plugin_main.c \
gtest_main.cpp

CcspAdvSecurityDmlTest_gtest_bin_LDFLAGS = -lgtest -lgmock -lgcov -pthread
CcspAdvSecurityDmlTest_gtest_bin_LDFLAGS = -lgtest -lgmock -lgcov -pthread -lev

CcspAdvSecurityDmlTest_gtest_bin_LDADD = \
$(HOME)/usr/local/lib/libmock_syscfg.la \
Expand Down
Loading