Skip to content
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

Jump to bottom

Address mixed use of ldmsd_log() and ovis_log() #1481

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Address mixed use of ldmsd_log() and ovis_log() #1481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75f424c
Consolidate fprintf() calls in ldmsd_log() and ovis_log()
nichamon Oct 23, 2024
bf9b315
Allow loglevel command to control ovis_log verbosity
nichamon Oct 25, 2024
b8b7270
Add test mode support for loglevel command in ldmsd_controller
nichamon Oct 25, 2024
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
11 changes: 8 additions & 3 deletions ldms/python/ldmsd/ldmsd_communicator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
'udata_regex': {'req_attr': ['instance', 'regex', 'base'],
'opt_attr': ['incr']},
'version': {'req_attr': [], 'opt_attr': []},
'loglevel': {'req_attr': ['level'],},
'loglevel': {'req_attr': ['level'], 'opt_attr' : ['test']},
'include': {'req_attr': ['path'] },
'env': {'req_attr': []},
'logrotate': {'req_attr': [], 'opt_attr': []},
Expand Down Expand Up @@ -1722,7 +1722,7 @@ def update_time_stats(self, name=None):
self.close()
return errno.ENOTCONN, str(e)

def loglevel(self, level):
def loglevel(self, level, is_test = False):
"""
Change the verbosity level of ldmsd

Expand All @@ -1733,8 +1733,13 @@ def loglevel(self, level):
- status is an errno from the errno module
- data is an error message if status !=0 or None
"""
if is_test:
__test = "true"
else:
__test = "false"
req = LDMSD_Request(command_id=LDMSD_Request.VERBOSITY_CHANGE,
attrs=[LDMSD_Req_Attr(attr_id=LDMSD_Req_Attr.LEVEL, value=level)])
attrs=[LDMSD_Req_Attr(attr_id=LDMSD_Req_Attr.LEVEL, value=level),
LDMSD_Req_Attr(attr_id=LDMSD_Req_Attr.TEST, value=__test)])
try:
req.send(self)
resp = req.receive(self)
Expand Down
2 changes: 1 addition & 1 deletion ldms/python/ldmsd/ldmsd_controller
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1367,7 +1367,7 @@ class LdmsdCmdParser(cmd.Cmd):
"""
arg = self.handle_args('loglevel', arg)
if arg:
rc, msg = self.comm.loglevel(arg['level'])
rc, msg = self.comm.loglevel(arg['level'], arg['test'])
if rc:
print(f'Error changing log level to arg["level"]: {msg}')

Expand Down
3 changes: 2 additions & 1 deletion ldms/src/ldmsd/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ LJSON_UTIL = $(top_builddir)/lib/src/ovis_json/libovis_json.la
LOVIS_EVENT = $(top_builddir)/lib/src/ovis_event/libovis_event.la
LOVIS_CTRL = $(top_builddir)/lib/src/ovis_ctrl/libovis_ctrl.la
LOVIS_EV = $(top_builddir)/lib/src/ovis_ev/libovis_ev.la
LOVIS_LOG = $(top_builddir)/lib/src/ovis_log/libovis_log.la

AM_CFLAGS += -DPLUGINDIR='"$(pkglibdir)"'

Expand All @@ -47,7 +48,7 @@ ldmsd_SOURCES = ldmsd.c ldmsd_config.c \
ldmsd_decomp.c ldmsd_row_cache.c
ldmsd_LDADD = ../core/libldms.la libldmsd_request.la libldmsd_stream.la \
$(LZAP) $(LMMALLOC) $(LOVIS_UTIL) $(LCOLL) $(LJSON_UTIL) $(LTLIBJANSSON) \
$(LOVIS_EVENT) $(LOVIS_EV) -lpthread $(LOVIS_CTRL) -lm -ldl
$(LOVIS_EVENT) $(LOVIS_EV) $(LOVIS_LOG) -lpthread $(LOVIS_CTRL) -lm -ldl
ldmsd_CFLAGS = $(AM_CFLAGS)
ldmsd_LDFLAGS = $(AM_LDFLAGS) -rdynamic -pthread

Expand Down
26 changes: 17 additions & 9 deletions ldms/src/ldmsd/ldmsd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
#include "kldms_req.h"

#include "ovis_event/ovis_event.h"
#include "ovis_log/ovis_log.h"

#ifdef DEBUG
#include <mcheck.h>
Expand Down Expand Up @@ -219,6 +220,14 @@ int ldmsd_loglevel_set(char *verbose_level)
if (level < 0)
return level;
log_level_thr = level;

/*
* Set ovis_log's log masks
* The code is added to handle the situation of
* the mixed use of ldmsd_log() and ovis_log().
*/
level = ovis_log_str_to_level(verbose_level);
ovis_log_set_level(NULL, level);
return 0;
}

Expand Down Expand Up @@ -297,19 +306,17 @@ int __log(enum ldmsd_loglevel level, char *msg, struct timeval *tv, struct tm *t
return 0;
}

char log_time[200];
if (log_time_sec) {
fprintf(log_fp, "%lu.%06lu: ", tv->tv_sec, tv->tv_usec);
snprintf(log_time, 199, "%lu.%06lu: ", tv->tv_sec, tv->tv_usec);
} else {
char dtsz[200];
if (strftime(dtsz, sizeof(dtsz), "%a %b %d %H:%M:%S %Y", tm))
fprintf(log_fp, "%s: ", dtsz);
}

if (level < LDMSD_LALL) {
fprintf(log_fp, "%-10s: ", ldmsd_loglevel_names[level]);
strftime(log_time, 199, "%a %b %d %H:%M:%S %Y", tm);
}

fprintf(log_fp, "%s", msg);
fprintf(log_fp, "%s: %-10s: %s",
log_time,
((level < LDMSD_LALL)?ldmsd_loglevel_names[level]:""),
msg);

return 0;
}
Expand Down Expand Up @@ -2300,6 +2307,7 @@ int main(int argc, char *argv[])

/* Initialize LDMS */
umask(0);
ovis_log_init(NULL, ovis_log_str_to_level("ERROR"), 0);
if (!auth_name)
auth_name = DEFAULT_AUTH_NAME;
if (-1 == banner)
Expand Down
21 changes: 15 additions & 6 deletions ldms/src/ldmsd/ldmsd_request.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@
#include "ldmsd_stream.h"
#include "ldms_xprt.h"


#include "ovis_log/ovis_log.h"

#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
/*
* This file implements an LDMSD control protocol. The protocol is
Expand Down Expand Up @@ -5704,12 +5707,18 @@ static int verbosity_change_handler(ldmsd_req_ctxt_t reqc)
__dlog(DLOG_CFGOK, "loglevel level=%s%s\n", level_s,
is_test ? " test" : "");
if (is_test) {
ldmsd_log(LDMSD_LDEBUG, "TEST DEBUG\n");
ldmsd_log(LDMSD_LINFO, "TEST INFO\n");
ldmsd_log(LDMSD_LWARNING, "TEST WARNING\n");
ldmsd_log(LDMSD_LERROR, "TEST ERROR\n");
ldmsd_log(LDMSD_LCRITICAL, "TEST CRITICAL\n");
ldmsd_log(LDMSD_LALL, "TEST ALWAYS\n");
ldmsd_log(LDMSD_LDEBUG, "ldmsd_log(TEST DEBUG)\n");
ldmsd_log(LDMSD_LINFO, "ldmsd_log(TEST INFO)\n");
ldmsd_log(LDMSD_LWARNING, "ldmsd_log(TEST WARNING)\n");
ldmsd_log(LDMSD_LERROR, "ldmsd_log(TEST ERROR)\n");
ldmsd_log(LDMSD_LCRITICAL, "ldmsd_log(TEST CRITICAL)\n");
ldmsd_log(LDMSD_LALL, "ldmsd_log(TEST ALWAYS)\n");

ovis_log(NULL, OVIS_LDEBUG, "ovis_log(TEST DEBUG)\n");
ovis_log(NULL, OVIS_LINFO, "ovis_log(TEST INFO)\n");
ovis_log(NULL, OVIS_LWARNING, "ovis_log(TEST WARNING)\n");
ovis_log(NULL, OVIS_LERROR, "ovis_log(TEST ERROR)\n");
ovis_log(NULL, OVIS_LCRITICAL, "ovis_log(TEST CRITICAL)\n");
}

out:
Expand Down
27 changes: 12 additions & 15 deletions lib/src/ovis_log/ovis_log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,30 +651,25 @@ static int __log(ovis_log_t log, int level, char *msg,

int rc;
FILE *f;
char log_time[200] = "";

if (!log_fp)
f = stdout;
else
f = log_fp;

if (default_modes & OVIS_LOG_M_TS) {
rc = fprintf(f, "%lu.%06lu:", tv->tv_sec, tv->tv_usec);
snprintf(log_time, 199, "%lu.%06lu:", tv->tv_sec, tv->tv_usec);
} else if (default_modes & OVIS_LOG_M_DT) {
char dtsz[200];
if (strftime(dtsz, sizeof(dtsz), "%a %b %d %H:%M:%S %Y", tm))
rc = fprintf(f, "%s:", dtsz);
else
rc = -EINVAL; /* not expected with gnu libc */
} else {
rc = 0;
strftime(log_time, 199, "%a %b %d %H:%M:%S %Y", tm);
}
if (rc < 0)
return rc;

/* Print the level name */
rc = fprintf(f, "%9s:", ((level == OVIS_LALWAYS)?"":ovis_loglevel_names[level]));
if (rc < 0)
return rc;

rc = fprintf(f, " %s: %s", log->name, msg);
rc = fprintf(f, "%s: %-10s: %s: %s",
log_time,
((level == OVIS_LALWAYS)?"":ovis_loglevel_names[level]),
log->name,
msg);
if (rc < 0)
return rc;
return 0;
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this condition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

fflush(log_fp);
goto out;
}

Expand Down
Loading