The reason for this is to be later able to split parsing from defining in a convenient and transparent way, so eventually, the original virLogParse* (virLogParseAndDefine* after this patch) functions which were named a bit poorly will be dropped completely. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- daemon/libvirtd.c | 8 ++++---- src/libvirt_private.syms | 4 ++-- src/locking/lock_daemon.c | 8 ++++---- src/logging/log_daemon.c | 8 ++++---- src/util/virlog.c | 20 ++++++++++---------- src/util/virlog.h | 4 ++-- tests/virlogtest.c | 4 ++-- 7 files changed, 28 insertions(+), 28 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 95c1b1c..8efa69d 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -691,10 +691,10 @@ daemonSetupLogging(struct daemonConfig *config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseFilters(config->log_filters); + virLogParseAndDefineFilters(config->log_filters); if (virLogGetNbOutputs() == 0) - virLogParseOutputs(config->log_outputs); + virLogParseAndDefineOutputs(config->log_outputs); /* * Command line override for --verbose @@ -721,7 +721,7 @@ daemonSetupLogging(struct daemonConfig *config, if (virAsprintf(&tmp, "%d:journald", priority) < 0) goto error; - virLogParseOutputs(tmp); + virLogParseAndDefineOutputs(tmp); VIR_FREE(tmp); } } @@ -764,7 +764,7 @@ daemonSetupLogging(struct daemonConfig *config, if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) goto error; } - virLogParseOutputs(tmp); + virLogParseAndDefineOutputs(tmp); VIR_FREE(tmp); } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a32ce1c..35200a3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1854,9 +1854,9 @@ virLogLock; virLogMessage; virLogOutputFree; virLogOutputListFree; +virLogParseAndDefineFilters; +virLogParseAndDefineOutputs; virLogParseDefaultPriority; -virLogParseFilters; -virLogParseOutputs; virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 9509e0c..84c3029 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -476,10 +476,10 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseFilters(config->log_filters); + virLogParseAndDefineFilters(config->log_filters); if (virLogGetNbOutputs() == 0) - virLogParseOutputs(config->log_outputs); + virLogParseAndDefineOutputs(config->log_outputs); /* * Command line override for --verbose @@ -499,7 +499,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, if (access("/run/systemd/journal/socket", W_OK) >= 0) { if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) goto error; - virLogParseOutputs(tmp); + virLogParseAndDefineOutputs(tmp); VIR_FREE(tmp); } } @@ -543,7 +543,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) goto error; } - virLogParseOutputs(tmp); + virLogParseAndDefineOutputs(tmp); VIR_FREE(tmp); } diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 80e75bf..48eece9 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -404,10 +404,10 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseFilters(config->log_filters); + virLogParseAndDefineFilters(config->log_filters); if (virLogGetNbOutputs() == 0) - virLogParseOutputs(config->log_outputs); + virLogParseAndDefineOutputs(config->log_outputs); /* * Command line override for --verbose @@ -427,7 +427,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, if (access("/run/systemd/journal/socket", W_OK) >= 0) { if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) goto error; - virLogParseOutputs(tmp); + virLogParseAndDefineOutputs(tmp); VIR_FREE(tmp); } } @@ -471,7 +471,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) goto error; } - virLogParseOutputs(tmp); + virLogParseAndDefineOutputs(tmp); VIR_FREE(tmp); } diff --git a/src/util/virlog.c b/src/util/virlog.c index 06f9a60..3ada288 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1137,7 +1137,7 @@ int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) static int -virLogParseOutput(const char *src) +virLogParseAndDefineOutput(const char *src) { int ret = -1; char **tokens = NULL; @@ -1209,7 +1209,7 @@ virLogParseOutput(const char *src) /** - * virLogParseOutputs: + * virLogParseAndDefineOutputs: * @outputs: string defining a (set of) output(s) * * The format for an output can be: @@ -1234,7 +1234,7 @@ virLogParseOutput(const char *src) * Returns the number of output parsed or -1 in case of error. */ int -virLogParseOutputs(const char *src) +virLogParseAndDefineOutputs(const char *src) { int ret = -1; int count = 0; @@ -1254,7 +1254,7 @@ virLogParseOutputs(const char *src) if (STREQ(strings[i], "")) continue; - if (virLogParseOutput(strings[i]) < 0) + if (virLogParseAndDefineOutput(strings[i]) < 0) goto cleanup; count++; @@ -1268,7 +1268,7 @@ virLogParseOutputs(const char *src) static int -virLogParseFilter(const char *filter) +virLogParseAndDefineFilter(const char *filter) { int ret = -1; size_t count = 0; @@ -1314,7 +1314,7 @@ virLogParseFilter(const char *filter) } /** - * virLogParseFilters: + * virLogParseAndDefineFilters: * @filters: string defining a (set of) filter(s) * * The format for a filter is: @@ -1332,7 +1332,7 @@ virLogParseFilter(const char *filter) * Returns the number of filter parsed or -1 in case of error. */ int -virLogParseFilters(const char *filters) +virLogParseAndDefineFilters(const char *filters) { int ret = -1; int count = 0; @@ -1352,7 +1352,7 @@ virLogParseFilters(const char *filters) if (STREQ(strings[i], "")) continue; - if (virLogParseFilter(strings[i]) < 0) + if (virLogParseAndDefineFilter(strings[i]) < 0) goto cleanup; count++; @@ -1530,10 +1530,10 @@ virLogSetFromEnv(void) virLogParseDefaultPriority(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS"); if (debugEnv && *debugEnv) - virLogParseFilters(debugEnv); + virLogParseAndDefineFilters(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS"); if (debugEnv && *debugEnv) - virLogParseOutputs(debugEnv); + virLogParseAndDefineOutputs(debugEnv); } diff --git a/src/util/virlog.h b/src/util/virlog.h index f6ee8e6..de64f4c 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -206,8 +206,8 @@ void virLogLock(void); void virLogUnlock(void); int virLogReset(void); int virLogParseDefaultPriority(const char *priority); -int virLogParseFilters(const char *filters); -int virLogParseOutputs(const char *output); +int virLogParseAndDefineFilters(const char *filters); +int virLogParseAndDefineOutputs(const char *output); int virLogPriorityFromSyslog(int priority); void virLogMessage(virLogSourcePtr source, virLogPriority priority, diff --git a/tests/virlogtest.c b/tests/virlogtest.c index 5c492a2..afcd84a 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -50,7 +50,7 @@ testLogParseOutputs(const void *opaque) int noutputs; const struct testLogData *data = opaque; - noutputs = virLogParseOutputs(data->str); + noutputs = virLogParseAndDefineOutputs(data->str); if (noutputs < 0) { if (!data->pass) { VIR_TEST_DEBUG("Got expected error: %s\n", @@ -81,7 +81,7 @@ testLogParseFilters(const void *opaque) int nfilters; const struct testLogData *data = opaque; - nfilters = virLogParseFilters(data->str); + nfilters = virLogParseAndDefineFilters(data->str); if (nfilters < 0) { if (!data->pass) { VIR_TEST_DEBUG("Got expected error: %s\n", -- 2.5.5

This method allocated a new filter object which it then returns back to caller. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virlog.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 3 +++ 3 files changed, 51 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b5cee5f..088f9f3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1845,6 +1845,8 @@ virLockSpaceReleaseResourcesForOwner; virLogDefineFilter; virLogDefineOutput; virLogFilterFree; +virLogFilterListFree; +virLogFilterNew; virLogGetDefaultPriority; virLogGetFilters; virLogGetNbFilters; diff --git a/src/util/virlog.c b/src/util/virlog.c index 91c63a1..e4dc84b 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1604,3 +1604,49 @@ virLogOutputNew(virLogOutputFunc f, return ret; } + + +/** + * virLogFilterNew: + * @match: the pattern to match + * @priority: the priority to give to messages matching the pattern + * @flags: extra flags, see virLogFilterFlags enum + * + * Allocates and returns a new log filter object. The object has to be later + * defined, so that the pattern will be taken into account when executing the + * log filters (to select or reject a particular message) on messages. + * + * The filter defines a rules that will apply only to messages matching + * the pattern (currently if @match is a substring of the message category) + * + * Returns a reference to a newly created filter that needs to be defined using + * virLogDefineFilters, or NULL in case of an error. + */ +virLogFilterPtr +virLogFilterNew(const char *match, + virLogPriority priority, + unsigned int flags) +{ + virLogFilterPtr ret = NULL; + char *mdup = NULL; + + virCheckFlags(VIR_LOG_STACK_TRACE, NULL); + + if ((match == NULL) || (priority < VIR_LOG_DEBUG) || + (priority > VIR_LOG_ERROR)) + return NULL; + + if (VIR_STRDUP_QUIET(mdup, match) < 0) + return NULL; + + if (VIR_ALLOC_QUIET(ret) < 0) { + VIR_FREE(mdup); + return NULL; + } + + ret->match = mdup; + ret->priority = priority; + ret->flags = flags; + + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index fb32c41..a56d297 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -232,5 +232,8 @@ virLogOutputPtr virLogOutputNew(virLogOutputFunc f, virLogPriority priority, virLogDestination dest, const char *name); +virLogFilterPtr virLogFilterNew(const char *match, + virLogPriority priority, + unsigned int flags); #endif -- 2.5.5

Prepare a method that only defines a set of outputs. It takes a list of outputs, preferably created by virLogParseOutputs. The original set of outputs is reset and replaced by the new user-provided set of outputs. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 25 +++++++++++++++++++ src/util/virlog.h | 63 ++++++++++++++++++++++++------------------------ 3 files changed, 58 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d28405c..fb7f277 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1844,6 +1844,7 @@ virLockSpaceReleaseResourcesForOwner; # util/virlog.h virLogDefineFilter; virLogDefineOutput; +virLogDefineOutputs; virLogFilterFree; virLogFilterListFree; virLogFilterNew; diff --git a/src/util/virlog.c b/src/util/virlog.c index 5916ac8..2651f70 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1682,3 +1682,28 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, return -1; } + + +/** + * virLogDefineOutputs: + * @outputs: new set of outputs to be defined + * @noutputs: number of outputs in @outputs + * + * Resets any existing set of outputs and defines a completely new one. + * + * Returns number of outputs successfully defined or -1 in case of error; + */ +int +virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs) +{ + if (virLogInitialize() < 0) + return -1; + + virLogLock(); + virLogResetOutputs(); + virLogOutputs = outputs; + virLogNbOutputs = noutputs; + virLogUnlock(); + + return virLogNbOutputs; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index 2045c06..8568830 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -173,7 +173,7 @@ typedef void (*virLogOutputFunc) (virLogSourcePtr src, typedef void (*virLogCloseFunc) (void *data); typedef enum { - VIR_LOG_STACK_TRACE = (1 << 0), +VIR_LOG_STACK_TRACE = (1 << 0), } virLogFlags; int virLogGetNbFilters(void); @@ -184,23 +184,23 @@ virLogPriority virLogGetDefaultPriority(void); int virLogSetDefaultPriority(virLogPriority priority); void virLogSetFromEnv(void); int virLogDefineFilter(const char *match, - virLogPriority priority, - unsigned int flags); + virLogPriority priority, + unsigned int flags); int virLogDefineOutput(virLogOutputFunc f, - virLogCloseFunc c, - void *data, - virLogPriority priority, - virLogDestination dest, - const char *name, - unsigned int flags); + virLogCloseFunc c, + void *data, + virLogPriority priority, + virLogDestination dest, + const char *name, + unsigned int flags); void virLogOutputFree(virLogOutputPtr output); void virLogOutputListFree(virLogOutputPtr *list, int count); void virLogFilterFree(virLogFilterPtr filter); void virLogFilterListFree(virLogFilterPtr *list, int count); /* - * Internal logging API - */ +* Internal logging API +*/ void virLogLock(void); void virLogUnlock(void); @@ -210,32 +210,33 @@ int virLogParseAndDefineFilters(const char *filters); int virLogParseAndDefineOutputs(const char *output); int virLogPriorityFromSyslog(int priority); void virLogMessage(virLogSourcePtr source, - virLogPriority priority, - const char *filename, - int linenr, - const char *funcname, - virLogMetadataPtr metadata, - const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(7, 8); + virLogPriority priority, + const char *filename, + int linenr, + const char *funcname, + virLogMetadataPtr metadata, + const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(7, 8); void virLogVMessage(virLogSourcePtr source, - virLogPriority priority, - const char *filename, - int linenr, - const char *funcname, - virLogMetadataPtr metadata, - const char *fmt, - va_list vargs) ATTRIBUTE_FMT_PRINTF(7, 0); + virLogPriority priority, + const char *filename, + int linenr, + const char *funcname, + virLogMetadataPtr metadata, + const char *fmt, + va_list vargs) ATTRIBUTE_FMT_PRINTF(7, 0); bool virLogProbablyLogMessage(const char *str); virLogOutputPtr virLogOutputNew(virLogOutputFunc f, - virLogCloseFunc c, - void *data, - virLogPriority priority, - virLogDestination dest, - const char *name); + virLogCloseFunc c, + void *data, + virLogPriority priority, + virLogDestination dest, + const char *name); virLogFilterPtr virLogFilterNew(const char *match, - virLogPriority priority, - unsigned int flags); + virLogPriority priority, + unsigned int flags); int virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, virLogDestination dest, const void *opaque); +int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs); #endif -- 2.5.5

Continuing with the effort to split output parsing and defining, these new functions return a logging object reference instead of defining the output. Eventually, these functions will replace the existing ones (virLogAddOutputTo*) which will then be dropped. Also, make the new functions non-static, so they can be introduced prior to usage and the compiler won't complain (will be switched back to static once the usage is sorted out by later patches). Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 4 ++ src/util/virlog.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 6 +++ 3 files changed, 108 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bceba7..39b0270 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1857,6 +1857,10 @@ virLogGetNbOutputs; virLogGetOutputs; virLogLock; virLogMessage; +virLogNewOutputToFile; +virLogNewOutputToJournald; +virLogNewOutputToStderr; +virLogNewOutputToSyslog; virLogOutputFree; virLogOutputListFree; virLogOutputNew; diff --git a/src/util/virlog.c b/src/util/virlog.c index a74967b..c0b8b9a 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -780,6 +780,14 @@ virLogAddOutputToStderr(virLogPriority priority) } +virLogOutputPtr +virLogNewOutputToStderr(virLogPriority priority) +{ + return virLogOutputNew(virLogOutputToFd, NULL, (void *)2L, priority, + VIR_LOG_TO_STDERR, NULL); +} + + static int virLogAddOutputToFile(virLogPriority priority, const char *file) @@ -799,6 +807,27 @@ virLogAddOutputToFile(virLogPriority priority, } +virLogOutputPtr +virLogNewOutputToFile(virLogPriority priority, + const char *file) +{ + int fd; + virLogOutputPtr ret = NULL; + + fd = open(file, O_CREAT | O_APPEND | O_WRONLY, S_IRUSR | S_IWUSR); + if (fd < 0) + return NULL; + + if (!(ret = virLogOutputNew(virLogOutputToFd, virLogCloseFd, + (void *)(intptr_t)fd, + priority, VIR_LOG_TO_FILE, file))) { + VIR_LOG_CLOSE(fd); + return NULL; + } + return ret; +} + + #if HAVE_SYSLOG_H || USE_JOURNALD /* Compat in case we build with journald, but no syslog */ @@ -886,6 +915,51 @@ virLogAddOutputToSyslog(virLogPriority priority, } +virLogOutputPtr +virLogNewOutputToSyslog(virLogPriority priority, + const char *ident) +{ + virLogOutputPtr ret = NULL; + int at = -1; + + /* There are a couple of issues with syslog: + * 1) If we re-opened the connection by calling openlog now, it would change + * the message tag immediately which is not what we want, since we might be + * in the middle of parsing of a new set of outputs where anything still can + * go wrong and we would introduce an inconsistent state to the log. We're + * also not holding a lock on the logger if we tried to change the tag + * while other workers are actively logging. + * + * 2) Syslog keeps the open file descriptor private, so we can't just dup() + * it like we would do with files if an output already existed. + * + * If a syslog connection already exists changing the message tag has to be + * therefore special-cased and postponed until the very last moment. + */ + if ((at = virLogFindOutput(virLogOutputs, virLogNbOutputs, + VIR_LOG_TO_SYSLOG, NULL)) < 0) { + /* + * rather than copying @ident, syslog uses caller's reference instead + */ + VIR_FREE(current_ident); + if (VIR_STRDUP(current_ident, ident) < 0) + return NULL; + + openlog(current_ident, 0, 0); + } + + if (!(ret = virLogOutputNew(virLogOutputToSyslog, virLogCloseSyslog, + NULL, priority, VIR_LOG_TO_SYSLOG, ident))) { + if (at < 0) { + closelog(); + VIR_FREE(current_ident); + } + return NULL; + } + return ret; +} + + # if USE_JOURNALD # define IOVEC_SET(iov, data, size) \ do { \ @@ -1102,6 +1176,30 @@ static int virLogAddOutputToJournald(int priority) } return 0; } + + +virLogOutputPtr +virLogNewOutputToJournald(int priority) +{ + virLogOutputPtr ret = NULL; + + if ((journalfd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) + return NULL; + + if (virSetInherit(journalfd, false) < 0) { + VIR_LOG_CLOSE(journalfd); + return NULL; + } + + if (!(ret = virLogOutputNew(virLogOutputToJournald, + virLogCloseJournald, NULL, + priority, VIR_LOG_TO_JOURNALD, NULL))) { + VIR_LOG_CLOSE(journalfd); + return NULL; + } + + return ret; +} # endif /* USE_JOURNALD */ int virLogPriorityFromSyslog(int priority) diff --git a/src/util/virlog.h b/src/util/virlog.h index e0fe008..e3d5243 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -239,5 +239,11 @@ int virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, virLogDestination dest, const void *opaque); int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs); int virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters); +virLogOutputPtr virLogNewOutputToStderr(virLogPriority priority); +virLogOutputPtr virLogNewOutputToFile(virLogPriority priority, + const char *file); +virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, + const char *ident); +virLogOutputPtr virLogNewOutputToJournald(int priority); #endif -- 2.5.5

Introduce a method to parse an individual logging output. The difference compared to the virLogParseAndDefineOutput is that this method does not define the output, instead it makes use of the virLogNewOutputTo* methods introduced in the previous patch and just returns the virLogOutput object that has to be added to a list of object which then can be defined as a whole via virLogDefineOutputs. The idea remains still the same - split parsing and defining of the logging primitives (outputs, filters). Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 73 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 39b0270..79a6adc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1867,6 +1867,7 @@ virLogOutputNew; virLogParseAndDefineFilters; virLogParseAndDefineOutputs; virLogParseDefaultPriority; +virLogParseOutput; virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; diff --git a/src/util/virlog.c b/src/util/virlog.c index 61e71a3..7a6e639 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1850,3 +1850,74 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters) return virLogNbFilters; } + +virLogOutputPtr +virLogParseOutput(const char *src) +{ + virLogOutputPtr ret = NULL; + char **tokens = NULL; + char *abspath = NULL; + size_t count = 0; + virLogPriority prio; + int dest; + bool isSUID = virIsSUID(); + + if (!src) + return NULL; + + VIR_DEBUG("output=%s", src); + + /* split our format prio:destination:additional_data to tokens and parse + * them individually + */ + if (!(tokens = virStringSplitCount(src, ":", 0, &count))) + return NULL; + + if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || + (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) + goto cleanup; + + if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0) + goto cleanup; + + if (((dest == VIR_LOG_TO_STDERR || + dest == VIR_LOG_TO_JOURNALD) && count != 2) || + ((dest == VIR_LOG_TO_FILE || + dest == VIR_LOG_TO_SYSLOG) && count != 3)) + goto cleanup; + + /* if running with setuid, only 'stderr' is allowed */ + if (isSUID && dest != VIR_LOG_TO_STDERR) + goto cleanup; + + switch ((virLogDestination) dest) { + case VIR_LOG_TO_STDERR: + ret = virLogNewOutputToStderr(prio); + break; + case VIR_LOG_TO_SYSLOG: +#if HAVE_SYSLOG_H + ret = virLogNewOutputToSyslog(prio, tokens[2]); +#endif + break; + case VIR_LOG_TO_FILE: + if (virFileAbsPath(tokens[2], &abspath) < 0) + goto cleanup; + ret = virLogNewOutputToFile(prio, abspath); + VIR_FREE(abspath); + break; + case VIR_LOG_TO_JOURNALD: +#if USE_JOURNALD + ret = virLogNewOutputToJournald(prio); +#endif + break; + case VIR_LOG_TO_OUTPUT_LAST: + break; + } + + cleanup: + if (!ret) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse and define log output %s"), src); + virStringFreeList(tokens); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index e3d5243..af26e30 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -245,5 +245,6 @@ virLogOutputPtr virLogNewOutputToFile(virLogPriority priority, virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, const char *ident); virLogOutputPtr virLogNewOutputToJournald(int priority); +virLogOutputPtr virLogParseOutput(const char *src); #endif -- 2.5.5

Another abstraction added on the top of parsing a single logging output. This method takes and parses the whole set of outputs, adding each single output that has already been parsed into a caller-provided array. If the user-supplied string contained duplicate outputs, only the last occurrence is taken into account (all the others are removed from the list), so we silently avoid duplicate logs and leaking journald fds. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 81 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1dfd7c8..b12ca92 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1869,6 +1869,7 @@ virLogParseAndDefineOutputs; virLogParseDefaultPriority; virLogParseFilter; virLogParseOutput; +virLogParseOutputs; virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; diff --git a/src/util/virlog.c b/src/util/virlog.c index 43b3d75..89e58cd 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1966,3 +1966,82 @@ virLogParseFilter(const char *filter) virStringFreeList(tokens); return ret; } + +/** + * virLogParseOutputs: + * @src: string defining a (set of) output(s) + * @outputs: user-supplied list where parsed outputs from @src shall be stored + * + * The format for an output can be: + * x:stderr + * output goes to stderr + * x:syslog:name + * use syslog for the output and use the given name as the ident + * x:file:file_path + * output to a file, with the given filepath + * In all case the x prefix is the minimal level, acting as a filter + * 1: DEBUG + * 2: INFO + * 3: WARNING + * 4: ERROR + * + * Multiple outputs can be defined within @src string, they just need to be + * separated by spaces. + * + * If running in setuid mode, then only the 'stderr' output will + * be allowed + * + * Returns the number of outputs parsed or -1 in case of error. + */ +int +virLogParseOutputs(const char *src, virLogOutputPtr **outputs) +{ + int ret = -1; + int at = -1; + size_t noutputs = 0; + size_t i; + char **strings = NULL; + virLogOutputPtr output = NULL; + virLogOutputPtr *list = NULL; + + if (!src) + return -1; + + VIR_DEBUG("outputs=%s", src); + + if (!(strings = virStringSplit(src, " ", 0))) + goto cleanup; + + for (i = 0; strings[i]; i++) { + /* virStringSplit may return empty strings */ + if (STREQ(strings[i], "")) + continue; + + if (!(output = virLogParseOutput(strings[i]))) + goto cleanup; + + /* let's check if a duplicate output does not already exist in which + * case we replace it with its last occurrence + */ + if ((at = virLogFindOutput(list, noutputs, output->dest, + output->name)) >= 0) { + virLogOutputFree(list[at]); + VIR_DELETE_ELEMENT(list, at, noutputs); + } + + if (VIR_APPEND_ELEMENT(list, noutputs, output) < 0) { + virLogOutputFree(output); + goto cleanup; + } + + virLogOutputFree(output); + } + + ret = noutputs; + *outputs = list; + cleanup: + if (ret < 0) + virLogOutputListFree(list, noutputs); + virStringFreeList(strings); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index e7f6b85..ed60c00 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -247,5 +247,6 @@ virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, virLogOutputPtr virLogNewOutputToJournald(int priority); virLogOutputPtr virLogParseOutput(const char *src); virLogFilterPtr virLogParseFilter(const char *src); +int virLogParseOutputs(const char *src, virLogOutputPtr **outputs); #endif -- 2.5.5

This API is the entry point to output modification of the logger. Currently, everything is done by virLogParseAndDefineOutputs. Parsing and defining will be split into two operations both handled by this method transparently. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 31 +++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 33 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 063fe5f..5fb30e6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1876,6 +1876,7 @@ virLogProbablyLogMessage; virLogReset; virLogSetDefaultPriority; virLogSetFromEnv; +virLogSetOutputs; virLogUnlock; virLogVMessage; diff --git a/src/util/virlog.c b/src/util/virlog.c index 4b2aa4d..a60c027 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -2110,3 +2110,34 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) virStringFreeList(strings); return ret; } + +/** + * virLogSetOutputs: + * @outputs: string defining a (set of) output(s) + * + * Replaces the current set of defined outputs with a new set of outputs. + * + * Returns the number of outputs successfully defined or -1 in case of an + * error. + */ +int +virLogSetOutputs(const char *src) +{ + int ret = -1; + int noutputs = 0; + virLogOutputPtr *outputs = NULL; + + if (virLogInitialize() < 0) + return -1; + + if ((noutputs = virLogParseOutputs(src, &outputs)) < 0) + goto cleanup; + + if ((ret = virLogDefineOutputs(outputs, noutputs)) < 0) + goto cleanup; + + cleanup: + if (ret < 0) + virLogOutputListFree(outputs, noutputs); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index 9ccc650..88e6ac8 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -197,6 +197,7 @@ void virLogOutputFree(virLogOutputPtr output); void virLogOutputListFree(virLogOutputPtr *list, int count); void virLogFilterFree(virLogFilterPtr filter); void virLogFilterListFree(virLogFilterPtr *list, int count); +int virLogSetOutputs(const char *outputs); /* * Internal logging API -- 2.5.5

Since virLogParseAndDefineOutputs is going to be stripped from 'output defining' logic, replace all relevant occurrences with virLogSetOutputs call to make the change transparent to all original callers (daemons mostly). Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- daemon/libvirtd.c | 6 +++--- src/locking/lock_daemon.c | 6 +++--- src/logging/log_daemon.c | 6 +++--- src/util/virlog.c | 2 +- tests/testutils.c | 11 +++++++++-- tests/virlogtest.c | 10 ++++++---- 6 files changed, 25 insertions(+), 16 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 8efa69d..1251208 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -694,7 +694,7 @@ daemonSetupLogging(struct daemonConfig *config, virLogParseAndDefineFilters(config->log_filters); if (virLogGetNbOutputs() == 0) - virLogParseAndDefineOutputs(config->log_outputs); + virLogSetOutputs(config->log_outputs); /* * Command line override for --verbose @@ -721,7 +721,7 @@ daemonSetupLogging(struct daemonConfig *config, if (virAsprintf(&tmp, "%d:journald", priority) < 0) goto error; - virLogParseAndDefineOutputs(tmp); + virLogSetOutputs(tmp); VIR_FREE(tmp); } } @@ -764,7 +764,7 @@ daemonSetupLogging(struct daemonConfig *config, if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) goto error; } - virLogParseAndDefineOutputs(tmp); + virLogSetOutputs(tmp); VIR_FREE(tmp); } diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 84c3029..9e736af 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -479,7 +479,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, virLogParseAndDefineFilters(config->log_filters); if (virLogGetNbOutputs() == 0) - virLogParseAndDefineOutputs(config->log_outputs); + virLogSetOutputs(config->log_outputs); /* * Command line override for --verbose @@ -499,7 +499,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, if (access("/run/systemd/journal/socket", W_OK) >= 0) { if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) goto error; - virLogParseAndDefineOutputs(tmp); + virLogSetOutputs(tmp); VIR_FREE(tmp); } } @@ -543,7 +543,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) goto error; } - virLogParseAndDefineOutputs(tmp); + virLogSetOutputs(tmp); VIR_FREE(tmp); } diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 48eece9..08ad6e0 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -407,7 +407,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, virLogParseAndDefineFilters(config->log_filters); if (virLogGetNbOutputs() == 0) - virLogParseAndDefineOutputs(config->log_outputs); + virLogSetOutputs(config->log_outputs); /* * Command line override for --verbose @@ -427,7 +427,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, if (access("/run/systemd/journal/socket", W_OK) >= 0) { if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) goto error; - virLogParseAndDefineOutputs(tmp); + virLogSetOutputs(tmp); VIR_FREE(tmp); } } @@ -471,7 +471,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) goto error; } - virLogParseAndDefineOutputs(tmp); + virLogSetOutputs(tmp); VIR_FREE(tmp); } diff --git a/src/util/virlog.c b/src/util/virlog.c index b1d2543..b336529 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1630,7 +1630,7 @@ virLogSetFromEnv(void) virLogParseAndDefineFilters(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS"); if (debugEnv && *debugEnv) - virLogParseAndDefineOutputs(debugEnv); + virLogSetOutputs(debugEnv); } diff --git a/tests/testutils.c b/tests/testutils.c index 8af8707..21687e5 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -871,6 +871,9 @@ int virTestMain(int argc, #ifdef TEST_OOM char *oomstr; #endif + size_t noutputs = 0; + virLogOutputPtr output = NULL; + virLogOutputPtr *outputs = NULL; if (getenv("VIR_TEST_FILE_ACCESS")) VIRT_TEST_PRELOAD(TEST_MOCK); @@ -910,8 +913,11 @@ int virTestMain(int argc, virLogSetFromEnv(); if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) { - if (virLogDefineOutput(virtTestLogOutput, virtTestLogClose, &testLog, - VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) < 0) + if (!(output = virLogOutputNew(virtTestLogOutput, virtTestLogClose, + &testLog, VIR_LOG_DEBUG, + VIR_LOG_TO_STDERR, NULL)) || + VIR_APPEND_ELEMENT(outputs, noutputs, output) < 0 || + virLogDefineOutputs(outputs, noutputs) < 0) return EXIT_FAILURE; } @@ -987,6 +993,7 @@ int virTestMain(int argc, fprintf(stderr, "%*s", 40 - (int)(testCounter % 40), ""); fprintf(stderr, " %-3zu %s\n", testCounter, ret == 0 ? "OK" : "FAIL"); } + virLogReset(); VIR_FREE(perl); return ret; } diff --git a/tests/virlogtest.c b/tests/virlogtest.c index afcd84a..b2c66f7 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -48,9 +48,10 @@ testLogParseOutputs(const void *opaque) { int ret = -1; int noutputs; + virLogOutputPtr *outputs = NULL; const struct testLogData *data = opaque; - noutputs = virLogParseAndDefineOutputs(data->str); + noutputs = virLogParseOutputs(data->str, &outputs); if (noutputs < 0) { if (!data->pass) { VIR_TEST_DEBUG("Got expected error: %s\n", @@ -70,7 +71,7 @@ testLogParseOutputs(const void *opaque) ret = 0; cleanup: - virLogReset(); + virLogOutputListFree(outputs, noutputs); return ret; } @@ -79,9 +80,10 @@ testLogParseFilters(const void *opaque) { int ret = -1; int nfilters; + virLogFilterPtr *filters = NULL; const struct testLogData *data = opaque; - nfilters = virLogParseAndDefineFilters(data->str); + nfilters = virLogParseFilters(data->str, &filters); if (nfilters < 0) { if (!data->pass) { VIR_TEST_DEBUG("Got expected error: %s\n", @@ -101,7 +103,7 @@ testLogParseFilters(const void *opaque) ret = 0; cleanup: - virLogReset(); + virLogFilterListFree(filters, nfilters); return ret; } -- 2.5.5

This is mainly virLogAddOutputTo* which were replaced by virLogNewOutputTo* and the previously poorly named ones virLogParseAndDefine* functions. All of these are unnecessary now, since all the original callers were transparently switched to the new model of separate parsing and defining logic. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 4 - src/util/virlog.c | 427 ----------------------------------------------- src/util/virlog.h | 12 -- 3 files changed, 443 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fec0b8b..58d0d7e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1842,9 +1842,7 @@ virLockSpaceReleaseResourcesForOwner; # util/virlog.h -virLogDefineFilter; virLogDefineFilters; -virLogDefineOutput; virLogDefineOutputs; virLogFilterFree; virLogFilterListFree; @@ -1864,8 +1862,6 @@ virLogNewOutputToSyslog; virLogOutputFree; virLogOutputListFree; virLogOutputNew; -virLogParseAndDefineFilters; -virLogParseAndDefineOutputs; virLogParseDefaultPriority; virLogParseFilter; virLogParseFilters; diff --git a/src/util/virlog.c b/src/util/virlog.c index 36c3a38..12e6d94 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -279,70 +279,6 @@ virLogFilterListFree(virLogFilterPtr *list, int count) /** - * virLogDefineFilter: - * @match: the pattern to match - * @priority: the priority to give to messages matching the pattern - * @flags: extra flags, see virLogFilterFlags enum - * - * Defines a pattern used for log filtering, it allow to select or - * reject messages independently of the default priority. - * The filter defines a rules that will apply only to messages matching - * the pattern (currently if @match is a substring of the message category) - * - * Returns -1 in case of failure or the filter number if successful - */ -int -virLogDefineFilter(const char *match, - virLogPriority priority, - unsigned int flags) -{ - size_t i; - int ret = -1; - char *mdup = NULL; - virLogFilterPtr filter = NULL; - - virCheckFlags(VIR_LOG_STACK_TRACE, -1); - - if (virLogInitialize() < 0) - return -1; - - if ((match == NULL) || (priority < VIR_LOG_DEBUG) || - (priority > VIR_LOG_ERROR)) - return -1; - - virLogLock(); - for (i = 0; i < virLogNbFilters; i++) { - if (STREQ(virLogFilters[i]->match, match)) { - virLogFilters[i]->priority = priority; - ret = i; - goto cleanup; - } - } - - if (VIR_STRDUP_QUIET(mdup, match) < 0) - goto cleanup; - - if (VIR_ALLOC_QUIET(filter) < 0) { - VIR_FREE(mdup); - goto cleanup; - } - - filter->match = mdup; - filter->priority = priority; - filter->flags = flags; - - if (VIR_APPEND_ELEMENT_QUIET(virLogFilters, virLogNbFilters, filter) < 0) - goto cleanup; - - virLogFiltersSerial++; - cleanup: - virLogUnlock(); - if (ret < 0) - virReportOOMError(); - return virLogNbFilters; -} - -/** * virLogResetOutputs: * * Removes the set of logging output defined. @@ -390,73 +326,6 @@ virLogOutputListFree(virLogOutputPtr *list, int count) } -/** - * virLogDefineOutput: - * @f: the function to call to output a message - * @c: the function to call to close the output (or NULL) - * @data: extra data passed as first arg to the function - * @priority: minimal priority for this filter, use 0 for none - * @dest: where to send output of this priority - * @name: optional name data associated with an output - * @flags: extra flag, currently unused - * - * Defines an output function for log messages. Each message once - * gone though filtering is emitted through each registered output. - * - * Returns -1 in case of failure or the output number if successful - */ -int -virLogDefineOutput(virLogOutputFunc f, - virLogCloseFunc c, - void *data, - virLogPriority priority, - virLogDestination dest, - const char *name, - unsigned int flags) -{ - char *ndup = NULL; - virLogOutputPtr output = NULL; - - virCheckFlags(0, -1); - - if (virLogInitialize() < 0) - return -1; - - if (f == NULL) - return -1; - - if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) { - if (!name) { - virReportOOMError(); - return -1; - } - if (VIR_STRDUP(ndup, name) < 0) - return -1; - } - - if (VIR_ALLOC_QUIET(output) < 0) { - VIR_FREE(ndup); - return -1; - } - - output->logInitMessage = true; - output->f = f; - output->c = c; - output->data = data; - output->priority = priority; - output->dest = dest; - output->name = ndup; - - virLogLock(); - if (VIR_APPEND_ELEMENT_QUIET(virLogOutputs, virLogNbOutputs, output)) - goto cleanup; - - cleanup: - virLogUnlock(); - return virLogNbOutputs; -} - - static int virLogFormatString(char **msg, int linenr, @@ -770,16 +639,6 @@ virLogCloseFd(void *data) } -static int -virLogAddOutputToStderr(virLogPriority priority) -{ - if (virLogDefineOutput(virLogOutputToFd, NULL, (void *)2L, priority, - VIR_LOG_TO_STDERR, NULL, 0) < 0) - return -1; - return 0; -} - - virLogOutputPtr virLogNewOutputToStderr(virLogPriority priority) { @@ -788,25 +647,6 @@ virLogNewOutputToStderr(virLogPriority priority) } -static int -virLogAddOutputToFile(virLogPriority priority, - const char *file) -{ - int fd; - - fd = open(file, O_CREAT | O_APPEND | O_WRONLY, S_IRUSR | S_IWUSR); - if (fd < 0) - return -1; - if (virLogDefineOutput(virLogOutputToFd, virLogCloseFd, - (void *)(intptr_t)fd, - priority, VIR_LOG_TO_FILE, file, 0) < 0) { - VIR_FORCE_CLOSE(fd); - return -1; - } - return 0; -} - - virLogOutputPtr virLogNewOutputToFile(virLogPriority priority, const char *file) @@ -893,28 +733,6 @@ virLogCloseSyslog(void *data ATTRIBUTE_UNUSED) } -static int -virLogAddOutputToSyslog(virLogPriority priority, - const char *ident) -{ - /* - * ident needs to be kept around on Solaris - */ - VIR_FREE(current_ident); - if (VIR_STRDUP(current_ident, ident) < 0) - return -1; - - openlog(current_ident, 0, 0); - if (virLogDefineOutput(virLogOutputToSyslog, virLogCloseSyslog, NULL, - priority, VIR_LOG_TO_SYSLOG, ident, 0) < 0) { - closelog(); - VIR_FREE(current_ident); - return -1; - } - return 0; -} - - virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, const char *ident) @@ -1162,22 +980,6 @@ static void virLogCloseJournald(void *data ATTRIBUTE_UNUSED) } -static int virLogAddOutputToJournald(int priority) -{ - if ((journalfd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) - return -1; - if (virSetInherit(journalfd, false) < 0) { - VIR_LOG_CLOSE(journalfd); - return -1; - } - if (virLogDefineOutput(virLogOutputToJournald, virLogCloseJournald, NULL, - priority, VIR_LOG_TO_JOURNALD, NULL, 0) < 0) { - return -1; - } - return 0; -} - - virLogOutputPtr virLogNewOutputToJournald(int priority) { @@ -1233,235 +1035,6 @@ int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) (*cur == '\r') || (*cur == '\\')) -static int -virLogParseAndDefineOutput(const char *src) -{ - int ret = -1; - char **tokens = NULL; - char *abspath = NULL; - size_t count = 0; - virLogPriority prio; - int dest; - bool isSUID = virIsSUID(); - - if (!src) - return -1; - - VIR_DEBUG("output=%s", src); - - /* split our format prio:destination:additional_data to tokens and parse - * them individually - */ - if (!(tokens = virStringSplitCount(src, ":", 0, &count))) - return -1; - - if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || - (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) - goto cleanup; - - if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0) - goto cleanup; - - if (((dest == VIR_LOG_TO_STDERR || - dest == VIR_LOG_TO_JOURNALD) && count != 2) || - ((dest == VIR_LOG_TO_FILE || - dest == VIR_LOG_TO_SYSLOG) && count != 3)) - goto cleanup; - - /* if running with setuid, only 'stderr' is allowed */ - if (isSUID && dest != VIR_LOG_TO_STDERR) - goto cleanup; - - switch ((virLogDestination) dest) { - case VIR_LOG_TO_STDERR: - ret = virLogAddOutputToStderr(prio); - break; - case VIR_LOG_TO_SYSLOG: -#if HAVE_SYSLOG_H - ret = virLogAddOutputToSyslog(prio, tokens[2]); -#endif - break; - case VIR_LOG_TO_FILE: - if (virFileAbsPath(tokens[2], &abspath) < 0) - goto cleanup; - ret = virLogAddOutputToFile(prio, abspath); - VIR_FREE(abspath); - break; - case VIR_LOG_TO_JOURNALD: -#if USE_JOURNALD - ret = virLogAddOutputToJournald(prio); -#endif - break; - case VIR_LOG_TO_OUTPUT_LAST: - break; - } - - cleanup: - if (ret < 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse and define log output %s"), src); - virStringFreeList(tokens); - return ret; -} - - -/** - * virLogParseAndDefineOutputs: - * @outputs: string defining a (set of) output(s) - * - * The format for an output can be: - * x:stderr - * output goes to stderr - * x:syslog:name - * use syslog for the output and use the given name as the ident - * x:file:file_path - * output to a file, with the given filepath - * In all case the x prefix is the minimal level, acting as a filter - * 1: DEBUG - * 2: INFO - * 3: WARNING - * 4: ERROR - * - * Multiple output can be defined in a single @output, they just need to be - * separated by spaces. - * - * If running in setuid mode, then only the 'stderr' output will - * be allowed - * - * Returns the number of output parsed or -1 in case of error. - */ -int -virLogParseAndDefineOutputs(const char *src) -{ - int ret = -1; - int count = 0; - size_t i; - char **strings = NULL; - - if (!src) - return -1; - - VIR_DEBUG("outputs=%s", src); - - if (!(strings = virStringSplit(src, " ", 0))) - goto cleanup; - - for (i = 0; strings[i]; i++) { - /* virStringSplit may return empty strings */ - if (STREQ(strings[i], "")) - continue; - - if (virLogParseAndDefineOutput(strings[i]) < 0) - goto cleanup; - - count++; - } - - ret = count; - cleanup: - virStringFreeList(strings); - return ret; -} - - -static int -virLogParseAndDefineFilter(const char *filter) -{ - int ret = -1; - size_t count = 0; - virLogPriority prio; - char **tokens = NULL; - unsigned int flags = 0; - char *ref = NULL; - - if (!filter) - return -1; - - VIR_DEBUG("filter=%s", filter); - - if (!(tokens = virStringSplitCount(filter, ":", 0, &count))) - return -1; - - if (count != 2) - goto cleanup; - - if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || - (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) - goto cleanup; - - ref = tokens[1]; - if (ref[0] == '+') { - flags |= VIR_LOG_STACK_TRACE; - ref++; - } - - if (!*ref) - goto cleanup; - - if (virLogDefineFilter(ref, prio, flags) < 0) - goto cleanup; - - ret = 0; - cleanup: - if (ret < 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse and define log filter %s"), filter); - virStringFreeList(tokens); - return ret; -} - -/** - * virLogParseAndDefineFilters: - * @filters: string defining a (set of) filter(s) - * - * The format for a filter is: - * x:name - * where name is a match string - * the x prefix is the minimal level where the messages should be logged - * 1: DEBUG - * 2: INFO - * 3: WARNING - * 4: ERROR - * - * Multiple filter can be defined in a single @filters, they just need to be - * separated by spaces. - * - * Returns the number of filter parsed or -1 in case of error. - */ -int -virLogParseAndDefineFilters(const char *filters) -{ - int ret = -1; - int count = 0; - size_t i; - char **strings = NULL; - - if (!filters) - return -1; - - VIR_DEBUG("filters=%s", filters); - - if (!(strings = virStringSplit(filters, " ", 0))) - goto cleanup; - - for (i = 0; strings[i]; i++) { - /* virStringSplit may return empty strings */ - if (STREQ(strings[i], "")) - continue; - - if (virLogParseAndDefineFilter(strings[i]) < 0) - goto cleanup; - - count++; - } - - ret = count; - cleanup: - virStringFreeList(strings); - return ret; -} - - /** * virLogGetDefaultPriority: * diff --git a/src/util/virlog.h b/src/util/virlog.h index 2f88f2f..6f5af2c 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -183,16 +183,6 @@ char *virLogGetOutputs(void); virLogPriority virLogGetDefaultPriority(void); int virLogSetDefaultPriority(virLogPriority priority); void virLogSetFromEnv(void); -int virLogDefineFilter(const char *match, - virLogPriority priority, - unsigned int flags); -int virLogDefineOutput(virLogOutputFunc f, - virLogCloseFunc c, - void *data, - virLogPriority priority, - virLogDestination dest, - const char *name, - unsigned int flags); void virLogOutputFree(virLogOutputPtr output); void virLogOutputListFree(virLogOutputPtr *list, int count); void virLogFilterFree(virLogFilterPtr filter); @@ -208,8 +198,6 @@ void virLogLock(void); void virLogUnlock(void); int virLogReset(void); int virLogParseDefaultPriority(const char *priority); -int virLogParseAndDefineFilters(const char *filters); -int virLogParseAndDefineOutputs(const char *output); int virLogPriorityFromSyslog(int priority); void virLogMessage(virLogSourcePtr source, virLogPriority priority, -- 2.5.5

Now that previous patches tried to refactor the code and split parsing and defining logic of logging primitives, there is no reason why we could not keep journald's fd within the journald output object the same way as we do for regular file-based outputs. Quite the opposite, by doing that we gain the benefit of creating a journald-based object the same way as we create file-based ones and therefore when replacing the existing set of outputs for a new one, journald does not need to be special cased due to its globally shared fd and the only remaining thing to special case is syslog. Additionally, making this change, we don't need the virLogCloseJournald routinei anymore, since as it stores the fd within the object, plain virLogCloseFd will suffice. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/util/virlog.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 34209d0..713cd0c 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -852,8 +852,6 @@ journalAddInt(struct journalState *state, const char *field, int value) state->iov += 4; } -static int journalfd = -1; - static void virLogOutputToJournald(virLogSourcePtr source, virLogPriority priority, @@ -865,10 +863,11 @@ virLogOutputToJournald(virLogSourcePtr source, unsigned int flags, const char *rawstr, const char *str ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) + void *data) { virCheckFlags(VIR_LOG_STACK_TRACE,); int buffd = -1; + int journalfd = (intptr_t) data; struct msghdr mh; struct sockaddr_un sa; union { @@ -974,15 +973,10 @@ virLogOutputToJournald(virLogSourcePtr source, } -static void virLogCloseJournald(void *data ATTRIBUTE_UNUSED) -{ - VIR_LOG_CLOSE(journalfd); -} - - static virLogOutputPtr virLogNewOutputToJournald(int priority) { + int journalfd; virLogOutputPtr ret = NULL; if ((journalfd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) @@ -993,9 +987,9 @@ virLogNewOutputToJournald(int priority) return NULL; } - if (!(ret = virLogOutputNew(virLogOutputToJournald, - virLogCloseJournald, NULL, - priority, VIR_LOG_TO_JOURNALD, NULL))) { + if (!(ret = virLogOutputNew(virLogOutputToJournald, virLogCloseFd, + (void *)(intptr_t) journalfd, priority, + VIR_LOG_TO_JOURNALD, NULL))) { VIR_LOG_CLOSE(journalfd); return NULL; } -- 2.5.5

On 08/18/2016 07:47 AM, Erik Skultety wrote: Eventually the split functions will be named virLogSetOutputs and the virLogParseOutputs will be reused, so this temporary rename is required to get through that processing. My first thought was why not call this Set or Update instead, but I see by patch 13 you create a Set function... And by 17 it doesn't matter what the name is... In any case - the adjustment for the commit message can be added or not, it helped me though... ACK John

On 08/18/2016 07:47 AM, Erik Skultety wrote: s/allocated/allocates ^^ This is unrelated (so far) it seems... Similar to previous patch, no reason for failure, but the following two do provide a failure reason as would the failure from virCheckFlags. So add some sort of error message if "priority" is out of range before returning NULL Also I would do the ATTRIBUTE_NONNULL(1) trick and avoid the failure due to !match s/;/ ATTRIBUTE_NONNULL(1); ACK with the adjustments, John

On 08/18/2016 07:47 AM, Erik Skultety wrote: This could theoretically be NULL and 0, right? If so, then say so in order to make it clear that the ability to log can be "removed"... If not true, then we need to either add the ATTRIBUTE_NONNULL(1) or check for NULL outputs. See below... Why? Beyond the virLogInitialize() everything is a void, so this could return -1 on failure and 0 on success. ?? unrelated loss of indent, restore... Similarly unrelated... Similarly unrelated.... I sense a theme with some rogue loss of space by your editor.... ... here ... here ... here ... here This one's OK - although consider my name change suggestion... This is where an ATTRIBUTE_NONNULL(1) *could* go if it's not expected to be NULL. ACK w/ adjustments John

On 08/18/2016 07:47 AM, Erik Skultety wrote: Alternatively you could define them using one of the ATTRIBUTE_* macros - IIRC it's ATTRIBUTE_UNUSED, but it might be something different. I remember seeing it once and using it, but cannot recall what my fingers typed! Thus it would be "static $struct ATTRIBUTE_UNUSED" for each of the new definitions that will be static and that no one is calling. Then later when something calls it, just remove the ATTRIBUTE_UNUSED Thus no need to modify libvirt_private.syms BTW: I was wondering how long it was going to be before the virLogOutputNew would be used... Personally I would move usage closer to definition, but that's not a big deal. These wouldn't be necessary with that ATTRIBUTE_UNUSED I believe... static virLogOutputPtr ATTRIBUTE_UNUSED (same for all 4) Should '2L' be 'stderr' instead? Magic numbers are always a bit tricky... hmm.. nevermind my earlier comment about not allowing NULL for parameter 2 in the prototype... Sigh - even though I read ahead I forgot this usage model. Reminder, if you move patch 19 to earlier, then journalfd needs to be passed here. I believe if you use the ATTRIBUTE_UNUSED is used, then these aren't necessary either. ACK (in principal) with the edits. John

On 08/18/2016 07:47 AM, Erik Skultety wrote: Is this essentially open coded virLogFindOutput? Or is it that the ->name and current_ident check that would trip that up based on the weirdness from the previous patch? If you modify the Find to do that STREQ_NULLABLE check - I think then that function could be used... BTW: Still see no reason to not have ret be 0... From the earlier comment... Conditional ACK... curious about usage of virFindLogOutput... John

On 08/18/2016 07:47 AM, Erik Skultety wrote: Again some intro comments would be nice. See patch 12 for your text... Make use of ATTRIBUTE_NONNULL(1) in the prototype How about a comment like the previous patch w/r/t what the expected format is... !! Here you check count, but previous patch you did not !! Still requires some sort of error message to indicate why there's a failure. That way the message below becomes unnecessary [1] Similar to previous, split the checks and be sure provide error message on prio failure... Hmmmm... I know this is just a cut-n-paste of previous code, so is this just a way to make sure that some string was provided and not some empty string or not just "+"? I guess it just seems odd - but could use an error message in any case. [1] Rather than a catch-all error message which will/could overwrite some previous valid ones, I think you should avoid printing this and stick to setting error messages within the checks above. s/;/ATTRIBUTE_NONNULL(1); ACK w/ adjustments John

On 08/18/2016 07:47 AM, Erik Skultety wrote: Comment works well if you read the previous patch... most of the above could be moved to patch 10. Use ATTRIBUTE_NONNULL(1) in prototype. Similar comment about using virStringSplitCount function Not sure this is necessary unless you use the _COPY append macro If you set list = NULL, then... ... the ret < 0 check is not necessary s/;/ATTRIBUTE_NONNULL(1); ACK w/ adjustments John

On 08/18/2016 07:47 AM, Erik Skultety wrote: Here you need the 'filters = NULL;', so the following doesn't undo what you just did. s/;/ATTRIBUTE_NONNULL(1); ACK w/ adjustments John

On 08/18/2016 07:47 AM, Erik Skultety wrote: Similar to my comments in patch 15 - I think this needs to be merged with patch 13 and of course the test program updated... ACK with that merge - I just don't like the idea of a 2 patch window where strange stuff could happen with some git bisect. John

On 21/09/16 19:55, John Ferlan wrote: Sure, no problem at all. I think having non-NULL data with NULL free callback in general is a valid use-case especially if the data is void * and you store integers in it. Anyway, the problem here are stderr and syslog where you have NULL in the close callback and a file descriptor in @data for the former case, which you really do not want to close anyway, and a valid close callback with NULL @data (syslog keeps the file descriptor private) for the latter. While I can imagine having a dummy close callback for stderr which would just return instantly (however I'd rather avoid that), I really don't want to pass an arbitrary pointer to @data for syslog-based output just to satisfy ATTRIBUTE_NONNULL(3), even though it would be ignored by the syslog close callback. Let me know if I misunderstood your thoughts, I'll continue fixing the rest of the patches in the meantime. Erik

Well, this way we spared one unnecessary variable, but whatever, I can always add it there to make it more readable I guess. Ouch, perfect catch, thanks, we would definitely lose the original if the append failed. I suppose you're right, since it issues memmove, I'll try some scenarios to compare the behaviour though. Thanks a lot. Erik