Skip to content

Commit f13f95a

Browse files
lplewalplewa
committed
fix logger when enabled thru ctl
Logger configuration failed when set via the CTL env variable because the API used const char ** while the env var provided const char *. This mismatch affected the size field (sizeof(char *)==8 vs. string length). The API now accepts char * instead of char **.
1 parent 7cc203e commit f13f95a

File tree

6 files changed

+113
-49
lines changed

6 files changed

+113
-49
lines changed

‎src/ctl/ctl.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,13 @@ static void *ctl_parse_args(const struct ctl_argument *arg_proto, char *arg) {
198198
* structure as required by the node callback
199199
*/
200200
static void *ctl_query_get_real_args(const umf_ctl_node_t *n, void *write_arg,
201-
umf_ctl_query_source_t source) {
201+
umf_ctl_query_source_t source,
202+
size_t *size) {
202203
void *real_arg = NULL;
203204
switch (source) {
204205
case CTL_QUERY_CONFIG_INPUT:
205206
real_arg = ctl_parse_args(n->arg, write_arg);
207+
*size = n->arg->dest_size;
206208
break;
207209
case CTL_QUERY_PROGRAMMATIC:
208210
real_arg = write_arg;
@@ -251,7 +253,7 @@ static umf_result_t ctl_exec_query_write(void *ctx, const umf_ctl_node_t *n,
251253
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
252254
}
253255

254-
void *real_arg = ctl_query_get_real_args(n, arg, source);
256+
void *real_arg = ctl_query_get_real_args(n, arg, source, &size);
255257
if (real_arg == NULL) {
256258
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
257259
}

‎src/utils/utils_log.c‎

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,11 @@ typedef struct {
6666
utils_log_level_t level;
6767
utils_log_level_t flushLevel;
6868
FILE *output;
69-
const char *file_name;
69+
char file_name[MAX_FILE_PATH];
7070
} utils_log_config_t;
7171

7272
utils_log_config_t loggerConfig = {false, false, LOG_ERROR,
73-
LOG_ERROR, NULL, NULL};
73+
LOG_ERROR, NULL, ""};
7474

7575
static const char *level_to_str(utils_log_level_t l) {
7676
switch (l) {
@@ -257,10 +257,10 @@ void utils_log_init(void) {
257257
const char *arg;
258258
if (utils_parse_var(envVar, "output:stdout", NULL)) {
259259
loggerConfig.output = stdout;
260-
loggerConfig.file_name = "stdout";
260+
strncpy(loggerConfig.file_name, "stdout", MAX_FILE_PATH);
261261
} else if (utils_parse_var(envVar, "output:stderr", NULL)) {
262262
loggerConfig.output = stderr;
263-
loggerConfig.file_name = "stderr";
263+
strncpy(loggerConfig.file_name, "stderr", MAX_FILE_PATH);
264264
} else if (utils_parse_var(envVar, "output:file", &arg)) {
265265
loggerConfig.output = NULL;
266266
const char *argEnd = strstr(arg, ";");
@@ -289,7 +289,9 @@ void utils_log_init(void) {
289289
loggerConfig.output = NULL;
290290
return;
291291
}
292-
loggerConfig.file_name = file;
292+
strncpy(loggerConfig.file_name, file, MAX_FILE_PATH - 1);
293+
loggerConfig.file_name[MAX_FILE_PATH - 1] =
294+
'\0'; // ensure null-termination
293295
} else {
294296
loggerConfig.output = stderr;
295297
LOG_ERR("Logging output not set - logging disabled (UMF_LOG = \"%s\")",
@@ -506,17 +508,29 @@ static umf_result_t CTL_READ_HANDLER(output)(void *ctx,
506508
/* suppress unused-parameter errors */
507509
(void)source, (void)indexes, (void)ctx;
508510

509-
const char **arg_out = (const char **)arg;
510-
if (arg_out == NULL || size < sizeof(const char *)) {
511+
char *arg_out = (char *)arg;
512+
if (arg_out == NULL) {
511513
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
512514
}
513515

514516
if (loggerConfig.output == NULL) {
515-
*arg_out = "disabled";
517+
const char disabled[] = "disabled";
518+
if (size < sizeof(disabled)) {
519+
LOG_ERR("Invalid output argument size: %zu, expected at least %zu",
520+
size, sizeof(disabled));
521+
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
522+
}
523+
524+
strncpy(arg_out, disabled, size);
516525
return UMF_RESULT_SUCCESS;
517526
}
527+
if (size < strlen(loggerConfig.file_name)) {
528+
LOG_ERR("Invalid output argument size: %zu, expected at least %zu",
529+
size, strlen(loggerConfig.file_name));
530+
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
531+
}
518532

519-
*arg_out = loggerConfig.file_name;
533+
strncpy(arg_out, loggerConfig.file_name, size);
520534
return UMF_RESULT_SUCCESS;
521535
}
522536

@@ -525,16 +539,13 @@ static umf_result_t CTL_WRITE_HANDLER(output)(void *ctx,
525539
void *arg, size_t size,
526540
umf_ctl_index_utlist_t *indexes) {
527541
/* suppress unused-parameter errors */
528-
(void)source, (void)indexes, (void)ctx;
542+
(void)source, (void)indexes, (void)ctx, (void)size;
529543

530-
const char *arg_in = *(const char **)arg;
531-
if (size < sizeof(const char *)) {
532-
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
533-
}
544+
const char *arg_in = (const char *)arg;
534545

535546
FILE *oldHandle = loggerConfig.output;
536547
const char *oldName =
537-
loggerConfig.file_name ? loggerConfig.file_name : "disabled";
548+
*loggerConfig.file_name == '\0' ? loggerConfig.file_name : "disabled";
538549

539550
if (arg_in == NULL) {
540551
if (loggerConfig.output) {
@@ -543,7 +554,7 @@ static umf_result_t CTL_WRITE_HANDLER(output)(void *ctx,
543554
fclose(oldHandle);
544555
}
545556
loggerConfig.output = NULL;
546-
loggerConfig.file_name = NULL;
557+
loggerConfig.file_name[0] = '\0';
547558
}
548559
return UMF_RESULT_SUCCESS;
549560
}
@@ -552,16 +563,18 @@ static umf_result_t CTL_WRITE_HANDLER(output)(void *ctx,
552563

553564
if (strcmp(arg_in, "stdout") == 0) {
554565
newHandle = stdout;
555-
loggerConfig.file_name = "stdout";
566+
strncpy(loggerConfig.file_name, "stdout", MAX_FILE_PATH);
556567
} else if (strcmp(arg_in, "stderr") == 0) {
557568
newHandle = stderr;
558-
loggerConfig.file_name = "stderr";
569+
strncpy(loggerConfig.file_name, "stderr", MAX_FILE_PATH);
559570
} else {
560571
newHandle = fopen(arg_in, "a");
561572
if (!newHandle) {
562573
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
563574
}
564-
loggerConfig.file_name = arg_in;
575+
strncpy(loggerConfig.file_name, arg_in, MAX_FILE_PATH - 1);
576+
loggerConfig.file_name[MAX_FILE_PATH - 1] =
577+
'\0'; // ensure null-termination
565578
}
566579

567580
loggerConfig.output = newHandle;

‎test/ctl/ctl_api.cpp‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -414,21 +414,23 @@ TEST_F(test, ctl_logger_basic_rw) {
414414
UMF_RESULT_SUCCESS);
415415
EXPECT_EQ(flush_get, 2);
416416

417-
const char *out_name = "stdout";
418-
ASSERT_EQ(umfCtlSet("umf.logger.output", &out_name, sizeof(out_name)),
419-
UMF_RESULT_SUCCESS);
420-
const char *out_get = NULL;
421-
ASSERT_EQ(umfCtlGet("umf.logger.output", &out_get, sizeof(out_get)),
417+
const char out_name[] = "stdout";
418+
ASSERT_EQ(
419+
umfCtlSet("umf.logger.output", (void *)out_name, sizeof(out_name)),
420+
UMF_RESULT_SUCCESS);
421+
const char out_get[256] = "";
422+
ASSERT_EQ(umfCtlGet("umf.logger.output", (void *)out_get, sizeof(out_get)),
422423
UMF_RESULT_SUCCESS);
423424
EXPECT_STREQ(out_get, "stdout");
424425
}
425426

426427
TEST_F(test, ctl_logger_output_file) {
427-
const char *file_name = "ctl_log.txt";
428-
ASSERT_EQ(umfCtlSet("umf.logger.output", &file_name, sizeof(file_name)),
429-
UMF_RESULT_SUCCESS);
430-
const char *out_get = NULL;
431-
ASSERT_EQ(umfCtlGet("umf.logger.output", &out_get, sizeof(out_get)),
428+
const char file_name[] = "ctl_log.txt";
429+
ASSERT_EQ(
430+
umfCtlSet("umf.logger.output", (void *)file_name, sizeof(file_name)),
431+
UMF_RESULT_SUCCESS);
432+
const char out_get[256] = "";
433+
ASSERT_EQ(umfCtlGet("umf.logger.output", (void *)out_get, sizeof(out_get)),
432434
UMF_RESULT_SUCCESS);
433435
EXPECT_STREQ(out_get, file_name);
434436
}

‎test/ctl/ctl_env_app.cpp‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,32 @@ static int test_env_defaults(int argc, char **argv) {
4141
return 0;
4242
}
4343

44+
static int test_logger(int argc, char **argv) {
45+
char buf[256] = {0};
46+
int level = 0;
47+
48+
if (argc != 2) {
49+
std::cerr << "expected two arguments" << std::endl;
50+
std::cerr << "Usage: logger log_output log_level" << std::endl;
51+
return 1;
52+
}
53+
umfCtlGet("umf.logger.output", buf, sizeof(buf));
54+
if (strcmp(buf, argv[0]) != 0) {
55+
std::cerr << "Expected log_output to be '" << argv[0] << "', but got '"
56+
<< buf << "'" << std::endl;
57+
return 1;
58+
}
59+
60+
umfCtlGet("umf.logger.level", &level, sizeof(level));
61+
if (level != atoi(argv[1])) {
62+
std::cerr << "Expected log_level to be '" << argv[1] << "', but got '"
63+
<< level << "'" << std::endl;
64+
return 1;
65+
}
66+
67+
return 0;
68+
}
69+
4470
int main(int argc, char **argv) {
4571
if (argc < 2) {
4672
std::cerr << "Usage: " << argv[0] << " <test_name> args..."
@@ -53,5 +79,9 @@ int main(int argc, char **argv) {
5379
if (strcmp(test_name, "env_defaults") == 0) {
5480
return test_env_defaults(argc, argv);
5581
}
82+
83+
if (strcmp(test_name, "logger") == 0) {
84+
return test_logger(argc, argv);
85+
}
5686
return 1;
5787
}

‎test/ctl/ctl_env_driver.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,3 +121,8 @@ TEST_F(test, ctl_env_plus_file) {
121121
"opt_two_value2", "umf.pool.default.test_pool.opt_three",
122122
"second"});
123123
}
124+
125+
TEST_F(test, ctl_env_logger) {
126+
run_case({{"UMF_CONF", "umf.logger.output=stdout;umf.logger.level=0"}},
127+
{"logger", "stdout", "0"});
128+
}

‎test/utils/utils_log.cpp‎

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,8 @@ void helper_checkConfig(utils_log_config_t *expected, utils_log_config_t *is) {
144144

145145
TEST_F(test, parseEnv_errors) {
146146
expected_message = "";
147-
loggerConfig = {false, false, LOG_ERROR, LOG_ERROR, NULL, ""};
147+
loggerConfig =
148+
utils_log_config_t{false, false, LOG_ERROR, LOG_ERROR, NULL, ""};
148149

149150
expect_fput_count = 0;
150151
expected_stream = stderr;
@@ -217,8 +218,8 @@ TEST_F(test, parseEnv) {
217218
flushLevel.first + ";" +
218219
output.first + ";" +
219220
timestamp.first + ";" + pid.first;
220-
b = loggerConfig = {false, false, LOG_ERROR,
221-
LOG_ERROR, NULL, ""};
221+
b = loggerConfig = utils_log_config_t{
222+
false, false, LOG_ERROR, LOG_ERROR, NULL, ""};
222223
expect_fput_count = 0;
223224
expect_fopen_count = 0;
224225
expected_stream = stderr;
@@ -290,8 +291,8 @@ TEST_F(test, log_levels) {
290291
expected_stream = stderr;
291292
for (int i = LOG_DEBUG; i <= LOG_ERROR; i++) {
292293
for (int j = LOG_DEBUG; j <= LOG_ERROR; j++) {
293-
loggerConfig = {false, false, (utils_log_level_t)i,
294-
LOG_DEBUG, stderr, ""};
294+
loggerConfig = utils_log_config_t{
295+
false, false, (utils_log_level_t)i, LOG_DEBUG, stderr, ""};
295296
if (i > j) {
296297
expect_fput_count = 0;
297298
expect_fflush_count = 0;
@@ -314,7 +315,8 @@ TEST_F(test, log_outputs) {
314315
expect_fflush_count = 1;
315316
expected_message = "[DEBUG UMF] " + MOCK_FN_NAME + ": example log\n";
316317
for (auto o : outs) {
317-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, o, ""};
318+
loggerConfig =
319+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, o, ""};
318320
expected_stream = o;
319321
helper_test_log(LOG_DEBUG, MOCK_FN_NAME.c_str(), "%s", "example log");
320322
}
@@ -325,8 +327,8 @@ TEST_F(test, flush_levels) {
325327
expect_fput_count = 1;
326328
for (int i = LOG_DEBUG; i <= LOG_ERROR; i++) {
327329
for (int j = LOG_DEBUG; j <= LOG_ERROR; j++) {
328-
loggerConfig = {false, false, LOG_DEBUG, (utils_log_level_t)i,
329-
stderr, ""};
330+
loggerConfig = utils_log_config_t{
331+
false, false, LOG_DEBUG, (utils_log_level_t)i, stderr, ""};
330332
if (i > j) {
331333
expect_fflush_count = 0;
332334
} else {
@@ -343,7 +345,8 @@ TEST_F(test, flush_levels) {
343345
TEST_F(test, long_log) {
344346
expect_fput_count = 1;
345347
expect_fflush_count = 1;
346-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
348+
loggerConfig =
349+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
347350
expected_message = "[DEBUG UMF] " + MOCK_FN_NAME + ": " +
348351
std::string(8189 - MOCK_FN_NAME.size(), 'x') + "\n";
349352
helper_test_log(LOG_DEBUG, MOCK_FN_NAME.c_str(), "%s",
@@ -358,7 +361,8 @@ TEST_F(test, long_log) {
358361
TEST_F(test, timestamp_log) {
359362
expect_fput_count = 1;
360363
expect_fflush_count = 1;
361-
loggerConfig = {true, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
364+
loggerConfig =
365+
utils_log_config_t{true, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
362366
// TODO: for now we do not check output message,
363367
// as it requires more sophisticated message validation (a.k.a regrex)
364368
expected_message = "";
@@ -368,15 +372,17 @@ TEST_F(test, timestamp_log) {
368372
TEST_F(test, pid_log) {
369373
expect_fput_count = 1;
370374
expect_fflush_count = 1;
371-
loggerConfig = {false, true, LOG_DEBUG, LOG_DEBUG, stderr, ""};
375+
loggerConfig =
376+
utils_log_config_t{false, true, LOG_DEBUG, LOG_DEBUG, stderr, ""};
372377
// TODO: for now we do not check output message,
373378
// as it requires more sophisticated message validation (a.k.a regrex)
374379
expected_message = "";
375380
helper_test_log(LOG_DEBUG, MOCK_FN_NAME.c_str(), "%s", "example log");
376381
}
377382

378383
TEST_F(test, log_fatal) {
379-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, NULL, ""};
384+
loggerConfig =
385+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, NULL, ""};
380386
expected_stream = stderr;
381387
expect_fput_count = 1;
382388
expect_fflush_count = 1;
@@ -390,7 +396,8 @@ TEST_F(test, log_macros) {
390396
expected_stream = stderr;
391397
expect_fput_count = 1;
392398
expect_fflush_count = 1;
393-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
399+
loggerConfig =
400+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
394401

395402
expected_message = "[DEBUG UMF] TestBody: example log\n";
396403
fput_count = 0;
@@ -437,7 +444,8 @@ template <typename... Args> void helper_test_plog(Args... args) {
437444
}
438445

439446
TEST_F(test, plog_basic) {
440-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
447+
loggerConfig =
448+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
441449
expected_stream = stderr;
442450
errno = 1;
443451
strerr = "test error";
@@ -453,7 +461,8 @@ TEST_F(test, plog_basic) {
453461
}
454462

455463
TEST_F(test, plog_invalid) {
456-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
464+
loggerConfig =
465+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
457466
expected_stream = stderr;
458467
errno = INVALID_ERRNO;
459468
strerr = "test error";
@@ -469,7 +478,8 @@ TEST_F(test, plog_invalid) {
469478
}
470479

471480
TEST_F(test, plog_long_message) {
472-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
481+
loggerConfig =
482+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
473483
expected_stream = stderr;
474484
expect_fput_count = 1;
475485
expect_fflush_count = 1;
@@ -490,7 +500,8 @@ TEST_F(test, plog_long_message) {
490500
}
491501

492502
TEST_F(test, plog_long_error) {
493-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
503+
loggerConfig =
504+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
494505
expected_stream = stderr;
495506
expect_fput_count = 1;
496507
expect_fflush_count = 1;
@@ -516,7 +527,8 @@ TEST_F(test, log_pmacros) {
516527
expected_stream = stderr;
517528
expect_fput_count = 1;
518529
expect_fflush_count = 1;
519-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
530+
loggerConfig =
531+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
520532
errno = 1;
521533
strerr = "test error";
522534

0 commit comments

Comments
 (0)