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

Continuing with the refactor, in order to later split output parsing and output defining, introduce a new function which will create a new virLogOutput object which parser will insert into a list with the list being eventually defined. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virlog.h | 6 ++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 35200a3..b5cee5f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1854,6 +1854,7 @@ virLogLock; virLogMessage; virLogOutputFree; virLogOutputListFree; +virLogOutputNew; virLogParseAndDefineFilters; virLogParseAndDefineOutputs; virLogParseDefaultPriority; diff --git a/src/util/virlog.c b/src/util/virlog.c index 3ada288..91c63a1 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -366,7 +366,6 @@ virLogOutputFree(virLogOutputPtr output) output->c(output->data); VIR_FREE(output->name); VIR_FREE(output); - } @@ -1551,3 +1550,57 @@ bool virLogProbablyLogMessage(const char *str) ret = true; return ret; } + + +/** + * virLogOutputNew: + * @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 (see virLogDestination) + * @name: optional name data associated with an output + * + * Allocates and returns a new log output object. The object has to be later + * defined, so that the output will be taken into account when emitting a + * message. + * + * Returns reference to a newly created object or NULL in case of failure. + */ +virLogOutputPtr +virLogOutputNew(virLogOutputFunc f, + virLogCloseFunc c, + void *data, + virLogPriority priority, + virLogDestination dest, + const char *name) +{ + virLogOutputPtr ret = NULL; + char *ndup = NULL; + + if (!f) + return NULL; + + if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) { + if (!name) + return NULL; + + if (VIR_STRDUP(ndup, name) < 0) + return NULL; + } + + if (VIR_ALLOC_QUIET(ret) < 0) { + VIR_FREE(ndup); + return NULL; + } + + ret->logInitMessage = true; + ret->f = f; + ret->c = c; + ret->data = data; + ret->priority = priority; + ret->dest = dest; + ret->name = ndup; + + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index de64f4c..fb32c41 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -226,5 +226,11 @@ void virLogVMessage(virLogSourcePtr source, 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); #endif -- 2.5.5

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

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

Outputs are a bit trickier than filters, because if the user(config)-specified set of outputs does contain duplicates. So, not only we would log twice, but we would also leek FD for journald, since that one is global and is overwritten every time a journald output was specified. For compatibility reasons, we cannot just error out and forbid the daemon to start if we find duplicate outputs which do not make sense. Instead, we could silently take into account only the last occurrence of the duplicate output and remove all the previous ones, so that the logger will not try to use them when it is looping over all of its registered outputs. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 32 ++++++++++++++++++++++++++++++++ src/util/virlog.h | 2 ++ 3 files changed, 35 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 088f9f3..d28405c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1847,6 +1847,7 @@ virLogDefineOutput; virLogFilterFree; virLogFilterListFree; virLogFilterNew; +virLogFindOutput; virLogGetDefaultPriority; virLogGetFilters; virLogGetNbFilters; diff --git a/src/util/virlog.c b/src/util/virlog.c index e4dc84b..5916ac8 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1650,3 +1650,35 @@ virLogFilterNew(const char *match, return ret; } + + +/** + * virLogFindOutput: + * @outputs: a list of outputs where to look for the output of type @dest + * @noutputs: number of elements in @outputs + * @dest: destination type of an output + * @opaque: opaque data to the method (only filename at the moment) + * + * Looks for an output of destination type @dest in the source list @outputs. + * If such an output exists, index of the object in the list is returned. + * In case of the destination being of type FILE also a comparison of the + * output's filename with @opaque is performed first. + * + * Returns the index of the object in the list or -1 if no object matching the + * specified @dest type and/or @opaque data one was found. + */ +int +virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, + virLogDestination dest, const void *opaque) +{ + size_t i; + const char *name = opaque; + + for (i = 0; i < noutputs; i++) { + if (dest == outputs[i]->dest && + (dest != VIR_LOG_TO_FILE || STREQ(outputs[i]->name, name))) + return i; + } + + return -1; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index a56d297..2045c06 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -235,5 +235,7 @@ virLogOutputPtr virLogOutputNew(virLogOutputFunc f, virLogFilterPtr virLogFilterNew(const char *match, virLogPriority priority, unsigned int flags); +int virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, + virLogDestination dest, const void *opaque); #endif -- 2.5.5

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: Similar to previous patch - can filters be NULL? in order to wipe out filters... s/error;/error. Although, like previous patch -1 or 0 can be returned... Copy/Paste error - s/b virLogResetFilters(); ? Similar copy/paste error s/b virLogNbFilters And since virLogResetFilters updates this, it's not required to do it twice right? Again, return 0 - you have 1 failure scenario, followed by a bunch of void calls. Nice to see the editor wasn't going crazy this time ;-) Add the ATTRIBUTE_NONNULL(1) if that's an unexpected input... Otherwise, ACK with adjustments. John

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

Now that we're in the critical section, syslog connection can be re-opened by issuing openlog, which is something that cannot be done beforehand, since syslog keeps its file descriptor private and changing the tag earlier might introduce a log inconsistency if something went wrong with preparing a new set of logging outputs in order to replace the existing one. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/util/virlog.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index c0b8b9a..61e71a3 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1794,16 +1794,35 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs, int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs) { + int ret = -1; + size_t i; + char *tmp = NULL; + if (virLogInitialize() < 0) return -1; virLogLock(); virLogResetOutputs(); + + /* nothing can go wrong now (except for malloc) and we're also holding the + * lock so it's safe to call openlog and change the message tag */ + for (i = 0; i < noutputs; i++) { + if (outputs[i]->dest == VIR_LOG_TO_SYSLOG) { + if (VIR_STRDUP_QUIET(tmp, outputs[i]->name) < 0) + goto cleanup; + VIR_FREE(current_ident); + current_ident = tmp; + openlog(current_ident, 0, 0); + } + } + virLogOutputs = outputs; virLogNbOutputs = noutputs; - virLogUnlock(); - return virLogNbOutputs; + ret = virLogNbOutputs; + cleanup: + virLogUnlock(); + return ret; } -- 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

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: Much of this text would be moved to patch 9. This function doesn't do any of those format checks. Again ATTRIBUTE_NONNULL(1) in the prototype You could use the Count version and then... ...rather than strings[i], it's < count In this case, the old one is also gone... So we've effectively removed it. Is that intentional? or should the DELETE of 'at' occur after this successfully adds a new one? IOW: at = virLogFindOutput() if (VIR_APPEND_ELEMENT() < 0) ... } if (at >= 0) { virLogOutputFree(list[at]); VIR_DELETE_ELEMENT(); } Is this right? I don't think it's necessary unless you change to using the _COPY append macro If you then set "list = NULL"... ... the (ret < 0) is not necessary s/;/ATTRIBUTE_NONNULL(1); Conditional ACK - guess I'm curious how the duplication and error path issue falls out... John

Same as with outputs. Another layer of abstraction, this provides support for parsing a set of filters (preferred way). Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virlog.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 67 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b12ca92..063fe5f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1868,6 +1868,7 @@ virLogParseAndDefineFilters; virLogParseAndDefineOutputs; virLogParseDefaultPriority; virLogParseFilter; +virLogParseFilters; virLogParseOutput; virLogParseOutputs; virLogPriorityFromSyslog; diff --git a/src/util/virlog.c b/src/util/virlog.c index 89e58cd..4b2aa4d 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -2045,3 +2045,68 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) virStringFreeList(strings); return ret; } + +/** + * virLogParseFilters: + * @src: string defining a (set of) filter(s) + * @filters: pointer to a list where the individual filters shall be parsed + * + * 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 + * + * This method parses @src and produces a list of individual filters which then + * needs to be passed to virLogDefineFilters in order to be set and taken into + * effect. + * Multiple filters can be defined in a single @src, they just need to be + * separated by spaces. + * + * Returns the number of filter parsed or -1 in case of error. + */ +int +virLogParseFilters(const char *src, virLogFilterPtr **filters) +{ + int ret = -1; + size_t count = 0; + size_t i; + char **strings = NULL; + virLogFilterPtr filter = NULL; + virLogFilterPtr *list = NULL; + + if (!src) + return -1; + + VIR_DEBUG("filters=%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 (!(filter = virLogParseFilter(strings[i]))) + goto cleanup; + + if (VIR_APPEND_ELEMENT(list, count, filter)) { + virLogFilterFree(filter); + goto cleanup; + } + + virLogFilterFree(filter); + } + + ret = count; + *filters = list; + cleanup: + if (ret < 0) + virLogFilterListFree(list, count); + virStringFreeList(strings); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index ed60c00..9ccc650 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -248,5 +248,6 @@ virLogOutputPtr virLogNewOutputToJournald(int priority); virLogOutputPtr virLogParseOutput(const char *src); virLogFilterPtr virLogParseFilter(const char *src); int virLogParseOutputs(const char *src, virLogOutputPtr **outputs); +int virLogParseFilters(const char *src, virLogFilterPtr **filters); #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

This method will eventually replace virLogParseAndDefineFilters which currently does both parsing and defining. 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 5fb30e6..fec0b8b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1875,6 +1875,7 @@ virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; virLogSetDefaultPriority; +virLogSetFilters; virLogSetFromEnv; virLogSetOutputs; virLogUnlock; diff --git a/src/util/virlog.c b/src/util/virlog.c index a60c027..b1d2543 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -2141,3 +2141,34 @@ virLogSetOutputs(const char *src) virLogOutputListFree(outputs, noutputs); return ret; } + + +/** + * virLogSetFilters: + * @src: string defining a (set of) filter(s) + * + * Replaces the current set of defined filters with a new set of filters. + * + * Returns the number of filters successfully defined or -1 in case of an + * error. + */ +int +virLogSetFilters(const char *src) +{ + int ret = -1; + int nfilters = 0; + virLogFilterPtr *filters = NULL; + + if (virLogInitialize() < 0) + return -1; + + if ((nfilters = virLogParseFilters(src, &filters)) < 0) + goto cleanup; + + if ((ret = virLogDefineFilters(filters, nfilters)) < 0) + goto cleanup; + + cleanup: + virLogFilterListFree(filters, nfilters); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index 88e6ac8..2f88f2f 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -198,6 +198,7 @@ void virLogOutputListFree(virLogOutputPtr *list, int count); void virLogFilterFree(virLogFilterPtr filter); void virLogFilterListFree(virLogFilterPtr *list, int count); int virLogSetOutputs(const char *outputs); +int virLogSetFilters(const char *filters); /* * 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

On 21/09/16 21:14, John Ferlan wrote: My original intention was to make it clear in a separate patch that that was the specific moment where everything would switch to the new versions transparently, I can squash it to 13 I just wanted to make it more clear and more easier for the reviewer, that's all. Actually, not just the output but outputs as well if define fails. In that case APPEND without _COPY still might be a better solution, since when APPEND fails, I can free the output, if APPEND succeeds, it clears output, so that when define fails afterwards, I can still free both the output and outputs (the list), since one of those will always be NULL in any case of failure, but thanks anyway. Oops, dunno how that hunk got there :/ (probably multiple interactive rebases as usual...) Erik

Similar to outputs, parser should do parsing only, thus the 'define' logic is going to be stripped from virLogParseAndDefineFilters by replacing calls to this method to virLogSetFilters instead. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- daemon/libvirtd.c | 2 +- src/locking/lock_daemon.c | 2 +- src/logging/log_daemon.c | 2 +- src/util/virlog.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 1251208..2eac827 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -691,7 +691,7 @@ daemonSetupLogging(struct daemonConfig *config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseAndDefineFilters(config->log_filters); + virLogSetFilters(config->log_filters); if (virLogGetNbOutputs() == 0) virLogSetOutputs(config->log_outputs); diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 9e736af..9ee818e 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -476,7 +476,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseAndDefineFilters(config->log_filters); + virLogSetFilters(config->log_filters); if (virLogGetNbOutputs() == 0) virLogSetOutputs(config->log_outputs); diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 08ad6e0..a9aebdb 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -404,7 +404,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseAndDefineFilters(config->log_filters); + virLogSetFilters(config->log_filters); if (virLogGetNbOutputs() == 0) virLogSetOutputs(config->log_outputs); diff --git a/src/util/virlog.c b/src/util/virlog.c index b336529..36c3a38 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1627,7 +1627,7 @@ virLogSetFromEnv(void) virLogParseDefaultPriority(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS"); if (debugEnv && *debugEnv) - virLogParseAndDefineFilters(debugEnv); + virLogSetFilters(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS"); if (debugEnv && *debugEnv) virLogSetOutputs(debugEnv); -- 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

On 08/18/2016 07:47 AM, Erik Skultety wrote: See my followup to patch 9. Using ATTRIBUTE_UNUSED will remove the need for this patch. John

On 08/18/2016 07:47 AM, Erik Skultety wrote: By this point I was too lazy to check, but we're not going to start seeing strange failures from some rogue/incorrect setting that previously essentially ignored this. You'll need to update the comments to this function about treturn values... Conditional ACK w/ adjustment and assurance about the above usage of virReportError vs. VIR_WARN John