RDKEMW-14300 : Removed Log truncation & added testApp to measure CPU#53
RDKEMW-14300 : Removed Log truncation & added testApp to measure CPU#53karuna2git wants to merge 1 commit intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes the log truncation mechanism and introduces dynamic buffer allocation for handling large log messages. It also adds two new log format types (WITH_TID and WITH_TS_TID), adds a CPU performance measurement test application, and removes old test files.
Changes:
- Removed fixed-size buffer truncation logic, replaced with dynamic reallocation using log4c's native vlog function
- Added RDKLOG_FORMAT_WITH_TID and RDKLOG_FORMAT_WITH_TS_TID format types
- Added CPU measurement test application (rdk_logger_cpu_test.c) and removed legacy test files
- Modified log4crc configuration to set buffer size to 0 (enabling dynamic allocation)
- Added unit tests for new log formats
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rdk_debug_priv.c | Refactored log formatting to use dynamic buffer allocation; added support for two new log format types with thread ID |
| src/rdk_debug.c | Added NULL parameter validation (but with incorrect logic) |
| include/rdk_logger.h | Added new enum values for WITH_TID and WITH_TS_TID formats |
| log4crc | Changed buffer size from 1024 to 0 to enable dynamic allocation; added new layout types and changed default appenders |
| test/rdk_logger_cpu_test.c | New test application to measure CPU usage of logging operations |
| test/rdk_logger_debug_test.c | Removed legacy test file |
| test/rdk_logger_test_main.c | Removed legacy test file |
| test/Makefile.am | Updated to build new CPU test instead of old test program |
| unittests/rdkLoggerRotationTest.cpp | Added tests for new format types; fixed indentation issue |
| unittests/rdkLoggerErrorTest.cpp | Removed test case for very long format strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct tm tm; | ||
| char timeBuff[COMCAST_DATAED_BUFF_SIZE] = {0}; | ||
|
|
||
| /** Get the last part of the cagetory as "module" */ |
There was a problem hiding this comment.
Typo in comment: "cagetory" should be "category".
| /** Get the last part of the cagetory as "module" */ | |
| /** Get the last part of the category as "module" */ |
e44d413 to
453882e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/rdk_debug_priv.c:688
- getModuleName() casts away const from event->evt_category and also casts a string literal to char*. This can trigger compiler warnings and is undefined behavior if the returned pointer is ever modified. Use const char* throughout (and avoid C-style casts) since the function only needs read-only access.
static const char* getModuleName(log4c_logging_event_t* event)
{
/** Get the last part of the category as "module" */
char *p= (char *)(event->evt_category);
if (NULL == p)
{
p = (char*)"UNKNOWN";
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| event->evt_buffer.buf_size = n + 1; | ||
| event->evt_buffer.buf_data = (char *) realloc (event->evt_buffer.buf_data, event->evt_buffer.buf_size); | ||
| return rdk_format_log(layout, event, format); |
There was a problem hiding this comment.
In rdk_format_log(), realloc()’s return value is assigned directly back to event->evt_buffer.buf_data. If realloc fails, this overwrites the original pointer (leaking it) and subsequent recursion returns NULL, potentially breaking log rendering. Use a temporary pointer and only update buf_data/buf_size when realloc succeeds; handle allocation failure without losing the original buffer.
| event->evt_buffer.buf_size = n + 1; | |
| event->evt_buffer.buf_data = (char *) realloc (event->evt_buffer.buf_data, event->evt_buffer.buf_size); | |
| return rdk_format_log(layout, event, format); | |
| size_t new_size = (size_t)n + 1; | |
| char *new_buf = (char *)realloc(event->evt_buffer.buf_data, new_size); | |
| if (new_buf != NULL) | |
| { | |
| event->evt_buffer.buf_data = new_buf; | |
| event->evt_buffer.buf_size = new_size; | |
| return rdk_format_log(layout, event, format); | |
| } | |
| /* If realloc fails, keep the existing buffer and fall through, | |
| * returning the truncated log message that snprintf already wrote. | |
| */ |
| char line[256]; | ||
| memset(times, 0, sizeof(*times)); | ||
| if (fgets(line, sizeof(line), file) != NULL) { | ||
| printf ("%s\n", line); |
There was a problem hiding this comment.
getCPUStat() prints the raw /proc/stat line on every call. This adds significant I/O overhead/noise and will skew the CPU measurements this tool is trying to capture. Consider removing this printf or gating it behind an explicit verbose flag.
| printf ("%s\n", line); |
453882e to
0f8cc19
Compare
0f8cc19 to
c702d19
Compare
c702d19 to
3fbe5a8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| event->evt_buffer.buf_size = n + 1; | ||
| event->evt_buffer.buf_data = (char *) realloc (event->evt_buffer.buf_data, event->evt_buffer.buf_size); | ||
| return rdk_format_log(layout, event, format); |
There was a problem hiding this comment.
The recursive call to rdk_format_log after reallocation may lead to stack overflow if the buffer repeatedly needs to grow. This could be particularly problematic with extremely large log messages. Consider implementing an iterative approach with a maximum size limit instead of recursion.
Reason for change: 1. Removed Log split & truncation 2. Configured default buffer size to be 0 as same as Previous Releases 3. Added new log formats as TID & TS_TIDs 4. Added testApp to measure CPU 5. Updated default logging to console instead of recently changes syslog 6. Added unit test cases to verify the new formats 7. Added unified log function to handle all the log formats Test Procedure: Verify huge data Risks: Medium Signed-off-by: Karunakaran A <karunakaran_amirthalingam@cable.comcast.com>
3fbe5a8 to
fa6652b
Compare
Reason for change: Removed Log truncation & added testApp to measure CPU
Test Procedure: Verify huge data
Risks: Low