[libvirt] [PATCH v2 00/10] admin: Introduce runtime logging APIs

v1 here https://www.redhat.com/archives/libvir-list/2016-November/msg00009.html since v1: - incorporated notes raised during review - allowed passing of NULL via the public APIs * behaves the same way as an empty string, but lead to even more code reduction in 'daemonSetupLogging' for practically no extra code to enable such feature - added forgotten documentation - updated NEWS file Erik Skultety (10): virlog: Introduce virLog{Get,Set}DefaultOutput admin: Allow passing NULL to virLogSetFilters daemon: Hook up the virLog{Get,Set}DefaultOutput to the daemon's init routine admin: Introduce virAdmConnectGetLoggingOutputs admin: Introduce virAdmConnectGetLoggingFilters admin: Introduce virAdmConnectSetLoggingOutputs admin: Introduce virAdmConnectSetLoggingFilters virt-admin: Wire-up the logging APIs admin: Add an example demonstrating how to use the logging APIs admin: Update the news file to include the new logging features .gitignore | 1 + daemon/admin.c | 104 ++++++++++++++++++++++++++ daemon/libvirtd.c | 96 ++++-------------------- docs/news.html.in | 4 + examples/Makefile.am | 3 +- examples/admin/logging.c | 102 ++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 16 ++++ src/admin/admin_protocol.x | 50 ++++++++++++- src/admin/admin_remote.c | 70 ++++++++++++++++++ src/admin_protocol-structs | 26 +++++++ src/libvirt-admin.c | 157 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 6 ++ src/libvirt_admin_public.syms | 8 ++ src/libvirt_private.syms | 2 + src/locking/lock_daemon.c | 90 ++++------------------- src/logging/log_daemon.c | 92 ++++------------------- src/util/virlog.c | 100 ++++++++++++++++++++++++- src/util/virlog.h | 4 +- tools/virt-admin.c | 120 ++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 +++++++++++++++++++++++ 20 files changed, 899 insertions(+), 242 deletions(-) create mode 100644 examples/admin/logging.c -- 2.5.5

These helpers will manage the log destination defaults (fetch/set). The reason for this is to stay consistent with the current daemon's behaviour with respect to /etc/libvirt/<daemon>.conf file, since both assignment of an empty string or not setting the log output variable at all trigger the daemon's decision on the default log destination which depends on whether the daemon runs daemonized or not. This patch also changes the logic of the selection of the default logging output compared to how it is done now. The main difference though is that we should only really care if we're running daemonized or not, disregarding the fact of (not) having a TTY completely (introduced by commit eba36a3878) as that should be of the libvirtd's parent concern (what FD it will pass to it). Before: if (godaemon || !hasTTY): if (journald): use journald if (godaemon): if (privileged): use SYSCONFIG/libvirtd.log else: use XDG_CONFIG_HOME/libvirtd.log else: use stderr After: if (godaemon): if (journald): use journald else: if (privileged): use SYSCONFIG/libvirtd.log else: use XDG_CONFIG_HOME/libvirtd.log else: use stderr Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virlog.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 2 ++ 3 files changed, 96 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a04ba23..803458f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1883,6 +1883,7 @@ virLogFilterFree; virLogFilterListFree; virLogFilterNew; virLogFindOutput; +virLogGetDefaultOutput; virLogGetDefaultPriority; virLogGetFilters; virLogGetNbFilters; @@ -1901,6 +1902,7 @@ virLogParseOutputs; virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; +virLogSetDefaultOutput; virLogSetDefaultPriority; virLogSetFilters; virLogSetFromEnv; diff --git a/src/util/virlog.c b/src/util/virlog.c index 8f831fc..9b52e66 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -50,6 +50,7 @@ #include "virtime.h" #include "intprops.h" #include "virstring.h" +#include "configmake.h" /* Journald output is only supported on Linux new enough to expose * htole64. */ @@ -105,6 +106,7 @@ struct _virLogOutput { char *name; }; +static char *virLogDefaultOutput; static virLogOutputPtr *virLogOutputs; static size_t virLogNbOutputs; @@ -147,6 +149,96 @@ virLogUnlock(void) } +static int +virLogSetDefaultOutputToStderr(void) +{ + return virAsprintf(&virLogDefaultOutput, "%d:stderr", + virLogDefaultPriority); +} + + +static int +virLogSetDefaultOutputToJournald(void) +{ + virLogPriority priority = virLogDefaultPriority; + + /* By default we don't want to log too much stuff into journald as + * it may employ rate limiting and thus block libvirt execution. */ + if (priority == VIR_LOG_DEBUG) + priority = VIR_LOG_INFO; + + return virAsprintf(&virLogDefaultOutput, "%d:journald", priority); +} + + +static int +virLogSetDefaultOutputToFile(const char *filename, bool privileged) +{ + int ret = -1; + char *logdir = NULL; + mode_t old_umask; + + if (privileged) { + if (virAsprintf(&virLogDefaultOutput, + "%d:file:%s/log/libvirt/%s", virLogDefaultPriority, + LOCALSTATEDIR, filename) < 0) + goto cleanup; + } else { + if (!(logdir = virGetUserCacheDirectory())) + goto cleanup; + + old_umask = umask(077); + if (virFileMakePath(logdir) < 0) { + umask(old_umask); + goto cleanup; + } + umask(old_umask); + + if (virAsprintf(&virLogDefaultOutput, "%d:file:%s/%s", + virLogDefaultPriority, logdir, filename) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(logdir); + return ret; +} + + +/* + * virLogSetDefaultOutput: + * @filename: the file that the output should be redirected to (only needed + * when @godaemon equals true + * @godaemon: whether we're running daemonized + * @privileged: whether we're running with root privileges or not (session) + * + * Decides on what the default output (journald, file, stderr) should be + * according to @filename, @godaemon, @privileged. This function should be run + * exactly once at daemon startup, so no locks are used. + * + * Returns 0 on success, -1 in case of a failure. + */ +int +virLogSetDefaultOutput(const char *filename, bool godaemon, bool privileged) +{ + if (!godaemon) + return virLogSetDefaultOutputToStderr(); + + if (access("/run/systemd/journal/socket", W_OK) >= 0) + return virLogSetDefaultOutputToJournald(); + + return virLogSetDefaultOutputToFile(filename, privileged); +} + + +char * +virLogGetDefaultOutput(void) +{ + return virLogDefaultOutput; +} + + static const char * virLogPriorityString(virLogPriority lvl) { diff --git a/src/util/virlog.h b/src/util/virlog.h index 3f2d422..b4ffeca 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -189,6 +189,8 @@ void virLogFilterFree(virLogFilterPtr filter); void virLogFilterListFree(virLogFilterPtr *list, int count); int virLogSetOutputs(const char *outputs) ATTRIBUTE_NONNULL(1); int virLogSetFilters(const char *filters); +char *virLogGetDefaultOutput(void); +int virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged); /* * Internal logging API -- 2.5.5

On 11/25/2016 08:11 AM, Erik Skultety wrote:
These helpers will manage the log destination defaults (fetch/set). The reason for this is to stay consistent with the current daemon's behaviour with respect to /etc/libvirt/<daemon>.conf file, since both assignment of an empty string or not setting the log output variable at all trigger the daemon's decision on the default log destination which depends on whether the daemon runs daemonized or not. This patch also changes the logic of the selection of the default logging output compared to how it is done now. The main difference though is that we should only really care if we're running daemonized or not, disregarding the fact of (not) having a TTY completely (introduced by commit eba36a3878) as that should be of the libvirtd's parent concern (what FD it will pass to it).
Before: if (godaemon || !hasTTY): if (journald): use journald
if (godaemon): if (privileged): use SYSCONFIG/libvirtd.log else: use XDG_CONFIG_HOME/libvirtd.log else: use stderr
After: if (godaemon): if (journald): use journald
else: if (privileged): use SYSCONFIG/libvirtd.log else: use XDG_CONFIG_HOME/libvirtd.log else: use stderr
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virlog.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 2 ++ 3 files changed, 96 insertions(+)
ACK John

Along with an empty string, it should also be possible for users to pass NULL to the public APIs which in turn would trigger a routine (future work) responsible for defining an appropriate default logging output given the current circumstances. 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 | 8 +++++++- src/util/virlog.h | 2 +- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index cd25b50..3902a8b 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -693,7 +693,7 @@ daemonSetupLogging(struct daemonConfig *config, if (virLogGetNbFilters() == 0) virLogSetFilters(config->log_filters); - if (config->log_outputs && virLogGetNbOutputs() == 0) + if (virLogGetNbOutputs() == 0) virLogSetOutputs(config->log_outputs); /* diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 02745be..9ee818e 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -478,7 +478,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, if (virLogGetNbFilters() == 0) virLogSetFilters(config->log_filters); - if (config->log_outputs && virLogGetNbOutputs() == 0) + if (virLogGetNbOutputs() == 0) virLogSetOutputs(config->log_outputs); /* diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 04bb836..a9aebdb 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -406,7 +406,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, if (virLogGetNbFilters() == 0) virLogSetFilters(config->log_filters); - if (config->log_outputs && virLogGetNbOutputs() == 0) + if (virLogGetNbOutputs() == 0) virLogSetOutputs(config->log_outputs); /* diff --git a/src/util/virlog.c b/src/util/virlog.c index 9b52e66..acd2285 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1818,6 +1818,8 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) * @outputs: string defining a (set of) output(s) * * Replaces the current set of defined outputs with a new set of outputs. + * Should the set be empty or NULL, a default output is used according to the + * daemon's runtime attributes. * * Returns 0 on success or -1 in case of an error. */ @@ -1826,12 +1828,16 @@ virLogSetOutputs(const char *src) { int ret = -1; int noutputs = 0; + const char *outputstr = virLogDefaultOutput; virLogOutputPtr *outputs = NULL; if (virLogInitialize() < 0) return -1; - if ((noutputs = virLogParseOutputs(src, &outputs)) < 0) + if (src && *src) + outputstr = src; + + if ((noutputs = virLogParseOutputs(outputstr, &outputs)) < 0) goto cleanup; if (virLogDefineOutputs(outputs, noutputs) < 0) diff --git a/src/util/virlog.h b/src/util/virlog.h index b4ffeca..cc09f48 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -187,7 +187,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) ATTRIBUTE_NONNULL(1); +int virLogSetOutputs(const char *outputs); int virLogSetFilters(const char *filters); char *virLogGetDefaultOutput(void); int virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged); -- 2.5.5

$SUB: "virLogSetOutputs"? On 11/25/2016 08:12 AM, Erik Skultety wrote:
Along with an empty string, it should also be possible for users to pass NULL to the public APIs which in turn would trigger a routine (future work) responsible for defining an appropriate default logging output given the current circumstances.
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 | 8 +++++++- src/util/virlog.h | 2 +- 5 files changed, 11 insertions(+), 5 deletions(-)
ACK w/ commit msg adjustment John

Now that virLog{Get,Set}DefaultOutput routines are introduced we can wire them up to the daemon's logging initialization code. Also, change the order of operations a bit so that we still strictly honor our precedence of settings: cmdline > env > config now that outputs and filters are not appended anymore. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/libvirtd.c | 96 +++++++---------------------------------------- src/locking/lock_daemon.c | 90 +++++++------------------------------------- src/logging/log_daemon.c | 92 +++++++-------------------------------------- 3 files changed, 40 insertions(+), 238 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 3902a8b..b2e89fd 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -675,103 +675,33 @@ daemonSetupLogging(struct daemonConfig *config, * Libvirtd's order of precedence is: * cmdline > environment > config * - * In order to achieve this, we must process configuration in - * different order for the log level versus the filters and - * outputs. Because filters and outputs append, we have to look at - * the environment first and then only check the config file if - * there was no result from the environment. The default output is - * then applied only if there was no setting from either of the - * first two. Because we don't have a way to determine if the log - * level has been set, we must process variables in the opposite + * The default output is applied only if there was no setting from either + * the config or the environment. Because we don't have a way to determine + * if the log level has been set, we must process variables in the opposite * order, each one overriding the previous. */ if (config->log_level != 0) virLogSetDefaultPriority(config->log_level); + if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0) + return -1; + + /* In case the config is empty, the filters become empty and outputs will + * be set to default + */ + virLogSetFilters(config->log_filters); + virLogSetOutputs(config->log_outputs); + + /* If there are some environment variables defined, use those instead */ virLogSetFromEnv(); - if (virLogGetNbFilters() == 0) - virLogSetFilters(config->log_filters); - - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(config->log_outputs); - /* * Command line override for --verbose */ if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) virLogSetDefaultPriority(VIR_LOG_INFO); - /* - * If no defined outputs, and either running - * as daemon or not on a tty, then first try - * to direct it to the systemd journal - * (if it exists).... - */ - if (virLogGetNbOutputs() == 0 && - (godaemon || !isatty(STDIN_FILENO))) { - char *tmp; - if (access("/run/systemd/journal/socket", W_OK) >= 0) { - virLogPriority priority = virLogGetDefaultPriority(); - - /* By default we don't want to log too much stuff into journald as - * it may employ rate limiting and thus block libvirt execution. */ - if (priority == VIR_LOG_DEBUG) - priority = VIR_LOG_INFO; - - if (virAsprintf(&tmp, "%d:journald", priority) < 0) - goto error; - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - } - - /* - * otherwise direct to libvirtd.log when running - * as daemon. Otherwise the default output is stderr. - */ - if (virLogGetNbOutputs() == 0) { - char *tmp = NULL; - - if (godaemon) { - if (privileged) { - if (virAsprintf(&tmp, "%d:file:%s/log/libvirt/libvirtd.log", - virLogGetDefaultPriority(), - LOCALSTATEDIR) == -1) - goto error; - } else { - char *logdir = virGetUserCacheDirectory(); - mode_t old_umask; - - if (!logdir) - goto error; - - old_umask = umask(077); - if (virFileMakePath(logdir) < 0) { - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(&tmp, "%d:file:%s/libvirtd.log", - virLogGetDefaultPriority(), logdir) == -1) { - VIR_FREE(logdir); - goto error; - } - VIR_FREE(logdir); - } - } else { - if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) - goto error; - } - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - return 0; - - error: - return -1; } diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 9ee818e..881af11 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -460,97 +460,33 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config, * Libvirtd's order of precedence is: * cmdline > environment > config * - * In order to achieve this, we must process configuration in - * different order for the log level versus the filters and - * outputs. Because filters and outputs append, we have to look at - * the environment first and then only check the config file if - * there was no result from the environment. The default output is - * then applied only if there was no setting from either of the - * first two. Because we don't have a way to determine if the log - * level has been set, we must process variables in the opposite + * The default output is applied only if there was no setting from either + * the config or the environment. Because we don't have a way to determine + * if the log level has been set, we must process variables in the opposite * order, each one overriding the previous. */ if (config->log_level != 0) virLogSetDefaultPriority(config->log_level); + if (virLogSetDefaultOutput("virtlockd.log", godaemon, privileged) < 0) + return -1; + + /* In case the config is empty, the filters become empty and outputs will + * be set to default + */ + virLogSetFilters(config->log_filters); + virLogSetOutputs(config->log_outputs); + + /* If there are some environment variables defined, use those instead */ virLogSetFromEnv(); - if (virLogGetNbFilters() == 0) - virLogSetFilters(config->log_filters); - - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(config->log_outputs); - /* * Command line override for --verbose */ if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) virLogSetDefaultPriority(VIR_LOG_INFO); - /* - * If no defined outputs, and either running - * as daemon or not on a tty, then first try - * to direct it to the systemd journal - * (if it exists).... - */ - if (virLogGetNbOutputs() == 0 && - (godaemon || !isatty(STDIN_FILENO))) { - char *tmp; - if (access("/run/systemd/journal/socket", W_OK) >= 0) { - if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) - goto error; - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - } - - /* - * otherwise direct to libvirtd.log when running - * as daemon. Otherwise the default output is stderr. - */ - if (virLogGetNbOutputs() == 0) { - char *tmp = NULL; - - if (godaemon) { - if (privileged) { - if (virAsprintf(&tmp, "%d:file:%s/log/libvirt/virtlockd.log", - virLogGetDefaultPriority(), - LOCALSTATEDIR) == -1) - goto error; - } else { - char *logdir = virGetUserCacheDirectory(); - mode_t old_umask; - - if (!logdir) - goto error; - - old_umask = umask(077); - if (virFileMakePath(logdir) < 0) { - VIR_FREE(logdir); - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(&tmp, "%d:file:%s/virtlockd.log", - virLogGetDefaultPriority(), logdir) == -1) { - VIR_FREE(logdir); - goto error; - } - VIR_FREE(logdir); - } - } else { - if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) - goto error; - } - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - return 0; - - error: - return -1; } diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index a9aebdb..793811e 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -385,100 +385,36 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config, virLogReset(); /* - * virtlogd's order of precedence is: + * Libvirtd's order of precedence is: * cmdline > environment > config * - * In order to achieve this, we must process configuration in - * different order for the log level versus the filters and - * outputs. Because filters and outputs append, we have to look at - * the environment first and then only check the config file if - * there was no result from the environment. The default output is - * then applied only if there was no setting from either of the - * first two. Because we don't have a way to determine if the log - * level has been set, we must process variables in the opposite + * The default output is applied only if there was no setting from either + * the config or the environment. Because we don't have a way to determine + * if the log level has been set, we must process variables in the opposite * order, each one overriding the previous. */ if (config->log_level != 0) virLogSetDefaultPriority(config->log_level); + if (virLogSetDefaultOutput("virtlogd.log", godaemon, privileged) < 0) + return -1; + + /* In case the config is empty, the filters become empty and outputs will + * be set to default + */ + virLogSetFilters(config->log_filters); + virLogSetOutputs(config->log_outputs); + + /* If there are some environment variables defined, use those instead */ virLogSetFromEnv(); - if (virLogGetNbFilters() == 0) - virLogSetFilters(config->log_filters); - - if (virLogGetNbOutputs() == 0) - virLogSetOutputs(config->log_outputs); - /* * Command line override for --verbose */ if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO)) virLogSetDefaultPriority(VIR_LOG_INFO); - /* - * If no defined outputs, and either running - * as daemon or not on a tty, then first try - * to direct it to the systemd journal - * (if it exists).... - */ - if (virLogGetNbOutputs() == 0 && - (godaemon || !isatty(STDIN_FILENO))) { - char *tmp; - if (access("/run/systemd/journal/socket", W_OK) >= 0) { - if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0) - goto error; - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - } - - /* - * otherwise direct to libvirtd.log when running - * as daemon. Otherwise the default output is stderr. - */ - if (virLogGetNbOutputs() == 0) { - char *tmp = NULL; - - if (godaemon) { - if (privileged) { - if (virAsprintf(&tmp, "%d:file:%s/log/libvirt/virtlogd.log", - virLogGetDefaultPriority(), - LOCALSTATEDIR) == -1) - goto error; - } else { - char *logdir = virGetUserCacheDirectory(); - mode_t old_umask; - - if (!logdir) - goto error; - - old_umask = umask(077); - if (virFileMakePath(logdir) < 0) { - umask(old_umask); - VIR_FREE(logdir); - goto error; - } - umask(old_umask); - - if (virAsprintf(&tmp, "%d:file:%s/virtlogd.log", - virLogGetDefaultPriority(), logdir) == -1) { - VIR_FREE(logdir); - goto error; - } - VIR_FREE(logdir); - } - } else { - if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0) - goto error; - } - virLogSetOutputs(tmp); - VIR_FREE(tmp); - } - return 0; - - error: - return -1; } -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Now that virLog{Get,Set}DefaultOutput routines are introduced we can wire them up to the daemon's logging initialization code. Also, change the order of operations a bit so that we still strictly honor our precedence of settings: cmdline > env > config now that outputs and filters are not appended anymore.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/libvirtd.c | 96 +++++++---------------------------------------- src/locking/lock_daemon.c | 90 +++++++------------------------------------- src/logging/log_daemon.c | 92 +++++++-------------------------------------- 3 files changed, 40 insertions(+), 238 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 3902a8b..b2e89fd 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -675,103 +675,33 @@ daemonSetupLogging(struct daemonConfig *config, * Libvirtd's order of precedence is: * cmdline > environment > config * - * In order to achieve this, we must process configuration in - * different order for the log level versus the filters and - * outputs. Because filters and outputs append, we have to look at - * the environment first and then only check the config file if - * there was no result from the environment. The default output is - * then applied only if there was no setting from either of the - * first two. Because we don't have a way to determine if the log - * level has been set, we must process variables in the opposite + * The default output is applied only if there was no setting from either + * the config or the environment. Because we don't have a way to determine + * if the log level has been set, we must process variables in the opposite * order, each one overriding the previous. */ if (config->log_level != 0) virLogSetDefaultPriority(config->log_level);
+ if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0) + return -1; + + /* In case the config is empty, the filters become empty and outputs will + * be set to default + */ + virLogSetFilters(config->log_filters); + virLogSetOutputs(config->log_outputs);
Existing, but each ignores the return value and could be wrapped that way too. Repeated for all 3 consumers... Your call on adding the ignore_value(); around it. ACK John [...]

Enable libvirt users to query logging output settings. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 37 +++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 +++++++++++++++- src/admin/admin_remote.c | 35 +++++++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 ++++++++ src/libvirt-admin.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 5 +++++ 8 files changed, 146 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index a3c8b89..4fa3851 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -383,4 +383,41 @@ adminDispatchServerSetClientLimits(virNetServerPtr server ATTRIBUTE_UNUSED, virObjectUnref(srv); return rv; } + +/* Returns the number of outputs stored in @outputs */ +static int +adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) +{ + char *tmp = NULL; + + virCheckFlags(0, -1); + + if (!(tmp = virLogGetOutputs())) + return -1; + + *outputs = tmp; + return virLogGetNbOutputs(); +} + +static int +adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + admin_connect_get_logging_outputs_args *args, + admin_connect_get_logging_outputs_ret *ret) +{ + char *outputs = NULL; + int noutputs = 0; + + if ((noutputs = adminConnectGetLoggingOutputs(&outputs, args->flags) < 0)) { + virNetMessageSaveError(rerr); + return -1; + } + + VIR_STEAL_PTR(ret->outputs, outputs); + ret->noutputs = noutputs; + + return 0; +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index c810be3..46d2155 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -404,6 +404,10 @@ int virAdmServerSetClientLimits(virAdmServerPtr srv, int nparams, 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 5963233..237632a 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -183,6 +183,15 @@ struct admin_server_set_client_limits_args { unsigned int flags; }; +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; @@ -268,5 +277,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13 + ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, + + /** + * @generate: none + */ + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index f37ff5e..e472484 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -425,3 +425,38 @@ remoteAdminServerSetClientLimits(virAdmServerPtr srv, virObjectUnlock(priv); return rv; } + +static int +remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn, + char **outputs, + unsigned int flags) +{ + int rv = -1; + 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) + VIR_STEAL_PTR(*outputs, ret.outputs); + + rv = ret.noutputs; + 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 1437d9e..096bf5a 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -127,6 +127,13 @@ struct admin_server_set_client_limits_args { } params; u_int flags; }; +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, @@ -141,4 +148,5 @@ enum admin_procedure { ADMIN_PROC_CLIENT_CLOSE = 11, ADMIN_PROC_SERVER_GET_CLIENT_LIMITS = 12, ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 8877e49..13cba1d 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1084,3 +1084,43 @@ virAdmServerSetClientLimits(virAdmServerPtr srv, virDispatchError(NULL); return ret; } + +/** + * 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) or + * NULL if just the number of defined outputs is required + * @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); + + 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 191e3f2..7160af6 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -10,6 +10,8 @@ xdr_admin_client_close_args; xdr_admin_client_get_info_args; xdr_admin_client_get_info_ret; xdr_admin_connect_get_lib_version_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 2de28e9..9343a4f 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -39,3 +39,8 @@ LIBVIRT_ADMIN_2.0.0 { virAdmServerGetClientLimits; virAdmServerSetClientLimits; }; + +LIBVIRT_ADMIN_2.5.0 { + global: + virAdmConnectGetLoggingOutputs; +} LIBVIRT_ADMIN_2.0.0; -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Enable libvirt users to query logging output settings.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 37 +++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 +++++++++++++++- src/admin/admin_remote.c | 35 +++++++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 ++++++++ src/libvirt-admin.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 5 +++++ 8 files changed, 146 insertions(+), 1 deletion(-)
[...]
diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 2de28e9..9343a4f 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -39,3 +39,8 @@ LIBVIRT_ADMIN_2.0.0 { virAdmServerGetClientLimits; virAdmServerSetClientLimits; }; + +LIBVIRT_ADMIN_2.5.0 { + global: + virAdmConnectGetLoggingOutputs; +} LIBVIRT_ADMIN_2.0.0;
s/2.5.0/3.0.0/ ACK John

Enable libvirt users to query logging filter settings. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 47 +++++++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 +++++++++++++- src/admin/admin_remote.c | 35 ++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 +++++++ src/libvirt-admin.c | 41 +++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 8 files changed, 153 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index 4fa3851..f3bc1de 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -399,6 +399,22 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) return virLogGetNbOutputs(); } +/* Returns the number of defined filters or -1 in case of an error */ +static int +adminConnectGetLoggingFilters(char **filters, unsigned int flags) +{ + char *tmp = NULL; + int ret = 0; + + virCheckFlags(0, -1); + + if ((ret = virLogGetNbFilters()) > 0 && !(tmp = virLogGetFilters())) + return -1; + + *filters = tmp; + return ret; +} + static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, @@ -420,4 +436,35 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, return 0; } + +static int +adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + admin_connect_get_logging_filters_args *args, + admin_connect_get_logging_filters_ret *ret) +{ + char *filters = NULL; + int nfilters = 0; + + if ((nfilters = adminConnectGetLoggingFilters(&filters, args->flags)) < 0) { + virNetMessageSaveError(rerr); + return -1; + } + + if (nfilters == 0) { + ret->filters = NULL; + } else { + char **ret_filters = NULL; + if (VIR_ALLOC(ret_filters) < 0) + return -1; + + *ret_filters = filters; + ret->filters = ret_filters; + } + ret->nfilters = nfilters; + + return 0; +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 46d2155..d76ac20 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -408,6 +408,10 @@ int virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, char **outputs, unsigned int flags); +int virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 237632a..8316bc4 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -192,6 +192,15 @@ struct admin_connect_get_logging_outputs_ret { unsigned int noutputs; }; +struct admin_connect_get_logging_filters_args { + unsigned int flags; +}; + +struct admin_connect_get_logging_filters_ret { + admin_string filters; + unsigned int nfilters; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -282,5 +291,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14 + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, + + /** + * @generate: none + */ + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index e472484..b29d109 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -460,3 +460,38 @@ remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn, virObjectUnlock(priv); return rv; } + +static int +remoteAdminConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags) +{ + int rv = -1; + remoteAdminPrivPtr priv = conn->privateData; + admin_connect_get_logging_filters_args args; + admin_connect_get_logging_filters_ret ret; + + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + virObjectLock(priv); + + if (call(conn, + 0, + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS, + (xdrproc_t) xdr_admin_connect_get_logging_filters_args, + (char *) &args, + (xdrproc_t) xdr_admin_connect_get_logging_filters_ret, + (char *) &ret) == -1) + goto done; + + if (filters) + *filters = ret.filters ? *ret.filters : NULL; + + rv = ret.nfilters; + VIR_FREE(ret.filters); + + done: + virObjectUnlock(priv); + return rv; +} diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 096bf5a..c172557 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -134,6 +134,13 @@ struct admin_connect_get_logging_outputs_ret { admin_nonnull_string outputs; u_int noutputs; }; +struct admin_connect_get_logging_filters_args { + u_int flags; +}; +struct admin_connect_get_logging_filters_ret { + admin_string filters; + u_int nfilters; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -149,4 +156,5 @@ enum admin_procedure { ADMIN_PROC_SERVER_GET_CLIENT_LIMITS = 12, ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 13cba1d..29754a8 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1124,3 +1124,44 @@ virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectGetLoggingFilters: + * @conn: pointer to an active admin connection + * @filters: pointer to a variable to store a string containing all currently + * defined logging filters on daemon (allocated automatically) or + * NULL if just the number of defined outputs is required + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Retrieves a list of currently installed logging filters. Filters returned + * are contained within an automatically allocated string and delimited by + * spaces. The format of each filter conforms to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + * + * To retrieve individual filters, additional parsing needs to be done by the + * caller. Caller is also responsible for freeing @filters correctly. + * + * Returns the number of filters returned in @filters, or -1 in case of + * an error. + */ +int +virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + + if ((ret = remoteAdminConnectGetLoggingFilters(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 7160af6..f2e03d1 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -10,6 +10,8 @@ xdr_admin_client_close_args; xdr_admin_client_get_info_args; xdr_admin_client_get_info_ret; xdr_admin_connect_get_lib_version_ret; +xdr_admin_connect_get_logging_filters_args; +xdr_admin_connect_get_logging_filters_ret; xdr_admin_connect_get_logging_outputs_args; xdr_admin_connect_get_logging_outputs_ret; xdr_admin_connect_list_servers_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 9343a4f..234c50e 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -43,4 +43,5 @@ LIBVIRT_ADMIN_2.0.0 { LIBVIRT_ADMIN_2.5.0 { global: virAdmConnectGetLoggingOutputs; + virAdmConnectGetLoggingFilters; } LIBVIRT_ADMIN_2.0.0; -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Enable libvirt users to query logging filter settings.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 47 +++++++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 +++++++++++++- src/admin/admin_remote.c | 35 ++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 +++++++ src/libvirt-admin.c | 41 +++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 8 files changed, 153 insertions(+), 1 deletion(-)
diff --git a/daemon/admin.c b/daemon/admin.c index 4fa3851..f3bc1de 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -399,6 +399,22 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) return virLogGetNbOutputs(); }
+/* Returns the number of defined filters or -1 in case of an error */ +static int +adminConnectGetLoggingFilters(char **filters, unsigned int flags) +{ + char *tmp = NULL; + int ret = 0; + + virCheckFlags(0, -1); + + if ((ret = virLogGetNbFilters()) > 0 && !(tmp = virLogGetFilters())) + return -1; + + *filters = tmp; + return ret; +} + static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, @@ -420,4 +436,35 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED,
return 0; } + +static int +adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + admin_connect_get_logging_filters_args *args, + admin_connect_get_logging_filters_ret *ret) +{ + char *filters = NULL; + int nfilters = 0; + + if ((nfilters = adminConnectGetLoggingFilters(&filters, args->flags)) < 0) { + virNetMessageSaveError(rerr); + return -1; + } + + if (nfilters == 0) { + ret->filters = NULL; + } else { + char **ret_filters = NULL; + if (VIR_ALLOC(ret_filters) < 0) + return -1; + + *ret_filters = filters; + ret->filters = ret_filters;
So while this works - why the extra effort to allocate an array of 1 item to stuff the returned filter into it? While I understand outputs has a default and thus doesn't return NULL - the adminConnectGetLoggingFilters will return : -1 on failure 0 w/ NULL filters # w/ non-NULL filters So I believe you could still just VIR_STEAL_PTR(ret->filters, filters) as it doesn't distinguish if 'b' is NULL or not - it just assigns it.
+ } + ret->nfilters = nfilters; + + return 0; +} #include "admin_dispatch.h" diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 46d2155..d76ac20 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -408,6 +408,10 @@ int virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, char **outputs, unsigned int flags);
+int virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 237632a..8316bc4 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -192,6 +192,15 @@ struct admin_connect_get_logging_outputs_ret { unsigned int noutputs; };
+struct admin_connect_get_logging_filters_args { + unsigned int flags; +}; + +struct admin_connect_get_logging_filters_ret { + admin_string filters; + unsigned int nfilters; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -282,5 +291,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14 + ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, + + /** + * @generate: none + */ + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index e472484..b29d109 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -460,3 +460,38 @@ remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn, virObjectUnlock(priv); return rv; } + +static int +remoteAdminConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags) +{ + int rv = -1; + remoteAdminPrivPtr priv = conn->privateData; + admin_connect_get_logging_filters_args args; + admin_connect_get_logging_filters_ret ret; + + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + virObjectLock(priv); + + if (call(conn, + 0, + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS, + (xdrproc_t) xdr_admin_connect_get_logging_filters_args, + (char *) &args, + (xdrproc_t) xdr_admin_connect_get_logging_filters_ret, + (char *) &ret) == -1) + goto done; + + if (filters) + *filters = ret.filters ? *ret.filters : NULL;
Thus when you get back here you can just still *filters and not care that ret.filters is an array of 1 item.
+ + rv = ret.nfilters; + VIR_FREE(ret.filters); + + done: + virObjectUnlock(priv); + return rv; +} diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 096bf5a..c172557 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -134,6 +134,13 @@ struct admin_connect_get_logging_outputs_ret { admin_nonnull_string outputs; u_int noutputs; }; +struct admin_connect_get_logging_filters_args { + u_int flags; +}; +struct admin_connect_get_logging_filters_ret { + admin_string filters; + u_int nfilters; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -149,4 +156,5 @@ enum admin_procedure { ADMIN_PROC_SERVER_GET_CLIENT_LIMITS = 12, ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 13cba1d..29754a8 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1124,3 +1124,44 @@ virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectGetLoggingFilters: + * @conn: pointer to an active admin connection + * @filters: pointer to a variable to store a string containing all currently + * defined logging filters on daemon (allocated automatically) or + * NULL if just the number of defined outputs is required
s/outputs/filters Since you seem to be allowing the or NULL....
+ * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Retrieves a list of currently installed logging filters. Filters returned + * are contained within an automatically allocated string and delimited by + * spaces. The format of each filter conforms to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + * + * To retrieve individual filters, additional parsing needs to be done by the + * caller. Caller is also responsible for freeing @filters correctly. + * + * Returns the number of filters returned in @filters, or -1 in case of + * an error. + */ +int +virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, + char **filters, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%x", conn, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + + if ((ret = remoteAdminConnectGetLoggingFilters(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 7160af6..f2e03d1 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -10,6 +10,8 @@ xdr_admin_client_close_args; xdr_admin_client_get_info_args; xdr_admin_client_get_info_ret; xdr_admin_connect_get_lib_version_ret; +xdr_admin_connect_get_logging_filters_args; +xdr_admin_connect_get_logging_filters_ret; xdr_admin_connect_get_logging_outputs_args; xdr_admin_connect_get_logging_outputs_ret; xdr_admin_connect_list_servers_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 9343a4f..234c50e 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -43,4 +43,5 @@ LIBVIRT_ADMIN_2.0.0 { LIBVIRT_ADMIN_2.5.0 { global: virAdmConnectGetLoggingOutputs; + virAdmConnectGetLoggingFilters; } LIBVIRT_ADMIN_2.0.0;
s/2.5.0/3.0.0 ACK w/ adjustments John

On Fri, Dec 09, 2016 at 06:59:57AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Enable libvirt users to query logging filter settings.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- daemon/admin.c | 47 +++++++++++++++++++++++++++++++++++++++++ include/libvirt/libvirt-admin.h | 4 ++++ src/admin/admin_protocol.x | 16 +++++++++++++- src/admin/admin_remote.c | 35 ++++++++++++++++++++++++++++++ src/admin_protocol-structs | 8 +++++++ src/libvirt-admin.c | 41 +++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 2 ++ src/libvirt_admin_public.syms | 1 + 8 files changed, 153 insertions(+), 1 deletion(-)
diff --git a/daemon/admin.c b/daemon/admin.c index 4fa3851..f3bc1de 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -399,6 +399,22 @@ adminConnectGetLoggingOutputs(char **outputs, unsigned int flags) return virLogGetNbOutputs(); }
+/* Returns the number of defined filters or -1 in case of an error */ +static int +adminConnectGetLoggingFilters(char **filters, unsigned int flags) +{ + char *tmp = NULL; + int ret = 0; + + virCheckFlags(0, -1); + + if ((ret = virLogGetNbFilters()) > 0 && !(tmp = virLogGetFilters())) + return -1; + + *filters = tmp; + return ret; +} + static int adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client ATTRIBUTE_UNUSED, @@ -420,4 +436,35 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr server ATTRIBUTE_UNUSED,
return 0; } + +static int +adminDispatchConnectGetLoggingFilters(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + admin_connect_get_logging_filters_args *args, + admin_connect_get_logging_filters_ret *ret) +{ + char *filters = NULL; + int nfilters = 0; + + if ((nfilters = adminConnectGetLoggingFilters(&filters, args->flags)) < 0) { + virNetMessageSaveError(rerr); + return -1; + } + + if (nfilters == 0) { + ret->filters = NULL; + } else { + char **ret_filters = NULL; + if (VIR_ALLOC(ret_filters) < 0) + return -1; + + *ret_filters = filters; + ret->filters = ret_filters;
So while this works - why the extra effort to allocate an array of 1 item to stuff the returned filter into it?
While I understand outputs has a default and thus doesn't return NULL - the adminConnectGetLoggingFilters will return :
-1 on failure 0 w/ NULL filters # w/ non-NULL filters
So I believe you could still just VIR_STEAL_PTR(ret->filters, filters) as it doesn't distinguish if 'b' is NULL or not - it just assigns it.
I almost forgot to reply to this patch. Unfortunately, VIR_STEAL_PTR is not enough and that's because of the way XDR stores pointers - string that actually can be NULL is defined as char **, so you have to allocate the 1 member array because what virNetServerProgramDispatchCall will then do is call xdr_free() which will try to free both ret->filters and *ret->filters - it's just how XDR defines it. I remember wanting to do it as you suggest (the intuitive way) but then the daemon either crashed or I got an RPC error so I looked at the XDR spec and found out that to be able to pass both NULL and string data in the same data type, double pointer is needed. Erik

Enable libvirt users to modify daemon's logging output settings from outside. If either an empty string or NULL is passed, a default logging output will be used the same way as it would be in case writing an empty string to the libvirtd.conf Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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 | 38 ++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 70 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index f3bc1de..16a3453 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -416,6 +416,16 @@ adminConnectGetLoggingFilters(char **filters, unsigned int flags) } static int +adminConnectSetLoggingOutputs(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *outputs, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogSetOutputs(outputs); +} + +static int adminDispatchConnectGetLoggingOutputs(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 d76ac20..aa33fef 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -412,6 +412,10 @@ int virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, char **filters, unsigned int flags); +int virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, + const char *outputs, + unsigned int flags); + # ifdef __cplusplus } # endif diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index 8316bc4..1d2116d 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -201,6 +201,11 @@ struct admin_connect_get_logging_filters_ret { unsigned int nfilters; }; +struct admin_connect_set_logging_outputs_args { + admin_string outputs; + unsigned int flags; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -296,5 +301,10 @@ enum admin_procedure { /** * @generate: none */ - ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15 + ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index c172557..04bbd7a 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -141,6 +141,10 @@ struct admin_connect_get_logging_filters_ret { admin_string filters; u_int nfilters; }; +struct admin_connect_set_logging_outputs_args { + admin_string outputs; + u_int flags; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -157,4 +161,5 @@ enum admin_procedure { ADMIN_PROC_SERVER_SET_CLIENT_LIMITS = 13, ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 29754a8..2bc8b2c 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1165,3 +1165,41 @@ virAdmConnectGetLoggingFilters(virAdmConnectPtr conn, virDispatchError(NULL); return -1; } + +/** + * virAdmConnectSetLoggingOutputs: + * @conn: pointer to an active admin connection + * @outputs: pointer to a string containing a list of outputs to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Redefine the existing (set of) outputs(s) with a new one specified in + * @outputs. If multiple outputs are specified, they need to be delimited by + * spaces. The format of each output must conform to the format described in + * daemon's configuration file (e.g. libvirtd.conf). + * + * To reset the existing (set of) output(s) to libvirt's defaults, an empty + * string ("") or NULL should be passed in @outputs. + * + * Returns 0 if the new output or the set of outputs has been defined + * successfully, or -1 in case of an error. + */ +int +virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, + const char *outputs, + unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, outputs=%s, flags=%x", conn, outputs, flags); + + virResetLastError(); + virCheckAdmConnectReturn(conn, -1); + + if ((ret = remoteAdminConnectSetLoggingOutputs(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 f2e03d1..ad7fd1f 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -19,6 +19,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_outputs_args; xdr_admin_server_get_client_limits_args; xdr_admin_server_get_client_limits_ret; xdr_admin_server_get_threadpool_parameters_args; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 234c50e..d4713da 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -44,4 +44,5 @@ LIBVIRT_ADMIN_2.5.0 { global: virAdmConnectGetLoggingOutputs; virAdmConnectGetLoggingFilters; + virAdmConnectSetLoggingOutputs; } LIBVIRT_ADMIN_2.0.0; -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Enable libvirt users to modify daemon's logging output settings from outside. If either an empty string or NULL is passed, a default logging output will be used the same way as it would be in case writing an empty string to the libvirtd.conf
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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 | 38 ++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 70 insertions(+), 1 deletion(-)
[...]
diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 234c50e..d4713da 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -44,4 +44,5 @@ LIBVIRT_ADMIN_2.5.0 { global: virAdmConnectGetLoggingOutputs; virAdmConnectGetLoggingFilters; + virAdmConnectSetLoggingOutputs; } LIBVIRT_ADMIN_2.0.0;
s/2.5.0/3.0.0 Of course makes me think "eventually" would someone want or find it useful to have "Add" or "Remove" options too rather than just wholesale replacement... Maybe it's just the fresh coffee talking. ACK John

On Fri, Dec 09, 2016 at 07:12:38AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Enable libvirt users to modify daemon's logging output settings from outside. If either an empty string or NULL is passed, a default logging output will be used the same way as it would be in case writing an empty string to the libvirtd.conf
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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 | 38 ++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 70 insertions(+), 1 deletion(-)
[...]
diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 234c50e..d4713da 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -44,4 +44,5 @@ LIBVIRT_ADMIN_2.5.0 { global: virAdmConnectGetLoggingOutputs; virAdmConnectGetLoggingFilters; + virAdmConnectSetLoggingOutputs; } LIBVIRT_ADMIN_2.0.0;
s/2.5.0/3.0.0
Of course makes me think "eventually" would someone want or find it useful to have "Add" or "Remove" options too rather than just wholesale replacement... Maybe it's just the fresh coffee talking.
I thought about this a while back and yes, I can imagine someone wanting that, but at the same I think we should not push features that nobody has a usecase for because you know, just in case...So, sure, we can add that easily, it just isn't urgent IMHO. Erik
ACK
John

Enable libvirt users to modify logging filters of a daemon from outside. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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 | 38 ++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 70 insertions(+), 1 deletion(-) diff --git a/daemon/admin.c b/daemon/admin.c index 16a3453..c5678bb 100644 --- a/daemon/admin.c +++ b/daemon/admin.c @@ -426,6 +426,16 @@ adminConnectSetLoggingOutputs(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, } static int +adminConnectSetLoggingFilters(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *filters, + unsigned int flags) +{ + virCheckFlags(0, -1); + + return virLogSetFilters(filters); +} + +static int adminDispatchConnectGetLoggingOutputs(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 aa33fef..161727e 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -416,6 +416,10 @@ int virAdmConnectSetLoggingOutputs(virAdmConnectPtr conn, const char *outputs, 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 1d2116d..d19d132 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -206,6 +206,11 @@ struct admin_connect_set_logging_outputs_args { unsigned int flags; }; +struct admin_connect_set_logging_filters_args { + admin_string filters; + unsigned int flags; +}; + /* Define the program number, protocol version and procedure numbers here. */ const ADMIN_PROGRAM = 0x06900690; const ADMIN_PROTOCOL_VERSION = 1; @@ -306,5 +311,10 @@ enum admin_procedure { /** * @generate: both */ - ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16 + ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, + + /** + * @generate: both + */ + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17 }; diff --git a/src/admin_protocol-structs b/src/admin_protocol-structs index 04bbd7a..a6c5380 100644 --- a/src/admin_protocol-structs +++ b/src/admin_protocol-structs @@ -145,6 +145,10 @@ struct admin_connect_set_logging_outputs_args { admin_string outputs; u_int flags; }; +struct admin_connect_set_logging_filters_args { + admin_string filters; + u_int flags; +}; enum admin_procedure { ADMIN_PROC_CONNECT_OPEN = 1, ADMIN_PROC_CONNECT_CLOSE = 2, @@ -162,4 +166,5 @@ enum admin_procedure { ADMIN_PROC_CONNECT_GET_LOGGING_OUTPUTS = 14, ADMIN_PROC_CONNECT_GET_LOGGING_FILTERS = 15, ADMIN_PROC_CONNECT_SET_LOGGING_OUTPUTS = 16, + ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17, }; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 2bc8b2c..67d9b6b 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -1203,3 +1203,41 @@ virAdmConnectSetLoggingOutputs(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). + * + * To clear the currently defined (set of) filter(s), pass either an empty + * string ("") or NULL in @filters. + * + * 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); + + 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 ad7fd1f..9526412 100644 --- a/src/libvirt_admin_private.syms +++ b/src/libvirt_admin_private.syms @@ -19,6 +19,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_outputs_args; xdr_admin_server_get_client_limits_args; xdr_admin_server_get_client_limits_ret; diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index d4713da..4f0be7f 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -45,4 +45,5 @@ LIBVIRT_ADMIN_2.5.0 { virAdmConnectGetLoggingOutputs; virAdmConnectGetLoggingFilters; virAdmConnectSetLoggingOutputs; + virAdmConnectSetLoggingFilters; } LIBVIRT_ADMIN_2.0.0; -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Enable libvirt users to modify logging filters of a daemon from outside.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- 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 | 38 ++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_private.syms | 1 + src/libvirt_admin_public.syms | 1 + 7 files changed, 70 insertions(+), 1 deletion(-)
[...]
diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index d4713da..4f0be7f 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -45,4 +45,5 @@ LIBVIRT_ADMIN_2.5.0 { virAdmConnectGetLoggingOutputs; virAdmConnectGetLoggingFilters; virAdmConnectSetLoggingOutputs; + virAdmConnectSetLoggingFilters; } LIBVIRT_ADMIN_2.0.0;
s/2.5.0/3.0.0 ACK John

Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 70b0e51..0b9945a 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -971,6 +971,114 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd) goto cleanup; } +/* -------------------------- + * Command daemon-log-filters + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_filters[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging filters on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of filters on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_filters[] = { + {.name = "filters", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging filters"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) +{ + int nfilters; + char *filters = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "filters")) { + if ((vshCommandOptStringReq(ctl, cmd, "filters", + (const char **) &filters) < 0 || + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging filters: ")); + vshPrint(ctl, "%s\n", filters ? filters : ""); + } + + return true; +} + +/* -------------------------- + * Command daemon-log-outputs + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_outputs[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging outputs on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of outputs on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_outputs[] = { + {.name = "outputs", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging outputs"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) +{ + int noutputs; + char *outputs = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "outputs")) { + if ((vshCommandOptStringReq(ctl, cmd, "outputs", + (const char **) &outputs) < 0 || + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, + &outputs, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging outputs: ")); + vshPrint(ctl, "%s\n", outputs ? outputs : ""); + } + + return true; +} + static void * vshAdmConnectionHandler(vshControl *ctl) { @@ -1341,6 +1449,18 @@ static const vshCmdDef managementCmds[] = { .info = info_srv_clients_set, .flags = 0 }, + {.name = "daemon-log-filters", + .handler = cmdDaemonLogFilters, + .opts = opts_daemon_log_filters, + .info = info_daemon_log_filters, + .flags = 0 + }, + {.name = "daemon-log-outputs", + .handler = cmdDaemonLogOutputs, + .opts = opts_daemon_log_outputs, + .info = info_daemon_log_outputs, + .flags = 0 + }, {.name = NULL} }; diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod index 443730e..d3f1a35 100644 --- a/tools/virt-admin.pod +++ b/tools/virt-admin.pod @@ -155,6 +155,96 @@ change its internal configuration. Lists all manageable servers contained within the daemon the client is currently connected to. +=item B<daemon-log-info> [I<--outputs> B<string>] [I<--filters> B<string>] + +Print the currently defined set of logging outputs and/or filters. + +=over 4 + +=item I<--outputs> + +Print the currently defined set of logging outputs + +=item I<--filters> + +Print the currently defined set of logging filters + +=back + +=item B<daemon-log-define> + +Redefine the currently defined sets of logging outputs and/or filters with +completely new ones. + +=over 4 + +=item I<--outputs> + +Replaces the current set of logging outputs with a new one where multiple +outputs are delimited by space and each output must be of the following form: + +I<< X:<destination>:<auxiliary_data> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level: + +=over 4 + +=item * 1: DEBUG + +=item * 2: INFO + +=item * 3: WARNING + +=item * 4: ERROR + +=back + +=item * I<< <destination> >> is one of I<stderr>, I<file>, I<syslog>, or I<journald> + +=item * I<< <auxiliary_data> >> is only required for the I<file> and I<syslog> +destination types and shall therefore contain either a path to a file or a message tag +respectively, see the B<Examples> section below. + +=back + +=item I<--filters> + +Replaces the current set of logging filters with a new one where multiple +filters are delimited by space and each filter must be of the following form: + +I<< X:<match_string> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level the same way as for I<--outputs> + +=item * I<< <match_string> >> references either a specific module in libvirt's +source tree or the whole directory in the source tree, thus affecting all +modules in the directory + +=back + +=back + +B<Examples> + To replace the current logging output with one that writes to a file while + logging logging errors only, the following could be used: + + $ virt-admin daemon-log-define --outputs "4:file:<path_to_the_file>" + + To define multiple outputs at once: + + $ virt-admin daemon-log-define --outputs "4:stderr 2:syslog:<tag>" + + To define a filter which suppresses all e.g. 'virObjectUnref' DEBUG + messages, use the following (note the '.' symbol which can be used to more + fine-grained filters tailored to specific modules, in contrast, to affect + a whole directory containing several modules it would become "4:util"): + + $ virt-admin daemon-log-define --filters "4:util.object" + =back =head1 SERVER COMMANDS -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 70b0e51..0b9945a 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -971,6 +971,114 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+/* -------------------------- + * Command daemon-log-filters + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_filters[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging filters on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of filters on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_filters[] = { + {.name = "filters", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging filters"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) +{ + int nfilters; + char *filters = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "filters")) { + if ((vshCommandOptStringReq(ctl, cmd, "filters", + (const char **) &filters) < 0 || + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging filters: ")); + vshPrint(ctl, "%s\n", filters ? filters : ""); + } + + return true; +} + +/* -------------------------- + * Command daemon-log-outputs + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_outputs[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging outputs on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of outputs on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_outputs[] = { + {.name = "outputs", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging outputs"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) +{ + int noutputs; + char *outputs = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "outputs")) { + if ((vshCommandOptStringReq(ctl, cmd, "outputs", + (const char **) &outputs) < 0 || + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, + &outputs, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging outputs: ")); + vshPrint(ctl, "%s\n", outputs ? outputs : ""); + } + + return true; +} + static void * vshAdmConnectionHandler(vshControl *ctl) { @@ -1341,6 +1449,18 @@ static const vshCmdDef managementCmds[] = { .info = info_srv_clients_set, .flags = 0 }, + {.name = "daemon-log-filters", + .handler = cmdDaemonLogFilters, + .opts = opts_daemon_log_filters, + .info = info_daemon_log_filters, + .flags = 0 + }, + {.name = "daemon-log-outputs", + .handler = cmdDaemonLogOutputs, + .opts = opts_daemon_log_outputs, + .info = info_daemon_log_outputs, + .flags = 0 + }, {.name = NULL} };
diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod index 443730e..d3f1a35 100644 --- a/tools/virt-admin.pod +++ b/tools/virt-admin.pod
All of this is way off - no such command as daemon-log-info or daemon-log-define. I think you should reference /etc/libvirt/libvirtd.conf in order to point at the place which describes the format. Although I trust you know what should be there so... ACK once you clean this up. John
@@ -155,6 +155,96 @@ change its internal configuration. Lists all manageable servers contained within the daemon the client is currently connected to.
+=item B<daemon-log-info> [I<--outputs> B<string>] [I<--filters> B<string>] + +Print the currently defined set of logging outputs and/or filters. + +=over 4 + +=item I<--outputs> + +Print the currently defined set of logging outputs + +=item I<--filters> + +Print the currently defined set of logging filters + +=back + +=item B<daemon-log-define> + +Redefine the currently defined sets of logging outputs and/or filters with +completely new ones. + +=over 4 + +=item I<--outputs> + +Replaces the current set of logging outputs with a new one where multiple +outputs are delimited by space and each output must be of the following form: + +I<< X:<destination>:<auxiliary_data> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level: + +=over 4 + +=item * 1: DEBUG + +=item * 2: INFO + +=item * 3: WARNING + +=item * 4: ERROR + +=back + +=item * I<< <destination> >> is one of I<stderr>, I<file>, I<syslog>, or I<journald> + +=item * I<< <auxiliary_data> >> is only required for the I<file> and I<syslog> +destination types and shall therefore contain either a path to a file or a message tag +respectively, see the B<Examples> section below. + +=back + +=item I<--filters> + +Replaces the current set of logging filters with a new one where multiple +filters are delimited by space and each filter must be of the following form: + +I<< X:<match_string> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level the same way as for I<--outputs> + +=item * I<< <match_string> >> references either a specific module in libvirt's +source tree or the whole directory in the source tree, thus affecting all +modules in the directory + +=back + +=back + +B<Examples> + To replace the current logging output with one that writes to a file while + logging logging errors only, the following could be used: + + $ virt-admin daemon-log-define --outputs "4:file:<path_to_the_file>" + + To define multiple outputs at once: + + $ virt-admin daemon-log-define --outputs "4:stderr 2:syslog:<tag>" + + To define a filter which suppresses all e.g. 'virObjectUnref' DEBUG + messages, use the following (note the '.' symbol which can be used to more + fine-grained filters tailored to specific modules, in contrast, to affect + a whole directory containing several modules it would become "4:util"): + + $ virt-admin daemon-log-define --filters "4:util.object" + =back
=head1 SERVER COMMANDS

On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have to create a logfile with that name on the filesystem. When we retrieve it, I think we should print it as we created it - garbage. The other option we have is to strip all the control characters and print the rest which in the worst case scenario will end up as an empty string...
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 70b0e51..0b9945a 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -971,6 +971,114 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+/* -------------------------- + * Command daemon-log-filters + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_filters[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging filters on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of filters on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_filters[] = { + {.name = "filters", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging filters"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) +{ + int nfilters; + char *filters = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "filters")) { + if ((vshCommandOptStringReq(ctl, cmd, "filters", + (const char **) &filters) < 0 || + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging filters: ")); + vshPrint(ctl, "%s\n", filters ? filters : ""); + } + + return true; +} + +/* -------------------------- + * Command daemon-log-outputs + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_outputs[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging outputs on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of outputs on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_outputs[] = { + {.name = "outputs", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging outputs"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) +{ + int noutputs; + char *outputs = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "outputs")) { + if ((vshCommandOptStringReq(ctl, cmd, "outputs", + (const char **) &outputs) < 0 || + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, + &outputs, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging outputs: ")); + vshPrint(ctl, "%s\n", outputs ? outputs : ""); + } + + return true; +} + static void * vshAdmConnectionHandler(vshControl *ctl) { @@ -1341,6 +1449,18 @@ static const vshCmdDef managementCmds[] = { .info = info_srv_clients_set, .flags = 0 }, + {.name = "daemon-log-filters", + .handler = cmdDaemonLogFilters, + .opts = opts_daemon_log_filters, + .info = info_daemon_log_filters, + .flags = 0 + }, + {.name = "daemon-log-outputs", + .handler = cmdDaemonLogOutputs, + .opts = opts_daemon_log_outputs, + .info = info_daemon_log_outputs, + .flags = 0 + }, {.name = NULL} };
diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod index 443730e..d3f1a35 100644 --- a/tools/virt-admin.pod +++ b/tools/virt-admin.pod
All of this is way off - no such command as daemon-log-info or daemon-log-define.
I think you should reference /etc/libvirt/libvirtd.conf in order to point at the place which describes the format.
Not sure what you mean, should I just add a reference or replace the sections explaining the format with the reference completely? Erik
Although I trust you know what should be there so...
ACK once you clean this up.
John
@@ -155,6 +155,96 @@ change its internal configuration. Lists all manageable servers contained within the daemon the client is currently connected to.
+=item B<daemon-log-info> [I<--outputs> B<string>] [I<--filters> B<string>] + +Print the currently defined set of logging outputs and/or filters. + +=over 4 + +=item I<--outputs> + +Print the currently defined set of logging outputs + +=item I<--filters> + +Print the currently defined set of logging filters + +=back + +=item B<daemon-log-define> + +Redefine the currently defined sets of logging outputs and/or filters with +completely new ones. + +=over 4 + +=item I<--outputs> + +Replaces the current set of logging outputs with a new one where multiple +outputs are delimited by space and each output must be of the following form: + +I<< X:<destination>:<auxiliary_data> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level: + +=over 4 + +=item * 1: DEBUG + +=item * 2: INFO + +=item * 3: WARNING + +=item * 4: ERROR + +=back + +=item * I<< <destination> >> is one of I<stderr>, I<file>, I<syslog>, or I<journald> + +=item * I<< <auxiliary_data> >> is only required for the I<file> and I<syslog> +destination types and shall therefore contain either a path to a file or a message tag +respectively, see the B<Examples> section below. + +=back + +=item I<--filters> + +Replaces the current set of logging filters with a new one where multiple +filters are delimited by space and each filter must be of the following form: + +I<< X:<match_string> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level the same way as for I<--outputs> + +=item * I<< <match_string> >> references either a specific module in libvirt's +source tree or the whole directory in the source tree, thus affecting all +modules in the directory + +=back + +=back + +B<Examples> + To replace the current logging output with one that writes to a file while + logging logging errors only, the following could be used: + + $ virt-admin daemon-log-define --outputs "4:file:<path_to_the_file>" + + To define multiple outputs at once: + + $ virt-admin daemon-log-define --outputs "4:stderr 2:syslog:<tag>" + + To define a filter which suppresses all e.g. 'virObjectUnref' DEBUG + messages, use the following (note the '.' symbol which can be used to more + fine-grained filters tailored to specific modules, in contrast, to affect + a whole directory containing several modules it would become "4:util"): + + $ virt-admin daemon-log-define --filters "4:util.object" + =back
=head1 SERVER COMMANDS

On 12/12/2016 10:42 AM, Erik Skultety wrote:
On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have to create a logfile with that name on the filesystem. When we retrieve it, I think we should print it as we created it - garbage. The other option we have is to strip all the control characters and print the rest which in the worst case scenario will end up as an empty string...
If someone provides garbage I thought our general practice was to escape the 'unprintable' chars when we Format the output. The first thing that came to mind that might be close to what's being done here was domain interface script, where when we print out the name we Escape whatever is in script path. Looking at the src/util/virtconf.c for VIR_CONF_STRING and virConfParseString would seem to imply to me the ability to read escaped characters. The difference here is that you're now formatting the name; whereas, previously the formatting was entirely left to whomever edited /etc/libvirt/libvirtd.conf. In the long run, recent memory says whenever there's a user provided character string - the review comments have always been be sure to Escape it. Searching for Escape in tools/*.c found a couple more "output" files.
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 70b0e51..0b9945a 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -971,6 +971,114 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
+/* -------------------------- + * Command daemon-log-filters + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_filters[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging filters on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of filters on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_filters[] = { + {.name = "filters", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging filters"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) +{ + int nfilters; + char *filters = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "filters")) { + if ((vshCommandOptStringReq(ctl, cmd, "filters", + (const char **) &filters) < 0 || + virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging filters: ")); + vshPrint(ctl, "%s\n", filters ? filters : ""); + } + + return true; +} + +/* -------------------------- + * Command daemon-log-outputs + * -------------------------- + */ +static const vshCmdInfo info_daemon_log_outputs[] = { + {.name = "help", + .data = N_("fetch or set the currently defined set of logging outputs on " + "daemon") + }, + {.name = "desc", + .data = N_("Depending on whether run with or without options, the command " + "fetches or redefines the existing active set of outputs on " + "daemon.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_daemon_log_outputs[] = { + {.name = "outputs", + .type = VSH_OT_STRING, + .help = N_("redefine the existing set of logging outputs"), + .flags = VSH_OFLAG_EMPTY_OK + }, + {.name = NULL} +}; + +static bool +cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) +{ + int noutputs; + char *outputs = NULL; + vshAdmControlPtr priv = ctl->privData; + + if (vshCommandOptBool(cmd, "outputs")) { + if ((vshCommandOptStringReq(ctl, cmd, "outputs", + (const char **) &outputs) < 0 || + virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { + vshError(ctl, _("Unable to change daemon logging settings")); + return false; + } + } else { + if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, + &outputs, 0)) < 0) { + vshError(ctl, _("Unable to get daemon logging filters information")); + return false; + } + + vshPrintExtra(ctl, " %-15s", _("Logging outputs: ")); + vshPrint(ctl, "%s\n", outputs ? outputs : ""); + } + + return true; +} + static void * vshAdmConnectionHandler(vshControl *ctl) { @@ -1341,6 +1449,18 @@ static const vshCmdDef managementCmds[] = { .info = info_srv_clients_set, .flags = 0 }, + {.name = "daemon-log-filters", + .handler = cmdDaemonLogFilters, + .opts = opts_daemon_log_filters, + .info = info_daemon_log_filters, + .flags = 0 + }, + {.name = "daemon-log-outputs", + .handler = cmdDaemonLogOutputs, + .opts = opts_daemon_log_outputs, + .info = info_daemon_log_outputs, + .flags = 0 + }, {.name = NULL} };
diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod index 443730e..d3f1a35 100644 --- a/tools/virt-admin.pod +++ b/tools/virt-admin.pod
All of this is way off - no such command as daemon-log-info or daemon-log-define.
I think you should reference /etc/libvirt/libvirtd.conf in order to point at the place which describes the format.
Not sure what you mean, should I just add a reference or replace the sections explaining the format with the reference completely?
I was think more along the lines of adding the reference indicating that log filters are described in detail in the /etc/libvirt/libvirtd.conf file. Similarly for log_outputs. That avoids repeating descriptions here and there. John
Erik
Although I trust you know what should be there so...
ACK once you clean this up.
John
@@ -155,6 +155,96 @@ change its internal configuration. Lists all manageable servers contained within the daemon the client is currently connected to.
+=item B<daemon-log-info> [I<--outputs> B<string>] [I<--filters> B<string>] + +Print the currently defined set of logging outputs and/or filters. + +=over 4 + +=item I<--outputs> + +Print the currently defined set of logging outputs + +=item I<--filters> + +Print the currently defined set of logging filters + +=back + +=item B<daemon-log-define> + +Redefine the currently defined sets of logging outputs and/or filters with +completely new ones. + +=over 4 + +=item I<--outputs> + +Replaces the current set of logging outputs with a new one where multiple +outputs are delimited by space and each output must be of the following form: + +I<< X:<destination>:<auxiliary_data> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level: + +=over 4 + +=item * 1: DEBUG + +=item * 2: INFO + +=item * 3: WARNING + +=item * 4: ERROR + +=back + +=item * I<< <destination> >> is one of I<stderr>, I<file>, I<syslog>, or I<journald> + +=item * I<< <auxiliary_data> >> is only required for the I<file> and I<syslog> +destination types and shall therefore contain either a path to a file or a message tag +respectively, see the B<Examples> section below. + +=back + +=item I<--filters> + +Replaces the current set of logging filters with a new one where multiple +filters are delimited by space and each filter must be of the following form: + +I<< X:<match_string> >> where + +=over 4 + +=item * I<X> denotes the minimal debug level the same way as for I<--outputs> + +=item * I<< <match_string> >> references either a specific module in libvirt's +source tree or the whole directory in the source tree, thus affecting all +modules in the directory + +=back + +=back + +B<Examples> + To replace the current logging output with one that writes to a file while + logging logging errors only, the following could be used: + + $ virt-admin daemon-log-define --outputs "4:file:<path_to_the_file>" + + To define multiple outputs at once: + + $ virt-admin daemon-log-define --outputs "4:stderr 2:syslog:<tag>" + + To define a filter which suppresses all e.g. 'virObjectUnref' DEBUG + messages, use the following (note the '.' symbol which can be used to more + fine-grained filters tailored to specific modules, in contrast, to affect + a whole directory containing several modules it would become "4:util"): + + $ virt-admin daemon-log-define --filters "4:util.object" + =back
=head1 SERVER COMMANDS

On Mon, Dec 12, 2016 at 11:21:08AM -0500, John Ferlan wrote:
On 12/12/2016 10:42 AM, Erik Skultety wrote:
On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have to create a logfile with that name on the filesystem. When we retrieve it, I think we should print it as we created it - garbage. The other option we have is to strip all the control characters and print the rest which in the worst case scenario will end up as an empty string...
If someone provides garbage I thought our general practice was to escape the 'unprintable' chars when we Format the output.
In which case? Yes, we do it for XMLs (hence the function virBufferEscapeString) so that the resulting XML is compliant with the specification. We also do it for shell-related stuff like escaping the arguments that will end up on qemu cmdline. But what's the purpose of escaping in this case? Additionally, if you escape the output and let's say the user saved the output into a environment variable, he now has an escaped garbage which is further unusable when instead we should have gone with the verbatim, unprintable garbage. I played around with this in the morning and found out that bash allows you to use structure like this one: $'nnn' (where nnn is the string to be escaped), now that would be doable from our perspective, but then, I'm not sure whether such a construction is supported with other shells out there.
The first thing that came to mind that might be close to what's being done here was domain interface script, where when we print out the name we Escape whatever is in script path.
Looking at the src/util/virtconf.c for VIR_CONF_STRING and virConfParseString would seem to imply to me the ability to read escaped characters.
The difference here is that you're now formatting the name; whereas, previously the formatting was entirely left to whomever edited /etc/libvirt/libvirtd.conf.
What difference? I put some garbage into the config via vim's controlling sequences and when I used 'cat' on the config, what I got was an unescaped garbage, not an escaped version of what I put there, so in that aspect the behaviour is the same, user provides us with garbage, and in turn, he gets the same garbage back, there shouldn't be any compatibility or functioning issues connected to that.
In the long run, recent memory says whenever there's a user provided character string - the review comments have always been be sure to Escape it.
Searching for Escape in tools/*.c found a couple more "output" files.
I looked at those and as I said, those are XML escaping routines and shell escaping routines present mainly for reasons I mentioned above. Erik

On 12/13/2016 09:32 AM, Erik Skultety wrote:
On Mon, Dec 12, 2016 at 11:21:08AM -0500, John Ferlan wrote:
On 12/12/2016 10:42 AM, Erik Skultety wrote:
On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have to create a logfile with that name on the filesystem. When we retrieve it, I think we should print it as we created it - garbage. The other option we have is to strip all the control characters and print the rest which in the worst case scenario will end up as an empty string...
If someone provides garbage I thought our general practice was to escape the 'unprintable' chars when we Format the output.
In which case? Yes, we do it for XMLs (hence the function virBufferEscapeString) so that the resulting XML is compliant with the specification. We also do it for shell-related stuff like escaping the arguments that will end up on qemu cmdline. But what's the purpose of escaping in this case? Additionally, if you escape the output and let's say the user saved the output into a environment variable, he now has an escaped garbage which is further unusable when instead we should have gone with the verbatim, unprintable garbage. I played around with this in the morning and found out that bash allows you to use structure like this one: $'nnn' (where nnn is the string to be escaped), now that would be doable from our perspective, but then, I'm not sure whether such a construction is supported with other shells out there.
Escaping wasn't a requirement, just a concern. Hence the original question "should there be any concerns along the way over escaping characters? " And by escaping I meant only the name of the file, although perhaps that wasn't clear as I re-read the original comment.
The first thing that came to mind that might be close to what's being done here was domain interface script, where when we print out the name we Escape whatever is in script path.
Looking at the src/util/virtconf.c for VIR_CONF_STRING and virConfParseString would seem to imply to me the ability to read escaped characters.
The difference here is that you're now formatting the name; whereas, previously the formatting was entirely left to whomever edited /etc/libvirt/libvirtd.conf.
What difference? I put some garbage into the config via vim's controlling
semantics... where 'difference' means prior to your changes someone would have to either edit or append libvirtd.conf in order to alter/define the name. With these changes the name (while running) is altered to some file which if it has unprintable characters in the name, how would it look?
sequences and when I used 'cat' on the config, what I got was an unescaped garbage, not an escaped version of what I put there, so in that aspect the behaviour is the same, user provides us with garbage, and in turn, he gets the same garbage back, there shouldn't be any compatibility or functioning issues connected to that.
In the long run, recent memory says whenever there's a user provided character string - the review comments have always been be sure to Escape it.
Searching for Escape in tools/*.c found a couple more "output" files.
I looked at those and as I said, those are XML escaping routines and shell escaping routines present mainly for reasons I mentioned above.
Fair enough - it's been considered... I think what I was going at there though was how virsh-snapshot handles output file names where the 'file=' or 'snapshot=' output is Escaped. John

On Tue, Dec 13, 2016 at 09:50:49AM -0500, John Ferlan wrote:
On 12/13/2016 09:32 AM, Erik Skultety wrote:
On Mon, Dec 12, 2016 at 11:21:08AM -0500, John Ferlan wrote:
On 12/12/2016 10:42 AM, Erik Skultety wrote:
On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote:
Finally, now that all APIs have been introduced, wire them up to virt-admin and introduce daemon-log-outputs and daemon-log-filters commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+)
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have to create a logfile with that name on the filesystem. When we retrieve it, I think we should print it as we created it - garbage. The other option we have is to strip all the control characters and print the rest which in the worst case scenario will end up as an empty string...
If someone provides garbage I thought our general practice was to escape the 'unprintable' chars when we Format the output.
In which case? Yes, we do it for XMLs (hence the function virBufferEscapeString) so that the resulting XML is compliant with the specification. We also do it for shell-related stuff like escaping the arguments that will end up on qemu cmdline. But what's the purpose of escaping in this case? Additionally, if you escape the output and let's say the user saved the output into a environment variable, he now has an escaped garbage which is further unusable when instead we should have gone with the verbatim, unprintable garbage. I played around with this in the morning and found out that bash allows you to use structure like this one: $'nnn' (where nnn is the string to be escaped), now that would be doable from our perspective, but then, I'm not sure whether such a construction is supported with other shells out there.
Escaping wasn't a requirement, just a concern. Hence the original question "should there be any concerns along the way over escaping characters? "
And by escaping I meant only the name of the file, although perhaps that wasn't clear as I re-read the original comment.
No, it was perfectly clear that you meant just the filename, but it doesn't contradict my point anyway.
The first thing that came to mind that might be close to what's being done here was domain interface script, where when we print out the name we Escape whatever is in script path.
Looking at the src/util/virtconf.c for VIR_CONF_STRING and virConfParseString would seem to imply to me the ability to read escaped characters.
The difference here is that you're now formatting the name; whereas, previously the formatting was entirely left to whomever edited /etc/libvirt/libvirtd.conf.
What difference? I put some garbage into the config via vim's controlling
semantics... where 'difference' means prior to your changes someone would have to either edit or append libvirtd.conf in order to alter/define the name. With these changes the name (while running) is altered to some file which if it has unprintable characters in the name, how would it look?
Ugly. Still, they can put a name containing unprintable characters to the config and the daemon will create the file on the filesystem. After my changes, the same logic applies. IOW the user is responsible for the formatting of the filename in both cases.
sequences and when I used 'cat' on the config, what I got was an unescaped garbage, not an escaped version of what I put there, so in that aspect the behaviour is the same, user provides us with garbage, and in turn, he gets the same garbage back, there shouldn't be any compatibility or functioning issues connected to that.
In the long run, recent memory says whenever there's a user provided character string - the review comments have always been be sure to Escape it.
Searching for Escape in tools/*.c found a couple more "output" files.
I looked at those and as I said, those are XML escaping routines and shell escaping routines present mainly for reasons I mentioned above.
Fair enough - it's been considered... I think what I was going at there though was how virsh-snapshot handles output file names where the 'file=' or 'snapshot=' output is Escaped.
I understand, XMLs are special in this way and since since commit @aeb5262e we strip the control characters completely anyway so even our existing 'escaping' routine for XMLs isn't a proper one. Soo, did we reach a consensus? Erik
John

On 12/13/2016 11:43 AM, Erik Skultety wrote:
On Tue, Dec 13, 2016 at 09:50:49AM -0500, John Ferlan wrote:
On 12/13/2016 09:32 AM, Erik Skultety wrote:
On Mon, Dec 12, 2016 at 11:21:08AM -0500, John Ferlan wrote:
On 12/12/2016 10:42 AM, Erik Skultety wrote:
On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
On 11/25/2016 08:12 AM, Erik Skultety wrote: > Finally, now that all APIs have been introduced, wire them up to virt-admin > and introduce daemon-log-outputs and daemon-log-filters commands. > > Signed-off-by: Erik Skultety <eskultet@redhat.com> > --- > tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ > tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 210 insertions(+) >
Now it *is* the coffee - should there be any concerns along the way over escaping characters? I think this is the only place where we read/print, but via the API if someone passes a string - do we need to worry about that? Yes, I know - not the normal thing, but you know someone will try...
Is it an issue though? If someone sends a binary garbage for filename, we have to create a logfile with that name on the filesystem. When we retrieve it, I think we should print it as we created it - garbage. The other option we have is to strip all the control characters and print the rest which in the worst case scenario will end up as an empty string...
If someone provides garbage I thought our general practice was to escape the 'unprintable' chars when we Format the output.
In which case? Yes, we do it for XMLs (hence the function virBufferEscapeString) so that the resulting XML is compliant with the specification. We also do it for shell-related stuff like escaping the arguments that will end up on qemu cmdline. But what's the purpose of escaping in this case? Additionally, if you escape the output and let's say the user saved the output into a environment variable, he now has an escaped garbage which is further unusable when instead we should have gone with the verbatim, unprintable garbage. I played around with this in the morning and found out that bash allows you to use structure like this one: $'nnn' (where nnn is the string to be escaped), now that would be doable from our perspective, but then, I'm not sure whether such a construction is supported with other shells out there.
Escaping wasn't a requirement, just a concern. Hence the original question "should there be any concerns along the way over escaping characters? "
And by escaping I meant only the name of the file, although perhaps that wasn't clear as I re-read the original comment.
No, it was perfectly clear that you meant just the filename, but it doesn't contradict my point anyway.
The first thing that came to mind that might be close to what's being done here was domain interface script, where when we print out the name we Escape whatever is in script path.
Looking at the src/util/virtconf.c for VIR_CONF_STRING and virConfParseString would seem to imply to me the ability to read escaped characters.
The difference here is that you're now formatting the name; whereas, previously the formatting was entirely left to whomever edited /etc/libvirt/libvirtd.conf.
What difference? I put some garbage into the config via vim's controlling
semantics... where 'difference' means prior to your changes someone would have to either edit or append libvirtd.conf in order to alter/define the name. With these changes the name (while running) is altered to some file which if it has unprintable characters in the name, how would it look?
Ugly. Still, they can put a name containing unprintable characters to the config and the daemon will create the file on the filesystem. After my changes, the same logic applies. IOW the user is responsible for the formatting of the filename in both cases.
sequences and when I used 'cat' on the config, what I got was an unescaped garbage, not an escaped version of what I put there, so in that aspect the behaviour is the same, user provides us with garbage, and in turn, he gets the same garbage back, there shouldn't be any compatibility or functioning issues connected to that.
In the long run, recent memory says whenever there's a user provided character string - the review comments have always been be sure to Escape it.
Searching for Escape in tools/*.c found a couple more "output" files.
I looked at those and as I said, those are XML escaping routines and shell escaping routines present mainly for reasons I mentioned above.
Fair enough - it's been considered... I think what I was going at there though was how virsh-snapshot handles output file names where the 'file=' or 'snapshot=' output is Escaped.
I understand, XMLs are special in this way and since since commit @aeb5262e we strip the control characters completely anyway so even our existing 'escaping' routine for XMLs isn't a proper one.
Soo, did we reach a consensus?
Erik
The only thing holding up this patch would be your update to tools/virt-admin.pod to describe the name of the command (there was no daemon-log-info). Like I said the escaping was only a question of whether it was necessary - I think you've answered that. John

Provide a simple C example demonstrating the use of both query APIs as well as setter APIs. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitignore | 1 + examples/Makefile.am | 3 +- examples/admin/logging.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 examples/admin/logging.c diff --git a/.gitignore b/.gitignore index 879ec24..984ad07 100644 --- a/.gitignore +++ b/.gitignore @@ -79,6 +79,7 @@ /examples/admin/client_limits /examples/admin/list_clients /examples/admin/list_servers +/examples/admin/logging /examples/admin/threadpool_params /examples/object-events/event-test /examples/dominfo/info1 diff --git a/examples/Makefile.am b/examples/Makefile.am index 7cb8258..2956e14 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -43,7 +43,7 @@ noinst_PROGRAMS=dominfo/info1 dommigrate/dommigrate domsuspend/suspend \ domtop/domtop hellolibvirt/hellolibvirt object-events/event-test \ openauth/openauth rename/rename admin/list_servers admin/list_clients \ admin/threadpool_params admin/client_limits admin/client_info \ - admin/client_close + admin/client_close admin/logging dominfo_info1_SOURCES = dominfo/info1.c dommigrate_dommigrate_SOURCES = dommigrate/dommigrate.c @@ -65,6 +65,7 @@ admin_threadpool_params_SOURCES = admin/threadpool_params.c admin_client_limits_SOURCES = admin/client_limits.c admin_client_info_SOURCES = admin/client_info.c admin_client_close_SOURCES = admin/client_close.c +admin_logging_SOURCES = admin/logging.c if WITH_APPARMOR_PROFILES apparmordir = $(sysconfdir)/apparmor.d/ diff --git a/examples/admin/logging.c b/examples/admin/logging.c new file mode 100644 index 0000000..e28c7d5 --- /dev/null +++ b/examples/admin/logging.c @@ -0,0 +1,102 @@ +#include<stdio.h> +#include<stdlib.h> +#include<stdbool.h> + +#include "config.h" +#include<unistd.h> +#include<libvirt/libvirt-admin.h> +#include<libvirt/virterror.h> + +static void printHelp(const char *argv0) +{ + fprintf(stderr, + ("Usage:\n" + " %s [options]\n" + "\n" + "Options:\n" + " -h Print this message.\n" + " -o [string] Specify new log outputs.\n" + " -f [string] Specify new log filters.\n" + "\n"), + argv0); +} + +int main(int argc, char **argv) +{ + int ret, c; + virAdmConnectPtr conn = NULL; + char *get_outputs = NULL; + char *get_filters = NULL; + const char *set_outputs = NULL; + const char *set_filters = NULL; + + ret = c = -1; + opterr = 0; + + while ((c = getopt(argc, argv, ":hpo:f:")) > 0) { + switch (c) { + case 'h': + printHelp(argv[0]); + exit(EXIT_SUCCESS); + case 'o': + set_outputs = optarg; + break; + case 'f': + set_filters = optarg; + break; + case ':': + fprintf(stderr, "Missing argument for option -%c\n", optopt); + exit(EXIT_FAILURE); + case '?': + fprintf(stderr, "Unrecognized option '-%c'\n", optopt); + exit(EXIT_FAILURE); + } + } + + /* first, open a connection to the daemon */ + if (!(conn = virAdmConnectOpen(NULL, 0))) + goto cleanup; + + /* get the currently defined log outputs and filters */ + if (virAdmConnectGetLoggingOutputs(conn, &get_outputs, 0) < 0 || + virAdmConnectGetLoggingFilters(conn, &get_filters, 0) < 0) + goto cleanup; + + fprintf(stdout, + "Current settings:\n" + " outputs: %s\n" + " filters: %s\n" + "\n", + get_outputs, get_filters ? get_filters : "None"); + + free(get_outputs); + free(get_filters); + + /* now, try to change the redefine the current log output and filters */ + if (virAdmConnectSetLoggingOutputs(conn, set_outputs, 0) < 0) + goto cleanup; + + if (virAdmConnectSetLoggingFilters(conn, set_filters, 0) < 0) + goto cleanup; + + /* get the currently defined log outputs and filters */ + if (virAdmConnectGetLoggingOutputs(conn, &get_outputs, 0) < 0 || + virAdmConnectGetLoggingFilters(conn, &get_filters, 0) < 0) + goto cleanup; + + fprintf(stdout, + "New settings:\n" + " outputs: %s\n" + " filters: %s\n" + "\n", + get_outputs ? get_outputs : "Default", + get_filters ? get_filters : "None"); + + free(get_outputs); + free(get_filters); + + ret = 0; + cleanup: + virAdmConnectClose(conn); + return ret; +} -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Provide a simple C example demonstrating the use of both query APIs as well as setter APIs.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .gitignore | 1 + examples/Makefile.am | 3 +- examples/admin/logging.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 examples/admin/logging.c
diff --git a/.gitignore b/.gitignore index 879ec24..984ad07 100644 --- a/.gitignore +++ b/.gitignore @@ -79,6 +79,7 @@ /examples/admin/client_limits /examples/admin/list_clients /examples/admin/list_servers +/examples/admin/logging /examples/admin/threadpool_params /examples/object-events/event-test /examples/dominfo/info1 diff --git a/examples/Makefile.am b/examples/Makefile.am index 7cb8258..2956e14 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -43,7 +43,7 @@ noinst_PROGRAMS=dominfo/info1 dommigrate/dommigrate domsuspend/suspend \ domtop/domtop hellolibvirt/hellolibvirt object-events/event-test \ openauth/openauth rename/rename admin/list_servers admin/list_clients \ admin/threadpool_params admin/client_limits admin/client_info \ - admin/client_close + admin/client_close admin/logging
dominfo_info1_SOURCES = dominfo/info1.c dommigrate_dommigrate_SOURCES = dommigrate/dommigrate.c @@ -65,6 +65,7 @@ admin_threadpool_params_SOURCES = admin/threadpool_params.c admin_client_limits_SOURCES = admin/client_limits.c admin_client_info_SOURCES = admin/client_info.c admin_client_close_SOURCES = admin/client_close.c +admin_logging_SOURCES = admin/logging.c
if WITH_APPARMOR_PROFILES apparmordir = $(sysconfdir)/apparmor.d/ diff --git a/examples/admin/logging.c b/examples/admin/logging.c new file mode 100644 index 0000000..e28c7d5 --- /dev/null +++ b/examples/admin/logging.c @@ -0,0 +1,102 @@ +#include<stdio.h> +#include<stdlib.h> +#include<stdbool.h> + +#include "config.h" +#include<unistd.h> +#include<libvirt/libvirt-admin.h> +#include<libvirt/virterror.h> + +static void printHelp(const char *argv0) +{ + fprintf(stderr, + ("Usage:\n" + " %s [options]\n" + "\n" + "Options:\n" + " -h Print this message.\n" + " -o [string] Specify new log outputs.\n" + " -f [string] Specify new log filters.\n" + "\n"), + argv0); +} + +int main(int argc, char **argv) +{ + int ret, c; + virAdmConnectPtr conn = NULL; + char *get_outputs = NULL; + char *get_filters = NULL; + const char *set_outputs = NULL; + const char *set_filters = NULL; + + ret = c = -1; + opterr = 0; + + while ((c = getopt(argc, argv, ":hpo:f:")) > 0) { + switch (c) { + case 'h': + printHelp(argv[0]); + exit(EXIT_SUCCESS); + case 'o': + set_outputs = optarg; + break; + case 'f': + set_filters = optarg; + break; + case ':': + fprintf(stderr, "Missing argument for option -%c\n", optopt); + exit(EXIT_FAILURE); + case '?': + fprintf(stderr, "Unrecognized option '-%c'\n", optopt); + exit(EXIT_FAILURE); + } + } + + /* first, open a connection to the daemon */ + if (!(conn = virAdmConnectOpen(NULL, 0))) + goto cleanup; + + /* get the currently defined log outputs and filters */ + if (virAdmConnectGetLoggingOutputs(conn, &get_outputs, 0) < 0 || + virAdmConnectGetLoggingFilters(conn, &get_filters, 0) < 0) + goto cleanup; + + fprintf(stdout, + "Current settings:\n" + " outputs: %s\n" + " filters: %s\n" + "\n", + get_outputs, get_filters ? get_filters : "None"); + + free(get_outputs); + free(get_filters);
If run without any arguments, we should just stop here. e.g. if set_outputs and set_filters are both NULL, just jump to the ret = 0; ACK - John
+ + /* now, try to change the redefine the current log output and filters */ + if (virAdmConnectSetLoggingOutputs(conn, set_outputs, 0) < 0) + goto cleanup; + + if (virAdmConnectSetLoggingFilters(conn, set_filters, 0) < 0) + goto cleanup; + + /* get the currently defined log outputs and filters */ + if (virAdmConnectGetLoggingOutputs(conn, &get_outputs, 0) < 0 || + virAdmConnectGetLoggingFilters(conn, &get_filters, 0) < 0) + goto cleanup; + + fprintf(stdout, + "New settings:\n" + " outputs: %s\n" + " filters: %s\n" + "\n", + get_outputs ? get_outputs : "Default", + get_filters ? get_filters : "None"); + + free(get_outputs); + free(get_filters); + + ret = 0; + cleanup: + virAdmConnectClose(conn); + return ret; +}

Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/news.html.in | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/news.html.in b/docs/news.html.in index 59bf20d..f2a65f5 100644 --- a/docs/news.html.in +++ b/docs/news.html.in @@ -31,6 +31,10 @@ Add the capability to pass through a scsi_host HBA and the associated LUNs to the guest. </li> + <li>admin-logging: Add support for runtime logging settings adjustment<br/> + Logging-related settings like log outputs and filters can now be + adjusted during runtime without the necessity of the daemon's restart. + </li> </ul> </li> <li><strong>Improvements</strong> -- 2.5.5

On 11/25/2016 08:12 AM, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/news.html.in | 4 ++++ 1 file changed, 4 insertions(+)
Andrea will be thrilled, but of course it does have to move to the 3.0.0 section, but I would say that's obvious - still wouldn't want you to forget!
diff --git a/docs/news.html.in b/docs/news.html.in index 59bf20d..f2a65f5 100644 --- a/docs/news.html.in +++ b/docs/news.html.in @@ -31,6 +31,10 @@ Add the capability to pass through a scsi_host HBA and the associated LUNs to the guest. </li> + <li>admin-logging: Add support for runtime logging settings adjustment<br/> + Logging-related settings like log outputs and filters can now be + adjusted during runtime without the necessity of the daemon's restart.
Seems adding the stop (.) isn't desired (not my choice), so just be sure to remove that too when moving to 3.0.0 section Should the sentence start "daemon logging"? IDC either way. ACK w/ adjustments John
+ </li> </ul> </li> <li><strong>Improvements</strong>

On Fri, Nov 25, 2016 at 02:11:58PM +0100, Erik Skultety wrote:
v1 here https://www.redhat.com/archives/libvir-list/2016-November/msg00009.html
since v1: - incorporated notes raised during review - allowed passing of NULL via the public APIs * behaves the same way as an empty string, but lead to even more code reduction in 'daemonSetupLogging' for practically no extra code to enable such feature - added forgotten documentation - updated NEWS file
So since there weren't further objections/notes/advices, I addressed the necessary adjustments as requested in the review and pushed the series. Thanks for review. Erik
participants (2)
-
Erik Skultety
-
John Ferlan