control logger thru CTL #1446

lplewa · 2025-07-17T12:09:32Z

Description

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

CI workflows, not executed per PR (e.g. Nightly), execute properly
New tests added, especially if they will fail without my changes
Added/extended example(s) to cover this functionality
Extended the README/documentation
All newly added source files have a license
All newly added source files are referenced in CMake files
Logger (with debug/info/... messages) is used
All API changes are reflected in docs and def/map files, and are tested

bratpiorka · 2025-07-17T15:02:43Z

src/utils/CMakeLists.txt

@@ -32,7 +33,7 @@ add_umf_library(
    NAME umf_utils
    TYPE STATIC
    SRCS ${UMF_UTILS_SOURCES}
-    LIBS ${UMF_UTILS_LIBS} ${CMAKE_THREAD_LIBS_INIT})
+    LIBS ${UMF_UTILS_LIBS} ${CMAKE_THREAD_LIBS_INIT} umf_ba)


we should avoid this if possible

ok for now, but please fill an issue to fix this

bratpiorka · 2025-07-22T12:36:50Z

src/utils/utils_log.c

@@ -61,14 +62,15 @@ char const __umf_str_1__all_cmake_vars[] =
 #define MAX_ENV_LEN 2048

 typedef struct {
-    int timestamp;
-    int pid;
+    bool timestamp;


please add comment "enable timestamp logging" or rename to enableTimestamp

bratpiorka · 2025-07-22T12:36:57Z

src/utils/utils_log.c

-    int timestamp;
-    int pid;
+    bool timestamp;
+    bool pid;


bratpiorka · 2025-07-22T12:37:20Z

src/utils/utils_log.c

 } utils_log_config_t;

-utils_log_config_t loggerConfig = {0, 0, LOG_ERROR, LOG_ERROR, NULL};
+utils_log_config_t loggerConfig = {0, 0, LOG_ERROR, LOG_ERROR, NULL, NULL};


0, 0 -> false, false

bratpiorka · 2025-07-22T12:40:36Z

src/utils/utils_log.c

+    }
+
+    if (loggerConfig.output == NULL) {
+        *arg_out = "logger_disabled";


just "disabled"

what about "" ? is it different from NULL?

it will never be set to ""

src/utils/utils_log.h

bratpiorka · 2025-07-22T12:44:50Z

test/ctl/ctl_api.cpp

+
+TEST_F(test, ctl_logger_output_file) {
+    const char *file_name = "ctl_log.txt";
+


remove empty line

bratpiorka · 2025-07-22T12:45:13Z

test/utils/utils_log.cpp

 #include "test_helpers.h"
+#include <umf/experimental/ctl.h>


move to the top (new section)

bratpiorka · 2025-07-22T12:45:46Z

test/utils/utils_log.cpp

@@ -141,7 +143,7 @@ void helper_checkConfig(utils_log_config_t *expected, utils_log_config_t *is) {

 TEST_F(test, parseEnv_errors) {
    expected_message = "";
-    loggerConfig = {0, 0, LOG_ERROR, LOG_ERROR, NULL};
+    loggerConfig = {0, 0, LOG_ERROR, LOG_ERROR, NULL, ""};


bool, bool?

bratpiorka · 2025-07-22T12:46:19Z

test/utils/utils_log.cpp

@@ -212,7 +214,8 @@ TEST_F(test, parseEnv) {
                                             flushLevel.first + ";" +
                                             output.first + ";" +
                                             timestamp.first + ";" + pid.first;
-                        b = loggerConfig = {0, 0, LOG_ERROR, LOG_ERROR, NULL};
+                        b = loggerConfig = {0,         0,    LOG_ERROR,


false, false here and in other places
btw why such weird formatting?

done,

clang format? who knows

bratpiorka · 2025-07-22T12:48:54Z

src/utils/utils_log.c

+    const char *oldName =
+        loggerConfig.file_name ? loggerConfig.file_name : "disabled";
+
+    if (arg_in == NULL) {


what about ""?

fopen will fail, and we will return an error for invalid argument

lplewa requested a review from a team as a code owner July 17, 2025 12:09

lplewa force-pushed the ctl_logger branch 3 times, most recently from 0a0d049 to 777a17a Compare July 17, 2025 14:08

bratpiorka reviewed Jul 17, 2025

View reviewed changes

bratpiorka requested changes Jul 22, 2025

View reviewed changes

KFilipek approved these changes Jul 22, 2025

View reviewed changes

lplewa force-pushed the ctl_logger branch from 777a17a to 1b687d4 Compare July 22, 2025 15:05

control logger thru CTL

9a61dda

lplewa force-pushed the ctl_logger branch from 1b687d4 to 9a61dda Compare July 22, 2025 15:12


		TEST_F(test, ctl_logger_output_file) {
		const char *file_name = "ctl_log.txt";

control logger thru CTL #1446

Are you sure you want to change the base?

control logger thru CTL #1446

Conversation

lplewa commented Jul 17, 2025

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!