Address mixed use of ldmsd_log() and ovis_log() #1481
Conversation
The majority of the code uses ldmsd_log(), but a few ldmsd plugins and the authentication plugins use ovis_log(). This mixed use of logging functions creates a possibility of message interleaving. ovis_log() prints to stdout, and both functions write to the same FILE* stream as ldmsd sets stdout to the log file. Modified both functions to use a single fprintf() call per log message instead of multiple calls. While this doesn't eliminate the possibility of message interleaving between threads, consolidating to a single fprintf() per message reduces the opportunities for interruption compared to multiple fprintf() calls.
The loglevel command now affects the default verbosity of messages logged through ovis_log() not only ldmsd_log(). Previously, users could only control the verbosity of messages logged through ldmsd_log(), while ovis_log() messages were unaffected by the loglevel setting.
| @@ -945,6 +940,8 @@ int ovis_vlog(ovis_log_t log, int level, const char *fmt, va_list ap) | |||
| /* No workers, so directly log to the file. */ | |||
| rc = __log(log, level, msg, &tv, &tm); | |||
| free(msg); | |||
| if (log_fp != OVIS_LOG_SYSLOG) | |||
There was a problem hiding this comment.
I don't understand this condition.
There was a problem hiding this comment.
The if-statement prevents calling fflush() on an invalid FILE * stream, which is OVIS_LOG_SYSLOG (0x7).
When ovis_log_open() is called with syslog as the path, I set log_fp to OVIS_LOG_SYSLOG. After investigation, I believe a better approach would be to let applications specify syslog output during initialization through the modes argument of the ovis_log_init() function. Since modes is a mask, we could add a new flag for syslog support. Then, the if-statement will check if the syslog mode is enabled.
The patch is going into b4.4. If we agree on chaning the syslog handling in ovis_log, I could implement the change for the top of tree.
The pull request resolves #1471 .