On 08/18/2016 07:47 AM, Erik Skultety wrote:

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.

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.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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(-)

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

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",

On 08/18/2016 07:47 AM, Erik Skultety wrote:

Continuing with the refactor, in order to later split output parsing and output

s/Continuing with the refactor, i/I

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.

s/which/which the/

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virlog.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virlog.h | 6 ++++++ 3 files changed, 61 insertions(+), 1 deletion(-)

When I first started reviewing I wondered what the deal with 'journalfd' was - why was it a global and not handled like the file fd. Then eventually I read patch 19... Would it be too painful to move patch 19 to in between 1 and 2? It's not that important since in the long run things work out. If you do decide to make that move, then of course the intervening patch 7 would need to pass journalfd..

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;

I think instead of this, modify the prototype to be ATTRIBUTE_NONNULL(1) to ensure you're passed a function... [1]

+ + if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) { + if (!name) + return NULL;

The above two fail without a message which could percolate some day as a "failed for some unknown reason"

+ + if (VIR_STRDUP(ndup, name) < 0) + return NULL; + } + + if (VIR_ALLOC_QUIET(ret) < 0) { + VIR_FREE(ndup); + return NULL; + }

The above two will fail with a OOM which is fine - just pointing out the difference... So if you add a message for the first !name check that would be good.

+ + ret->logInitMessage = true; + ret->f = f; + ret->c = c; + ret->data = data;

From future patches I see this can be a file or syslog fd.

Anyway, because you're relying on the "->c" to be the free function for ->data, perhaps there should be a check above that causes an error if "data" was passed as non-NULL, but ->c is NULL; otherwise, in some future world someone may begin to leak data unexpectedly. (and if patch 19 moves before here, then "data" could be further checked to be non NULL (I think). In fact in this case, the ATTRIBUTE_NONNULL(2) and ATTRIBUTE_NONNULL(3) could be used instead of an error.

+ 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);

[1] s/);/) ATTRIBUTE_NONNULL(1); ACK with the adjustments, John

#endif

On 08/18/2016 07:47 AM, Erik Skultety wrote:

This method allocated a new filter object which it then returns back to caller.

s/allocated/allocates

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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;

^^ This is unrelated (so far) it seems...

+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;

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

+ + 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);

s/;/ ATTRIBUTE_NONNULL(1); ACK with the adjustments, John

#endif

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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

"because the user(config) specified set of outputs can contain duplicates."

we would also leek FD for journald, since that one is global and is overwritten

s/leek/leak Which may not be true any more if patch 19 is moved. ACK - John

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@redhat.com> --- 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

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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@redhat.com> --- src/libvirt_private.syms | 4 ++ src/util/virlog.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 6 +++ 3 files changed, 108 insertions(+)

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.

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;

These wouldn't be necessary with that ATTRIBUTE_UNUSED I believe...

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

static virLogOutputPtr ATTRIBUTE_UNUSED (same for all 4)

+virLogNewOutputToStderr(virLogPriority priority) +{ + return virLogOutputNew(virLogOutputToFd, NULL, (void *)2L, priority, + VIR_LOG_TO_STDERR, NULL);

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.

+} + + 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,

Reminder, if you move patch 19 to earlier, then journalfd needs to be passed here.

+ 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);

I believe if you use the ATTRIBUTE_UNUSED is used, then these aren't necessary either. ACK (in principal) with the edits. John

#endif

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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@redhat.com> --- 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) {

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...

+ 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;

BTW: Still see no reason to not have ret be 0... From the earlier comment... Conditional ACK... curious about usage of virFindLogOutput... John

+ cleanup: + virLogUnlock(); + return ret; }

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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@redhat.com> --- 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; }

You've done well so far at adding comments to functions, but didn't do so for this... Let's add something about usage, returns, etc. I think you'll find the text you need in patch 11...

+ +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;

Similar to earlier comment, use ATTRIBUTE_NONNULL(1) in the prototype and avoid this check.

+ + 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;

Not that it could happen ;-), but count could probably be "validated" to be at least some value since you're checking tokens[0] and tokens[1] IOW: "if (count < 2)" then error with some malformed line message.

+ + if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || + (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR))

So the first part of the if would return an error message; however, the latter 'prio' range check doesn't. So I would think the two need to be separated and if we fail due to prio range check a message generated. [1] that also removes the need for the message below...

+ 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))

Again would be nice to know why we're failing.

+ goto cleanup; + + /* if running with setuid, only 'stderr' is allowed */ + if (isSUID && dest != VIR_LOG_TO_STDERR)

Same for error message.

+ 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);

[1] Rather than a catch-all error message which will/could overwrite some previous valid ones (such as OOM or virStrToLong failure), I think you should avoid printing this and stick to setting error messages within the checks above.

+ 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);

s/;/ATTRIBUTE_NONNULL(1); ACK w/ the adjustments John

#endif

...

+ + VIR_DEBUG("outputs=%s", src); + + if (!(strings = virStringSplit(src, " ", 0)))

You could use the Count version and then...

...

+ goto cleanup; + + for (i = 0; strings[i]; i++) {

...rather than strings[i], it's < count

Well, this way we spared one unnecessary variable, but whatever, I can always add it there to make it more readable I guess.

...

+ /* 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);

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(); }

Ouch, perfect catch, thanks, we would definitely lose the original if the append failed.

...

+ goto cleanup; + } + + virLogOutputFree(output);

Is this right? I don't think it's necessary unless you change to using the _COPY append macro

I suppose you're right, since it issues memmove, I'll try some scenarios to compare the behaviour though.

...

+ } + + ret = noutputs; + *outputs = list;

If you then set "list = NULL"...

...

+ cleanup: + if (ret < 0)

... the (ret < 0) is not necessary

...

+ 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);

s/;/ATTRIBUTE_NONNULL(1);

Conditional ACK - guess I'm curious how the duplication and error path issue falls out...

Thanks a lot. Erik

John

...

#endif

On 08/18/2016 07:47 AM, Erik Skultety wrote:

Same as with outputs. Another layer of abstraction, this provides support for parsing a set of filters (preferred way).

Comment works well if you read the previous patch...

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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. + *

most of the above could be moved to patch 10.

+ * 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;

Use ATTRIBUTE_NONNULL(1) in prototype.

+ + VIR_DEBUG("filters=%s", src); + + if (!(strings = virStringSplit(src, " ", 0))) + goto cleanup; + + for (i = 0; strings[i]; i++) {

Similar comment about using virStringSplitCount function

+ /* 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);

Not sure this is necessary unless you use the _COPY append macro

+ } + + ret = count; + *filters = list;

If you set list = NULL, then...

+ cleanup: + if (ret < 0)

... the ret < 0 check is not necessary

+ 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);

s/;/ATTRIBUTE_NONNULL(1); ACK w/ adjustments John

#endif

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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@redhat.com> --- 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; +

If you set outputs = NULL here, then the following "if (ret < 0)" isn't necessary

+ 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);

s/;/ ATTRIBUTE_NONNULL(1); ACK w/ adjustments John

/* * Internal logging API

On 08/18/2016 07:47 AM, Erik Skultety wrote:

This method will eventually replace virLogParseAndDefineFilters which currently does both parsing and defining.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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; +

Here you need the 'filters = NULL;', so the following doesn't undo what you just did.

+ 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);

s/;/ATTRIBUTE_NONNULL(1); ACK w/ adjustments John

/* * Internal logging API

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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).

I think the commit message needs to be adjusted... What's really happening is that now that you have a "real" virLogParseOutputs and a virLogSetOutputs, the "ParseAndDefine" is being duplicitous so you're replacing them with the Set call. Personally, I think this patch should be combined with Patch 13 so we don't have that duplicity and/or any other "strangeness" as a result of having the ParseAndDefine being called as well as the Parse.

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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)

I know - just a test, but you could leak output if the APPEND fails. You could use the _COPY macro, then do the Free in both the error and success case...

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; }

Shouldn't the rest of this go with the "next" patch which does the Filter magic (it's going to have a similar merge with patch comment) Conditional ACK on moving/merging with patch 13. Beyond that just a small justification for the separation... w/r/t: memory leak on output, that will probably raise the ire of Coverity or running with valgrind. John

@@ -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; }

On 10/06/2016 06:42 AM, Erik Skultety wrote:

On 21/09/16 21:14, John Ferlan wrote:

...

On 08/18/2016 07:47 AM, Erik Skultety wrote:

...

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).

I think the commit message needs to be adjusted... What's really happening is that now that you have a "real" virLogParseOutputs and a virLogSetOutputs, the "ParseAndDefine" is being duplicitous so you're replacing them with the Set call.

Personally, I think this patch should be combined with Patch 13 so we don't have that duplicity and/or any other "strangeness" as a result of having the ParseAndDefine being called as well as the Parse.

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.

Understood - the ordering and separation did make review easier. I think in 20/20 hindsight though - since nothing calls SetOutputs until now that the duplicity and bisect concern I noted cannot happen. Similarly for patch 16's comments... Keep the separation as is - that's fine. John

...

...

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)

I know - just a test, but you could leak output if the APPEND fails. You could use the _COPY macro, then do the Free in both the error and success case...

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.

...

Shouldn't the rest of this go with the "next" patch which does the Filter magic (it's going to have a similar merge with patch comment)

Oops, dunno how that hunk got there :/ (probably multiple interactive rebases as usual...)

Erik

...

Conditional ACK on moving/merging with patch 13. Beyond that just a small justification for the separation...

w/r/t: memory leak on output, that will probably raise the ire of Coverity or running with valgrind.

John

...

@@ -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; }

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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.

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

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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);

On 08/18/2016 07:47 AM, Erik Skultety wrote:

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@redhat.com> --- src/libvirt_private.syms | 4 - src/util/virlog.c | 427 ----------------------------------------------- src/util/virlog.h | 12 -- 3 files changed, 443 deletions(-)

NOTE: My git am -3 failed on this patch: Applying: virlog: Remove functions that aren't used anywhere anymore fatal: sha1 information is lacking or useless (src/libvirt_private.syms). error: could not build fake ancestor Patch failed at 0017 virlog: Remove functions that aren't used anywhere anymore The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". I'm really not sure why, but I NOTE: Existing, but IS_SPACE isn't used either. ACK - you could/should remove IS_SPACE separately and use this implied ACK John

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,

On 21/09/16 22:01, John Ferlan wrote:

On 08/18/2016 07:47 AM, Erik Skultety wrote:

...

Handling of outputs and filters has been changed in a way that splits parsing and defining. Do the same thing for logging priority as well, this however, doesn't need much of a preparation. --- src/util/virlog.c | 21 +++++++++------------ tests/eventtest.c | 3 ++- 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/src/util/virlog.c b/src/util/virlog.c index 713cd0c..683cf6b 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -220,7 +220,9 @@ int virLogSetDefaultPriority(virLogPriority priority) { if ((priority < VIR_LOG_DEBUG) || (priority > VIR_LOG_ERROR)) { - VIR_WARN("Ignoring invalid log level setting."); + virReportError(VIR_ERR_INVALID_ARG, + _("Failed to set logging priority, argument '%u' is " + "invalid"), priority);

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.

No, we won't. The only thing that changed is that now we'll see issues logged as errors rather than warnings, but so far, each of the original callers ignores the outcome of the function, so no unexpected crashes/behaviour related to incorrect setting shouldn't take place, if the new setting is incorrect, the original setting will stay intact. Erik

...

return -1; } if (virLogInitialize() < 0) @@ -1158,20 +1160,15 @@ virLogGetNbOutputs(void)

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