In this particular case, reset is meant as clearing the whole list of outputs/filters, not resetting it to a predefined default setting. Looking at it from that perspective, returning the number of records removed doesn't help the caller in any way (not that any of the callers would actually check for it). Well, callers could detect an error from the number of successfully removed records, but the only thing that can fail in virLogReset is force closing a file descriptor in which case the error isn't propagated back to virLogReset anyway. Conclusion: there is no practical use for having a return type of 'int' rather than 'void' in this case. --- src/util/virlog.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 738eaac..6d11328 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -117,8 +117,8 @@ static int virLogNbOutputs; */ static virLogPriority virLogDefaultPriority = VIR_LOG_DEFAULT; -static int virLogResetFilters(void); -static int virLogResetOutputs(void); +static void virLogResetFilters(void); +static void virLogResetOutputs(void); static void virLogOutputToFd(virLogSourcePtr src, virLogPriority priority, const char *filename, @@ -239,10 +239,8 @@ virLogSetDefaultPriority(virLogPriority priority) * virLogResetFilters: * * Removes the set of logging filters defined. - * - * Returns the number of filters removed */ -static int +static void virLogResetFilters(void) { size_t i; @@ -252,7 +250,6 @@ virLogResetFilters(void) VIR_FREE(virLogFilters); virLogNbFilters = 0; virLogFiltersSerial++; - return i; } @@ -319,10 +316,8 @@ virLogDefineFilter(const char *match, * virLogResetOutputs: * * Removes the set of logging output defined. - * - * Returns the number of output removed */ -static int +static void virLogResetOutputs(void) { size_t i; @@ -333,9 +328,7 @@ virLogResetOutputs(void) VIR_FREE(virLogOutputs[i].name); } VIR_FREE(virLogOutputs); - i = virLogNbOutputs; virLogNbOutputs = 0; - return i; } -- 2.4.11

On 05/04/2016 10:30 AM, Erik Skultety wrote: ACK, but IMO exporting it early in a separate patch without context makes it hard to follow the reasoning. Better would have been to export it with the first public function that needs it, looks like virLogDefineOutputs - Cole

On 10/05/16 12:29, Erik Skultety wrote: So, since there was a gap in the ACKed patches, I moved 15-18 a bit to front, squashed 4 and 5 into them (originally I squashed them into 10 and 11, but I didn't get an ACK on those. Anyhow exporting a pointer type wouldn't make a difference in any of those cases...) and pushed. Thanks, Erik

On 05/04/2016 10:30 AM, Erik Skultety wrote: ACK - Cole

This API is the entry point to output modification of the logger. Currently, everything is done by virLogParseOutputs which should do parsing only and a separate method which will do the definition part should be added. --- src/libvirt_private.syms | 1 + src/util/virlog.c | 23 +++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 25 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65fafeb..dafcb6c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1802,6 +1802,7 @@ virLogReset; virLogSetDefaultPriority; virLogSetFilters; virLogSetFromEnv; +virLogSetOutputs; virLogUnlock; virLogVMessage; diff --git a/src/util/virlog.c b/src/util/virlog.c index 0aa722d..2cd3773 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1516,3 +1516,26 @@ virLogSetFilters(const char *filters) 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 *outputs) +{ + int ret = -1; + + if (virLogInitialize() < 0) + return -1; + + ret = virLogParseOutputs(outputs); + + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index 4d7cc98..4141ecd 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -184,6 +184,7 @@ extern virLogPriority virLogGetDefaultPriority(void); extern int virLogSetDefaultPriority(virLogPriority priority); extern void virLogSetFromEnv(void); extern int virLogSetFilters(const char *filters); +extern int virLogSetOutputs(const char *outputs); extern int virLogDefineFilter(const char *match, virLogPriority priority, unsigned int flags); -- 2.4.11

Similar to outputs, parser should do parsing only, thus the 'define' logic is going to be stripped from virLogParseFilters. Make this change transparent to all callers by using virLogSetFilters which has already been introduced earlier in this series. --- 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 d0019b0..0f15ffb 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -690,7 +690,7 @@ daemonSetupLogging(struct daemonConfig *config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseFilters(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 8d760b2..cce70e7 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -476,7 +476,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseFilters(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 f8a96e8..f0e8ea0 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -399,7 +399,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, virLogSetFromEnv(); if (virLogGetNbFilters() == 0) - virLogParseFilters(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 538a5b8..9059722 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1472,7 +1472,7 @@ virLogSetFromEnv(void) virLogParseDefaultPriority(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS"); if (debugEnv && *debugEnv) - virLogParseFilters(debugEnv); + virLogSetFilters(debugEnv); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS"); if (debugEnv && *debugEnv) virLogSetOutputs(debugEnv); -- 2.4.11

Prepare a method that only defines a set of filters. It takes a list of filters, preferably created by virLogParseFilters. The original set of filters is reset and replaced by the new user-provided set of filters. --- src/libvirt_private.syms | 1 + src/util/virlog.c | 25 +++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 27 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 565fc7c..9f0dea0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1785,6 +1785,7 @@ virLockSpaceReleaseResourcesForOwner; # util/virlog.h virLogDefineFilter; +virLogDefineFilters; virLogDefineOutput; virLogDefineOutputs; virLogGetDefaultPriority; diff --git a/src/util/virlog.c b/src/util/virlog.c index e5923ce..60573a9 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1564,3 +1564,28 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs) return virLogNbOutputs; } + +/** + * virLogDefineFilters: + * @filters: new set of filters to be defined + * @nfilters: number of filters in @filters + * + * Resets any existing set of filters and defines a completely new one. + * + * Returns number of filters successfully defined or -1 in case of error; + */ +int +virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters) +{ + if (virLogInitialize() < 0) + return -1; + + virLogLock(); + virLogResetOutputs(); + virLogFilters = filters; + virLogNbOutputs = nfilters; + virLogFiltersSerial++; + virLogUnlock(); + + return virLogNbFilters; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index 166db5a..ff16034 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -225,5 +225,6 @@ extern void virLogVMessage(virLogSourcePtr source, bool virLogProbablyLogMessage(const char *str); int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs); +int virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters); #endif -- 2.4.11

Preparation for the fact, that opposite to the original define method that took an already existing global set of outputs and appended/defined a new output, virLogOutputNew only creates a new output, but caller is responsible for appending the output to a list and managing the list itself. --- src/libvirt_private.syms | 2 +- src/util/virlog.c | 34 +++++++++++++++++----------------- src/util/virlog.h | 14 +++++++------- tests/testutils.c | 4 ++-- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f0dea0..1f601b1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1786,7 +1786,6 @@ virLockSpaceReleaseResourcesForOwner; # util/virlog.h virLogDefineFilter; virLogDefineFilters; -virLogDefineOutput; virLogDefineOutputs; virLogGetDefaultPriority; virLogGetFilters; @@ -1795,6 +1794,7 @@ virLogGetNbOutputs; virLogGetOutputs; virLogLock; virLogMessage; +virLogOutputNew; virLogParseDefaultPriority; virLogParseFilters; virLogParseOutputs; diff --git a/src/util/virlog.c b/src/util/virlog.c index 387e08a..ec942a3 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -334,7 +334,7 @@ virLogResetOutputs(void) /** - * virLogDefineOutput: + * 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 @@ -349,13 +349,13 @@ virLogResetOutputs(void) * 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) +virLogOutputNew(virLogOutputFunc f, + virLogCloseFunc c, + void *data, + virLogPriority priority, + virLogDestination dest, + const char *name, + unsigned int flags) { char *ndup = NULL; virLogOutputPtr output = NULL; @@ -716,8 +716,8 @@ virLogCloseFd(void *data) static int virLogNewOutputToStderr(virLogPriority priority) { - if (virLogDefineOutput(virLogOutputToFd, NULL, (void *)2L, priority, - VIR_LOG_TO_STDERR, NULL, 0) < 0) + if (virLogOutputNew(virLogOutputToFd, NULL, (void *)2L, priority, + VIR_LOG_TO_STDERR, NULL, 0) < 0) return -1; return 0; } @@ -732,9 +732,9 @@ virLogNewOutputToFile(virLogPriority priority, 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) { + if (virLogOutputNew(virLogOutputToFd, virLogCloseFd, + (void *)(intptr_t)fd, + priority, VIR_LOG_TO_FILE, file, 0) < 0) { VIR_FORCE_CLOSE(fd); return -1; } @@ -819,8 +819,8 @@ virLogNewOutputToSyslog(virLogPriority priority, return -1; openlog(current_ident, 0, 0); - if (virLogDefineOutput(virLogOutputToSyslog, virLogCloseSyslog, NULL, - priority, VIR_LOG_TO_SYSLOG, ident, 0) < 0) { + if (virLogOutputNew(virLogOutputToSyslog, virLogCloseSyslog, NULL, + priority, VIR_LOG_TO_SYSLOG, ident, 0) < 0) { closelog(); VIR_FREE(current_ident); return -1; @@ -1039,8 +1039,8 @@ static int virLogNewOutputToJournald(int priority) VIR_LOG_CLOSE(journalfd); return -1; } - if (virLogDefineOutput(virLogOutputToJournald, virLogCloseJournald, NULL, - priority, VIR_LOG_TO_JOURNALD, NULL, 0) < 0) { + if (virLogOutputNew(virLogOutputToJournald, virLogCloseJournald, NULL, + priority, VIR_LOG_TO_JOURNALD, NULL, 0) < 0) { return -1; } return 0; diff --git a/src/util/virlog.h b/src/util/virlog.h index ff16034..27037a4 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -188,13 +188,13 @@ extern int virLogSetOutputs(const char *outputs); extern int virLogDefineFilter(const char *match, virLogPriority priority, unsigned int flags); -extern int virLogDefineOutput(virLogOutputFunc f, - virLogCloseFunc c, - void *data, - virLogPriority priority, - virLogDestination dest, - const char *name, - unsigned int flags); +extern int virLogOutputNew(virLogOutputFunc f, + virLogCloseFunc c, + void *data, + virLogPriority priority, + virLogDestination dest, + const char *name, + unsigned int flags); /* * Internal logging API diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..1b31eca 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -869,8 +869,8 @@ int virtTestMain(int argc, virLogSetFromEnv(); if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) { - if (virLogDefineOutput(virtTestLogOutput, virtTestLogClose, &testLog, - VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) < 0) + if (virLogOutputNew(virtTestLogOutput, virtTestLogClose, &testLog, + VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) < 0) return EXIT_FAILURE; } -- 2.4.11

Add a complementary method to virLogOutputNew. --- src/libvirt_private.syms | 1 + src/util/virlog.c | 21 ++++++++++++++++----- src/util/virlog.h | 1 + 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 56dfcf4..b40a405 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1794,6 +1794,7 @@ virLogGetNbOutputs; virLogGetOutputs; virLogLock; virLogMessage; +virLogOutputFree; virLogOutputNew; virLogParseDefaultPriority; virLogParseFilters; diff --git a/src/util/virlog.c b/src/util/virlog.c index e101484..e36ff73 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -323,16 +323,27 @@ virLogResetOutputs(void) { size_t i; - for (i = 0; i < virLogNbOutputs; i++) { - if (virLogOutputs[i]->c != NULL) - virLogOutputs[i]->c(virLogOutputs[i]->data); - VIR_FREE(virLogOutputs[i]->name); - } + for (i = 0; i < virLogNbOutputs; i++) + virLogOutputFree(virLogOutputs[i]); + VIR_FREE(virLogOutputs); virLogNbOutputs = 0; } +void +virLogOutputFree(virLogOutputPtr output) +{ + if (!output) + return; + + if (output->c) + output->c(output->data); + VIR_FREE(output->name); + VIR_FREE(output); + +} + /** * virLogOutputNew: * @f: the function to call to output a message diff --git a/src/util/virlog.h b/src/util/virlog.h index 93456db..7573984 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -195,6 +195,7 @@ extern int virLogOutputNew(virLogOutputFunc f, virLogDestination dest, const char *name, unsigned int flags); +extern void virLogOutputFree(virLogOutputPtr output); /* * Internal logging API -- 2.4.11

This is just a convenience method for discarding a list of outputs instead of using a 'for' loop everywhere. It is safe to pass -1 as the number of elements in the list as well as passing NULL as list reference. --- src/libvirt_private.syms | 1 + src/util/virlog.c | 20 ++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 22 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b40a405..608d959 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1795,6 +1795,7 @@ virLogGetOutputs; virLogLock; virLogMessage; virLogOutputFree; +virLogOutputListFree; virLogOutputNew; virLogParseDefaultPriority; virLogParseFilters; diff --git a/src/util/virlog.c b/src/util/virlog.c index e36ff73..5da1af7 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1600,3 +1600,23 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters) return virLogNbFilters; } + +/** + * virLogOutputsFreeList: + * @list: list of outputs to be freed + * @count: number of elements in the list + * + * Frees a list of outputs. + */ +void +virLogOutputListFree(virLogOutputPtr *list, int count) +{ + size_t i; + + if (!list || count < 0) + return; + + for (i = 0; i < count; i++) + virLogOutputFree(list[i]); + VIR_FREE(list); +} diff --git a/src/util/virlog.h b/src/util/virlog.h index 7573984..4f0eea7 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -196,6 +196,7 @@ extern int virLogOutputNew(virLogOutputFunc f, const char *name, unsigned int flags); extern void virLogOutputFree(virLogOutputPtr output); +extern void virLogOutputListFree(virLogOutputPtr *list, int count); /* * Internal logging API -- 2.4.11

On 10/05/16 03:05, Cole Robinson wrote: Yep, I did that and pushed along with 15. Thanks, Erik

This is just a convenience method for discarding a list of filters instead of using a 'for' loop everywhere. It is safe to pass -1 as the number of elements in the list as well as passing NULL as list reference. --- src/util/virlog.c | 20 ++++++++++++++++++++ src/util/virlog.h | 1 + 2 files changed, 21 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index 36fecda..a1f5872 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1630,3 +1630,23 @@ virLogFilterFree(virLogFilterPtr filter) VIR_FREE(filter->match); VIR_FREE(filter); } + +/** + * virLogFilterFreeList: + * @list: list of filters to be freed + * @count: number of elements in the list + * + * Frees a list of filters. + */ +void +virLogFilterListFree(virLogFilterPtr *list, int count) +{ + size_t i; + + if (!list || count < 0) + return; + + for (i = 0; i < count; i++) + virLogFilterFree(list[i]); + VIR_FREE(list); +} diff --git a/src/util/virlog.h b/src/util/virlog.h index eec3a5d..eca7894 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -198,6 +198,7 @@ extern int virLogOutputNew(virLogOutputFunc f, extern void virLogOutputFree(virLogOutputPtr output); extern void virLogFilterFree(virLogFilterPtr filter); extern void virLogOutputListFree(virLogOutputPtr *list, int count); +extern void virLogFilterListFree(virLogFilterPtr *list, int count); /* * Internal logging API -- 2.4.11

On 05/04/2016 10:30 AM, Erik Skultety wrote: I'd rather see this patch split and combined in with the previous two patches. Like you did for the virLogOutputFree patch. With those changes, ACK to the previous two patches, and would be fine to push now since this is a reasonable cleanup IMO - Cole

Everything has been prepared to successfully split parsing and defining logic to separate operations. --- src/util/virlog.c | 160 +++++++++++++++++++++++++++++------------------------ src/util/virlog.h | 15 +++-- tests/testutils.c | 19 ++++++- tests/virlogtest.c | 5 +- 4 files changed, 114 insertions(+), 85 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 7e0936c..369d7dd 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -343,64 +343,50 @@ virLogOutputFree(virLogOutputPtr output) * @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 + * @dest: where to send output of this priority (see virLogDestination) * @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. + * gone through filtering is emitted through each registered output. * - * Returns -1 in case of failure or the output number if successful + * Returns reference to a newly created object or NULL in case of failure. */ -int +virLogOutputPtr virLogOutputNew(virLogOutputFunc f, virLogCloseFunc c, void *data, virLogPriority priority, virLogDestination dest, - const char *name, - unsigned int flags) + const char *name) { + virLogOutputPtr ret = NULL; char *ndup = NULL; - virLogOutputPtr output = NULL; - - virCheckFlags(0, -1); - - if (virLogInitialize() < 0) - return -1; - if (f == NULL) - return -1; + if (!f) + return NULL; if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) { - if (!name) { - virReportOOMError(); - return -1; - } + if (!name) + return NULL; + if (VIR_STRDUP(ndup, name) < 0) - return -1; + return NULL; } - if (VIR_ALLOC_QUIET(output) < 0) { + if (VIR_ALLOC_QUIET(ret) < 0) { VIR_FREE(ndup); - return -1; + return NULL; } - output->logInitMessage = true; - output->f = f; - output->c = c; - output->data = data; - output->priority = priority; - output->dest = dest; - output->name = ndup; + ret->logInitMessage = true; + ret->f = f; + ret->c = c; + ret->data = data; + ret->priority = priority; + ret->dest = dest; + ret->name = ndup; - virLogLock(); - if (VIR_APPEND_ELEMENT_QUIET(virLogOutputs, virLogNbOutputs, output)) - goto cleanup; - - cleanup: - virLogUnlock(); - return virLogNbOutputs; + return ret; } @@ -717,32 +703,32 @@ virLogCloseFd(void *data) } -static int +static virLogOutputPtr virLogNewOutputToStderr(virLogPriority priority) { - if (virLogOutputNew(virLogOutputToFd, NULL, (void *)2L, priority, - VIR_LOG_TO_STDERR, NULL, 0) < 0) - return -1; - return 0; + return virLogOutputNew(virLogOutputToFd, NULL, (void *)2L, priority, + VIR_LOG_TO_STDERR, NULL); } -static int +static 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 -1; - if (virLogOutputNew(virLogOutputToFd, virLogCloseFd, - (void *)(intptr_t)fd, - priority, VIR_LOG_TO_FILE, file, 0) < 0) { - VIR_FORCE_CLOSE(fd); - return -1; + return NULL; + + if (!(ret = virLogOutputNew(virLogOutputToFd, virLogCloseFd, + (void *)(intptr_t)fd, + priority, VIR_LOG_TO_FILE, file))) { + VIR_LOG_CLOSE(fd); + return NULL; } - return 0; + return ret; } @@ -811,25 +797,28 @@ virLogCloseSyslog(void *data ATTRIBUTE_UNUSED) } -static int +static virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, const char *ident) { + virLogOutputPtr ret = NULL; + /* * ident needs to be kept around on Solaris */ VIR_FREE(current_ident); if (VIR_STRDUP(current_ident, ident) < 0) - return -1; + return NULL; openlog(current_ident, 0, 0); - if (virLogOutputNew(virLogOutputToSyslog, virLogCloseSyslog, NULL, - priority, VIR_LOG_TO_SYSLOG, ident, 0) < 0) { + if (!(ret = virLogOutputNew(virLogOutputToSyslog, virLogCloseSyslog, + NULL, priority, VIR_LOG_TO_SYSLOG, + ident))) { closelog(); VIR_FREE(current_ident); - return -1; + return NULL; } - return 0; + return ret; } @@ -1035,19 +1024,25 @@ static void virLogCloseJournald(void *data ATTRIBUTE_UNUSED) } -static int virLogNewOutputToJournald(int priority) +static virLogOutputPtr virLogNewOutputToJournald(int priority) { + virLogOutputPtr ret = NULL; + if ((journalfd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) - return -1; + return NULL; if (virSetInherit(journalfd, false) < 0) { VIR_LOG_CLOSE(journalfd); - return -1; + return NULL; } - if (virLogOutputNew(virLogOutputToJournald, virLogCloseJournald, NULL, - priority, VIR_LOG_TO_JOURNALD, NULL, 0) < 0) { - return -1; + + if (!(ret = virLogOutputNew(virLogOutputToJournald, + virLogCloseJournald, NULL, + priority, VIR_LOG_TO_JOURNALD, NULL))) { + VIR_LOG_CLOSE(journalfd); + return NULL; } - return 0; + + return ret; } # endif /* USE_JOURNALD */ @@ -1081,11 +1076,10 @@ int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) ((*cur == ' ') || (*cur == '\t') || (*cur == '\n') || \ (*cur == '\r') || (*cur == '\\')) - -static int +static virLogOutputPtr virLogParseOutput(const char *src) { - int ret = -1; + virLogOutputPtr ret = NULL; char **tokens = NULL; char *abspath = NULL; size_t count = 0; @@ -1094,7 +1088,7 @@ virLogParseOutput(const char *src) bool isSUID = virIsSUID(); if (!src) - return -1; + return NULL; VIR_DEBUG("output=%s", src); @@ -1102,7 +1096,7 @@ virLogParseOutput(const char *src) * them individually */ if (!(tokens = virStringSplitCount(src, ":", 0, &count))) - return -1; + return NULL; if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) @@ -1146,7 +1140,7 @@ virLogParseOutput(const char *src) } cleanup: - if (ret < 0) + if (!ret) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse and define log output %s"), src); virStringFreeList(tokens); @@ -1180,12 +1174,14 @@ virLogParseOutput(const char *src) * Returns the number of output parsed or -1 in case of error. */ int -virLogParseOutputs(const char *src) +virLogParseOutputs(const char *src, virLogOutputPtr **outputs) { int ret = -1; - int count = 0; + size_t count = 0; size_t i; char **strings = NULL; + virLogOutputPtr output = NULL; + virLogOutputPtr *list = NULL; if (!src) return -1; @@ -1200,14 +1196,22 @@ virLogParseOutputs(const char *src) if (STREQ(strings[i], "")) continue; - if (virLogParseOutput(strings[i]) < 0) + if (!(output = virLogParseOutput(strings[i]))) goto cleanup; - count++; + if (VIR_APPEND_ELEMENT(list, count, output) < 0) { + virLogOutputFree(output); + goto cleanup; + } + + virLogOutputFree(output); } ret = count; + *outputs = list; cleanup: + if (ret < 0) + virLogOutputListFree(list, count); virStringFreeList(strings); return ret; } @@ -1535,12 +1539,22 @@ int virLogSetOutputs(const char *outputs) { int ret = -1; + int count = 0; + virLogOutputPtr *list = NULL; if (virLogInitialize() < 0) return -1; - ret = virLogParseOutputs(outputs); + if ((count = virLogParseOutputs(outputs, &list)) < 0) + goto cleanup; + if (virLogDefineOutputs(list, count) < 0) + goto cleanup; + + ret = count; + cleanup: + if (ret < 0) + virLogOutputListFree(list, count); return ret; } diff --git a/src/util/virlog.h b/src/util/virlog.h index eca7894..9b9f643 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -188,13 +188,12 @@ extern int virLogSetOutputs(const char *outputs); extern int virLogFilterNew(const char *match, virLogPriority priority, unsigned int flags); -extern int virLogOutputNew(virLogOutputFunc f, - virLogCloseFunc c, - void *data, - virLogPriority priority, - virLogDestination dest, - const char *name, - unsigned int flags); +extern virLogOutputPtr virLogOutputNew(virLogOutputFunc f, + virLogCloseFunc c, + void *data, + virLogPriority priority, + virLogDestination dest, + const char *name); extern void virLogOutputFree(virLogOutputPtr output); extern void virLogFilterFree(virLogFilterPtr filter); extern void virLogOutputListFree(virLogOutputPtr *list, int count); @@ -209,7 +208,7 @@ extern void virLogUnlock(void); extern int virLogReset(void); extern int virLogParseDefaultPriority(const char *priority); extern int virLogParseFilters(const char *filters); -extern int virLogParseOutputs(const char *output); +extern int virLogParseOutputs(const char *src, virLogOutputPtr **outputs); extern int virLogPriorityFromSyslog(int priority); extern void virLogMessage(virLogSourcePtr source, virLogPriority priority, diff --git a/tests/testutils.c b/tests/testutils.c index 1b31eca..e679116 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -838,6 +838,9 @@ int virtTestMain(int argc, { int ret; char *testRange = NULL; + size_t count = 0; + virLogOutputPtr output = NULL; + virLogOutputPtr *list = NULL; #ifdef TEST_OOM char *oomstr; #endif @@ -869,9 +872,21 @@ int virtTestMain(int argc, virLogSetFromEnv(); if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) { - if (virLogOutputNew(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))) return EXIT_FAILURE; + + if (VIR_APPEND_ELEMENT(list, count, output) < 0) { + virLogOutputFree(output); + return EXIT_FAILURE; + } + + if (virLogDefineOutputs(list, count) < 0) { + virLogOutputListFree(list, count); + return EXIT_FAILURE; + } + } if ((testRange = getenv("VIR_TEST_RANGE")) != NULL) { diff --git a/tests/virlogtest.c b/tests/virlogtest.c index 6abd4ae..02613f5 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -48,9 +48,10 @@ testLogParseOutputs(const void *opaque) { int ret = -1; int noutputs; + virLogOutputPtr *list = NULL; const struct testLogData *data = opaque; - noutputs = virLogParseOutputs(data->str); + noutputs = virLogParseOutputs(data->str, &list); 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(list, noutputs); return ret; } -- 2.4.11

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 0116f56..9fe47fb 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); return -1; } if (virLogInitialize() < 0) @@ -1432,20 +1434,15 @@ virLogGetNbOutputs(void) int virLogParseDefaultPriority(const char *priority) { - int ret = -1; - if (STREQ(priority, "1") || STREQ(priority, "debug")) - ret = virLogSetDefaultPriority(VIR_LOG_DEBUG); + return VIR_LOG_DEBUG; else if (STREQ(priority, "2") || STREQ(priority, "info")) - ret = virLogSetDefaultPriority(VIR_LOG_INFO); + return VIR_LOG_INFO; else if (STREQ(priority, "3") || STREQ(priority, "warning")) - ret = virLogSetDefaultPriority(VIR_LOG_WARN); + return VIR_LOG_WARN; else if (STREQ(priority, "4") || STREQ(priority, "error")) - ret = virLogSetDefaultPriority(VIR_LOG_ERROR); - else - VIR_WARN("Ignoring invalid log level setting"); - - return ret; + return VIR_LOG_ERROR; + return -1; } @@ -1465,7 +1462,7 @@ virLogSetFromEnv(void) debugEnv = virGetEnvAllowSUID("LIBVIRT_DEBUG"); if (debugEnv && *debugEnv) - virLogParseDefaultPriority(debugEnv); + virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv)); debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS"); if (debugEnv && *debugEnv) virLogSetFilters(debugEnv); diff --git a/tests/eventtest.c b/tests/eventtest.c index c4be606..b6d12a5 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -311,7 +311,8 @@ mymain(void) if (virThreadInitialize() < 0) return EXIT_FAILURE; char *debugEnv = getenv("LIBVIRT_DEBUG"); - if (debugEnv && *debugEnv && (virLogParseDefaultPriority(debugEnv) == -1)) { + if (debugEnv && *debugEnv && + (virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv)) < 0)) { fprintf(stderr, "Invalid log level setting.\n"); return EXIT_FAILURE; } -- 2.4.11

Outputs are a little bit trickier than filters, because if user wants to change the set of outputs that is currently defined for a different set that may contain outputs that do already exist in the current set, opening a file descriptor for that output would only work for files, but not for syslog or journald, because that would overwrite the existing ones which might (and probably will be in use by workers). So, we need a mechanism to find out, if such an output is already defined and only copy its data. For files, the data also include the already opened file descriptor. For journald, the file descriptor is global, so nothing to copy there. Syslog is the most trickiest one, since it keeps the open file descriptor to system daemon private and cannot be reopened (to change the message prefix) until the very last moment when the whole output set is ready to be swapped with the defined one. This method is also used when deallocating a list of outputs (either the new copy that is just being prepared or the global active set of outputs). According to the paragraph above, the problem of changing data when it is being used has been solved. It still needs to be ensured that when deallocating the global active set of outputs after it was swap with the user-provided copy does not close all file descriptors, since that would cause invalid references in the copy that we just swapped. By issuing virLogOutputExist during deallocation, the described scenario can be safely prevented. --- src/libvirt_private.syms | 1 + src/util/virlog.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 38 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5d6224e..13598d0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1795,6 +1795,7 @@ virLogGetNbOutputs; virLogGetOutputs; virLogLock; virLogMessage; +virLogOutputExists; virLogOutputFree; virLogOutputListFree; virLogOutputNew; diff --git a/src/util/virlog.c b/src/util/virlog.c index 9fe47fb..204372f 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -186,6 +186,42 @@ VIR_ONCE_GLOBAL_INIT(virLog) /** + * virLogOutputExists: + * @dest: destination type + * @opaque: opaque data to the method + * + * Looks for an output of destination type @dest. If such output exists, + * a reference to the output is returned, unless the destination is of type + * file in which case a comparison of the output's data with @opaque needs to + * be done first. + * + * Returns a reference to an output if one was found or NULL if such output + * does not exist. + */ +virLogOutputPtr +virLogOutputExists(virLogDestination dest, const void *opaque) +{ + size_t i; + const char *name = opaque; + + for (i = 0; i < virLogNbOutputs; i++) { + if (!virLogOutputs[i]) + continue; + + if (dest == virLogOutputs[i]->dest) { + if (dest != VIR_LOG_TO_FILE) + return virLogOutputs[i]; + + if (STREQ(virLogOutputs[i]->name, name)) + return virLogOutputs[i]; + } + } + + return NULL; +} + + +/** * virLogReset: * * Reset the logging module to its default initial state diff --git a/src/util/virlog.h b/src/util/virlog.h index 0102489..1c55a48 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -229,5 +229,6 @@ extern void virLogVMessage(virLogSourcePtr source, bool virLogProbablyLogMessage(const char *str); int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs); int virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters); +virLogOutputPtr virLogOutputExists(virLogDestination dest, const void *opaque); #endif -- 2.4.11

Now that we're in the critical section, closelog and openlog need to be issued if the user setting changed, which is something that cannot be done beforehand, since syslog keeps its file descriptor private. --- src/util/virlog.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index 62533b1..a20cde4 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1605,6 +1605,8 @@ virLogSetOutputs(const char *outputs) int ret = -1; int count = 0; virLogOutputPtr *list = NULL; + size_t i; + char *tmp = NULL; if (virLogInitialize() < 0) return -1; @@ -1612,6 +1614,17 @@ virLogSetOutputs(const char *outputs) if ((count = virLogParseOutputs(outputs, &list)) < 0) goto cleanup; + /* syslog needs a special care */ + for (i = 0; i < count; i++) { + if (list[i]->dest == VIR_LOG_TO_SYSLOG) { + if (VIR_STRDUP(tmp, list[i]->name) < 0) + goto cleanup; + VIR_FREE(current_ident); + current_ident = tmp; + openlog(current_ident, 0, 0); + } + } + if (virLogDefineOutputs(list, count) < 0) goto cleanup; -- 2.4.11

Global priority can be changed from outside as well. So far, updating priority is filter driven, meaning that source priority can only be changed when filters change. To assure, users can change the global priority from outside, make the serial more generic, so that any change to priority would trigger source update. --- src/util/virlog.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 8e14a9e..a837e17 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -87,7 +87,7 @@ struct _virLogFilter { unsigned int flags; }; -static int virLogFiltersSerial = 1; +static int virLogSerial = 1; static virLogFilterPtr *virLogFilters; static size_t virLogNbFilters; @@ -280,7 +280,7 @@ virLogResetFilters(void) virLogFilterListFree(virLogFilters, virLogNbFilters); virLogFilters = NULL; virLogNbFilters = 0; - virLogFiltersSerial++; + virLogSerial++; } @@ -483,7 +483,7 @@ static void virLogSourceUpdate(virLogSourcePtr source) { virLogLock(); - if (source->serial < virLogFiltersSerial) { + if (source->serial < virLogSerial) { unsigned int priority = virLogDefaultPriority; unsigned int flags = 0; size_t i; @@ -498,7 +498,7 @@ virLogSourceUpdate(virLogSourcePtr source) source->priority = priority; source->flags = flags; - source->serial = virLogFiltersSerial; + source->serial = virLogSerial; } virLogUnlock(); } @@ -582,7 +582,7 @@ virLogVMessage(virLogSourcePtr source, * thread is updating log filter list concurrently * with a log message emission. */ - if (source->serial < virLogFiltersSerial) + if (source->serial < virLogSerial) virLogSourceUpdate(source); if (priority < source->priority) goto cleanup; @@ -1686,7 +1686,7 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters) virLogResetFilters(); virLogFilters = filters; virLogNbFilters = nfilters; - virLogFiltersSerial++; + virLogSerial++; virLogUnlock(); return virLogNbFilters; -- 2.4.11

If the API isn't closed, a situation with 2 setters where one is about to define a set of outputs and the other is already defining a new one, may occur. By resetting all outputs, all file descriptors are closed. However, the other setter may still have a dangling reference to a file descriptor which may have already been closed. --- src/libvirt_private.syms | 2 ++ src/util/virlog.c | 15 +++++++++++++++ src/util/virlog.h | 2 ++ 3 files changed, 19 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13598d0..5cab7e1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1784,6 +1784,8 @@ virLockSpaceReleaseResourcesForOwner; # util/virlog.h +virLogAPILock; +virLogAPIUnlock; virLogDefineFilters; virLogDefineOutputs; virLogFilterListFree; diff --git a/src/util/virlog.c b/src/util/virlog.c index 769dcec..6aa9c91 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -128,6 +128,21 @@ static void virLogOutputToFd(virLogSourcePtr src, void *data); +/* Setters need to be serialized on API entry point */ +static virMutex virLogAPIMutex; + +void +virLogAPILock(void) +{ + virMutexLock(&virLogAPIMutex); +} + +void +virLogAPIUnlock(void) +{ + virMutexUnlock(&virLogAPIMutex); +} + /* * Logs accesses must be serialized though a mutex */ diff --git a/src/util/virlog.h b/src/util/virlog.h index 1c55a48..f5c0a4f 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -203,6 +203,8 @@ extern void virLogFilterListFree(virLogFilterPtr *list, int count); * Internal logging API */ +extern void virLogAPILock(void); +extern void virLogAPIUnlock(void); extern void virLogLock(void); extern void virLogUnlock(void); extern int virLogReset(void); -- 2.4.11

Enable libvirt users to query for the current logging level setting. --- daemon/admin.c | 9 +++++++++ include/libvirt/libvirt-admin.h | 2 ++ src/admin/admin_protocol.x | 16 +++++++++++++++- src/admin_protocol-structs | 8 ++++++++ src/libvirt-admin.c | 34 ++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 7 files changed, 71 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index 3de09ca..5129e2d 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -295,4 +295,13 @@ adminDispatchClientGetInfo(virNetServerPtr server ATTRIBUTE_UNUSED, virObjectUnref(srv); return rv; } + +static int +adminConnectGetLoggingLevel(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogGetDefaultPriority(); +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 0a1ea61..2878c78 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -349,6 +349,8 @@ int virAdmClientGetInfo(virAdmClientPtr client, int *nparams, unsigned int flags); +int virAdmConnectGetLoggingLevel(virAdmConnectPtr conn, unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 67bdbf3..6a95665 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -158,6 +158,15 @@ struct admin_client_get_info_args { struct admin_client_get_info_ret { /* insert@1 */ admin_typed_param params<ADMIN_CLIENT_INFO_PARAMETERS_MAX>; + unsigned int flags; +}; + +struct admin_connect_get_logging_level_args { + unsigned int flags; +}; + +struct admin_connect_get_logging_level_ret { + int level; }; /* Define the program number, protocol version and procedure numbers here. */ @@ -230,5 +239,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CLIENT_GET_INFO = 10 + ADMIN_PROC_CLIENT_GET_INFO = 10, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_GET_LOGGING_LEVEL = 11 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index ea9adf6..72d93ec 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -104,6 +104,13 @@ struct admin_client_get_info_ret { u_int params_len; admin_typed_param * params_val; } params; + u_int flags; +}; +struct admin_connect_get_logging_level_args { + u_int flags; +}; +struct admin_connect_get_logging_level_ret { + int level; }; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, @@ -116,4 +123,5 @@ enum admin_procedure { ADMIN_PROC_SERVER_LIST_CLIENTS = 8, ADMIN_PROC_SERVER_LOOKUP_CLIENT = 9, ADMIN_PROC_CLIENT_GET_INFO = 10, + ADMIN_PROC_CONNECT_GET_LOGGING_LEVEL = 11, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index a94f5dd..898b9c1 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -975,3 +975,37 @@ virAdmClientGetInfo(virAdmClientPtr client, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectGetLoggingLevel: + * @conn: pointer to an active admin connection + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Retrieves the current global logging level (as per daemon configuration): + * 1: DEBUG + * 2: INFO + * 3: WARNING + * 4: ERROR + * + * Returns the numeric logging level representation or -1 in case of an error. + */ +int +virAdmConnectGetLoggingLevel(virAdmConnectPtr conn, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckFlagsGoto(0, error); + + if ((ret = remoteAdminConnectGetLoggingLevel(conn, flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index affe8c1..8534ab1 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -9,6 +9,8 @@ xdr_admin_client_get_info_args; xdr_admin_client_get_info_ret; xdr_admin_connect_get_lib_version_ret; +xdr_admin_connect_get_logging_level_args; +xdr_admin_connect_get_logging_level_ret; xdr_admin_connect_list_servers_args; xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 27e4a1d..4cc0a5b 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -26,6 +26,7 @@ LIBVIRT_ADMIN_1.3.0 { virAdmConnectRegisterCloseCallback; virAdmConnectUnregisterCloseCallback; virAdmConnectListServers; + virAdmConnectGetLoggingLevel; virAdmServerGetName; virAdmServerGetThreadPoolParameters; virAdmServerFree; -- 2.4.11

Enable libvirt users to query logging output settings. --- daemon/admin.c | 42 ++++++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 ++++++++++++++- src/admin/admin_remote.c | 43 +++++++++++++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 ++++++++ src/libvirt-admin.c | 39 +++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 8 files changed, 154 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index ac81b82..5f7a957 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -323,6 +323,23 @@ adminConnectGetLoggingFilters(char **filters, unsigned int flags) } static int +adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) +{ + char *tmp = NULL; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(tmp = virLogGetOutputs())) + return -1; + + ret = virLogGetNbOutputs(); + + *outputs = tmp; + return ret; +} + +static int adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, @@ -349,4 +366,29 @@ adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, VIR_FREE(filters); return rv; } + +static int +adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + admin_connect_get_logging_outputs_args *args, + admin_connect_get_logging_outputs_ret *ret) +{ + char *outputs = NULL; + int noutputs = 0; + int rv = -1; + + if ((noutputs = adminConnectGetLoggingOutputs(&outputs, args->flags) < 0)) + goto cleanup; + + ret->outputs = outputs; + ret->noutputs = noutputs; + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + return rv; +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 87616d0..755e6a6 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -355,6 +355,10 @@ int virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, char **filters, unsigned int flags); +int virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 05eecec..5465efe 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -178,6 +178,15 @@ struct admin_connect_get_logging_filters_ret { unsigned int nfilters; }; +struct admin_connect_get_logging_outputs_args { + unsigned int flags; +}; + +struct admin_connect_get_logging_outputs_ret { + admin_nonnull_string outputs; + unsigned int noutputs; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -258,5 +267,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 12 + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 12, + + /** + * @generate: none + */ + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 13 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 86deaec..0d00797 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -398,3 +398,46 @@ remoteAdminConnectGetLoggingFilters(virAdmConnectPtr conn, virObjectUnlock(priv); return rv; } + +static int +remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags) +{ + int rv = -1; + char *tmp_outputs = NULL; + remoteAdminPrivPtr priv = conn->privateData; + admin_connect_get_logging_outputs_args args; + admin_connect_get_logging_outputs_ret ret; + + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + virObjectLock(priv); + + if (call(conn, + 0, + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS, + (xdrproc_t) xdr_admin_connect_get_logging_outputs_args, + (char *) &args, + (xdrproc_t) xdr_admin_connect_get_logging_outputs_ret, + (char *) &ret) == -1) + goto done; + + if (outputs) { + if (VIR_STRDUP(tmp_outputs, ret.outputs) < 0) + goto cleanup; + + *outputs = tmp_outputs; + tmp_outputs = NULL; + } + + rv = ret.noutputs; + + cleanup: + xdr_free((xdrproc_t) xdr_admin_connect_get_logging_outputs_ret, (char *) &ret); + + done: + virObjectUnlock(priv); + return rv; +} diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index bbe4a5a..7cf2421 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -119,6 +119,13 @@ struct admin_connect_get_logging_filters_ret { admin_nonnull_string filters; u_int nfilters; }; +struct admin_connect_get_logging_outputs_args { + u_int flags; +}; +struct admin_connect_get_logging_outputs_ret { + admin_nonnull_string outputs; + u_int noutputs; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -132,4 +139,5 @@ enum admin_procedure { ADMIN_PROC_CLIENT_GET_INFO = 10, ADMIN_PROC_CONNECT_GET_LOGGING_LEVEL = 11, ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 12, + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 13, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index e61dc10..95b47c0 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1049,3 +1049,42 @@ virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectGetLoggingOutputs: + * @conn: pointer to an active admin connection + * @outputs: pointer to a variable to store a string containing all currently + * defined logging outputs on daemon (allocated automatically) + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Retrieves a list of currently installed logging outputs. Outputs returned + * are contained within an automatically allocated string and delimited by + * spaces. The format of each output conforms to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + * To retrieve individual outputs, additional parsing needs to be done by the + * caller. Caller is also responsible for freeing @outputs correctly. + * + * Returns the count of outputs in @outputs, or -1 in case of an error. + */ +int +virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(outputs, error); + + if ((ret = remoteAdminConnectGetLoggingOutputs(conn, outputs, + flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index 6029575..ac183c6 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -13,6 +13,8 @@ xdr_admin_connect_get_logging_filters_args; xdr_admin_connect_get_logging_filters_ret; xdr_admin_connect_get_logging_level_args; xdr_admin_connect_get_logging_level_ret; +xdr_admin_connect_get_logging_outputs_args; +xdr_admin_connect_get_logging_outputs_ret; xdr_admin_connect_list_servers_args; xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index d7648a3..68475b1 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -28,6 +28,7 @@ LIBVIRT_ADMIN_1.3.0 { virAdmConnectListServers; virAdmConnectGetLoggingLevel; virAdmConnectGetLoggingFilters; + virAdmConnectGetLoggingOutputs; virAdmServerGetName; virAdmServerGetThreadPoolParameters; virAdmServerFree; -- 2.4.11

Enable libvirt users to modify logging filters of a daemon from outside. --- daemon/admin.c | 10 ++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 12 +++++++++++- src/admin_protocol-structs | 5 +++++ src/libvirt-admin.c | 36 ++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 68 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index a2d35df..2a0a9ab 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -350,6 +350,16 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) } static int +adminConnectSetLoggingFilters(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *filters, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogSetFilters(filters); +} + +static int adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 41a9e2b..6be649a 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -363,6 +363,10 @@ int virAdmConnectSetLoggingLevel(virAdmConnectPtr conn, unsigned int level, unsigned int flags); +int virAdmConnectSetLoggingFilters(virAdmConnectPtr conn, + const char *filters, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index d20c9d5..80de75a 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -192,6 +192,11 @@ struct admin_connect_set_logging_level_args { unsigned int flags; }; +struct admin_connect_set_logging_filters_args { + admin_nonnull_string filters; + unsigned int flags; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -282,5 +287,10 @@ enum admin_procedure { /** * @generate: both */ - ADMIN_PROC_CONNECT_SET_LOGGING_LEVEL = 14 + ADMIN_PROC_CONNECT_SET_LOGGING_LEVEL = 14, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 15 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 6620fb3..000cef9 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -130,6 +130,10 @@ struct admin_connect_set_logging_level_args { u_int level; u_int flags; }; +struct admin_connect_set_logging_filters_args { + admin_nonnull_string filters; + u_int flags; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -145,4 +149,5 @@ enum admin_procedure { ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 12, ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 13, ADMIN_PROC_CONNECT_SET_LOGGING_LEVEL = 14, + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 15, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 7ac72d1..fedd4a6 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1123,3 +1123,39 @@ virAdmConnectSetLoggingLevel(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectSetLoggingFilters: + * @conn: pointer to an active admin connection + * @filters: pointer to a string containing a list of filters to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Redefine the existing (set of) filter(s) with a new one specified in + * @filters. If multiple filters are specified, they need to be delimited by + * spaces. The format of each filter must conform to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + * + * Returns 0 if the new filter or the set of filters has been defined + * successfully, or -1 in case of an error. + */ +int +virAdmConnectSetLoggingFilters(virAdmConnectPtr conn, + const char *filters, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + virCheckNonNullArgGoto(filters, error); + + if ((ret = remoteAdminConnectSetLoggingFilters(conn, filters, flags)) < 0) + goto error; + + return ret; + error: + virDispatchError(NULL); + return -1; +} diff --git a/src/libvirt_admin_private.syms b/src/libvirt_admin_private.syms index 28ff81c..2d72a86 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -20,6 +20,7 @@ xdr_admin_connect_list_servers_ret; xdr_admin_connect_lookup_server_args; xdr_admin_connect_lookup_server_ret; xdr_admin_connect_open_args; +xdr_admin_connect_set_logging_filters_args; xdr_admin_connect_set_logging_level_args; xdr_admin_server_get_threadpool_parameters_args; xdr_admin_server_get_threadpool_parameters_ret; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index a8b51c3..ffc8606 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -38,4 +38,5 @@ LIBVIRT_ADMIN_1.3.0 { virAdmServerListClients; virAdmClientGetInfo; virAdmConnectSetLoggingLevel; + virAdmConnectSetLoggingFilters; }; -- 2.4.11

We should encourage libvirt users to use our own constants when modifying logging level, instead of their own values. --- include/libvirt/libvirt-admin.h | 15 +++++++++++++++ src/util/virlog.h | 10 ---------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 6fe5732..3b6eb95 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -35,6 +35,21 @@ extern "C" { # undef __VIR_ADMIN_H_INCLUDES__ /** + * virLogPriority: + * + * These levels are recognized by daemon's logger and can be statically + * configured in daemon's configuration file (e.g. libvirtd.conf). For runtime + * level retrieval and modification use virAdmConnectGetLoggingLevel and + * virAdmConnectSetLoggingLevel APIs. + */ +typedef enum { + VIR_LOG_DEBUG = 1, + VIR_LOG_INFO, + VIR_LOG_WARN, + VIR_LOG_ERROR, +} virLogPriority; + +/** * virAdmConnect: * * a virAdmConnect is a private structure representing a connection to diff --git a/src/util/virlog.h b/src/util/virlog.h index f5c0a4f..b631327 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -38,16 +38,6 @@ "libvirt version: " VERSION # endif -/* - * To be made public - */ -typedef enum { - VIR_LOG_DEBUG = 1, - VIR_LOG_INFO, - VIR_LOG_WARN, - VIR_LOG_ERROR, -} virLogPriority; - # define VIR_LOG_DEFAULT VIR_LOG_WARN typedef enum { -- 2.4.11