On 11/01/2016 06:27 AM, Erik Skultety wrote:
> The reason why we need something like this lies in the daemon's config where we
> treat the @log_outputs variable (but not just this one) the very same way in
> cases where the variable was explicitly set to an empty string or wasn't set at
> all, using some default output in both. The selection of a default output
> depends on whether the daemon runs daemonized or not. Before the runtime
> logging APIs can be enabled, we need to make sure that the behaviour will be the
> same in case someone tries to replace the set of logging outputs with an empty
> string, hoping that it would turn the logging off.
> In order to be able to reset the logging output to some default we either need
> to store the daemon mode or we store a default logging output which we'll be
> able to fallback to later. This patch goes for the latter by introducing new
> methods to set and retrieve the default logging output.
The commit message feels like a continuation of the cover and
justification for a static virLogDefaultOutput.
The shortened version is - introduce new helpers to handle managing
output log file defaults and save/fetch of a default log location. This
patch will store the default
All these changes should be usable by lib{virtd|logd|lockd}... Although
I didn't quite dig into the details of those...
>
> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 ++
> src/util/virlog.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virlog.h | 2 ++
> 3 files changed, 98 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 162fda5..5b0e07d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1877,6 +1877,7 @@ virLogFilterFree;
> virLogFilterListFree;
> virLogFilterNew;
> virLogFindOutput;
> +virLogGetDefaultOutput;
> virLogGetDefaultPriority;
> virLogGetFilters;
> virLogGetNbFilters;
> @@ -1895,6 +1896,7 @@ virLogParseOutputs;
> virLogPriorityFromSyslog;
> virLogProbablyLogMessage;
> virLogReset;
> +virLogSetDefaultOutput;
> virLogSetDefaultPriority;
> virLogSetFilters;
> virLogSetFromEnv;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 8f831fc..4ac72dc 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;
After reading through to the end, I could see use for a
virLogDefaultFilter too... But that's a different problem. Focus,
focus, focus on the current one ;-)!
> static virLogOutputPtr *virLogOutputs;
> static size_t virLogNbOutputs;
>
> @@ -146,6 +148,98 @@ virLogUnlock(void)
> virMutexUnlock(&virLogMutex);
> }
>
Two spaces between functions (more than one occurrence)
> +static int
> +virLogSetDefaultOutputToStderr(void)
> +{
> + char *tmp = NULL;
> + if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
> + return -1;
> +
> + virLogDefaultOutput = tmp;
> + return 0;
Or more simply
return virAsprintf(&virLogDefaultOutput, ...);
of course on error virLogDefaultOutput = NULL;, which shouldn't matter
since this is only ever being done once
Also since virLogDefaultPriority is owned here, do you really need the
virLogGetDefaultPriority() helper?
> +}
> +
> +static int
> +virLogSetDefaultOutputToJournald(void)
> +{
> + char *tmp = NULL;
> + virLogPriority priority = virLogDefaultPriority;
here you went directly to the local 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;
> +
> + if (virAsprintf(&tmp, "%d:journald", priority) < 0)
> + return -1;
> +
> + virLogDefaultOutput = tmp;
> + return 0;
similar here return virAsprintf(&virLogDefaultOutput, ...);
> +}
> +
> +static int
> +virLogSetDefaultOutputToFile(bool privileged)
If this took a param "char const *fname, then the "libvirtd.log" could
be replaced with %s using fname. Thus perhaps being reusable for
vir{Lock|Log}DaemonSetupLogging.
> +{
> + int ret = -1;
> + char *tmp = NULL;
> + char *logdir = NULL;
> +
> + if (privileged) {
> + if (virAsprintf(&tmp, "%d:file:%s/log/libvirt/libvirtd.log",
> + virLogGetDefaultPriority(),
Again use virLogDefaultPriority
and "libvirt.log" gets replaced by %s with the 3rd param being fname
> + LOCALSTATEDIR) < 0)
> + goto cleanup;
> + } else {
> + if (!(logdir = virGetUserCacheDirectory()))
> + goto cleanup;
> +
> + mode_t old_umask = umask(077);
Promote 'mode_t old_umask;' to the top too
> + if (virFileMakePath(logdir) < 0) {
> + umask(old_umask);
> + goto cleanup;
> + }
> + umask(old_umask);
> +
> + if (virAsprintf(&tmp, "%d:file:%s/libvirtd.log",
> + virLogGetDefaultPriority(), logdir) < 0)
Use virLogDefaultPriority - likewise with 3rd param
> + goto cleanup;
> + }
> +
> + virLogDefaultOutput = tmp;
> + tmp = NULL;
similar in here w/r/t virAsprintf(&virLogDefaultOutput, ...)
> + ret = 0;
> + cleanup:
> + VIR_FREE(tmp);
> + VIR_FREE(logdir);
> + return ret;
> +}
> +
> +/* this should be run exactly once at daemon startup, so no locking is
> + * necessary
> + */
Add some comments regarding input/output, actions, etc. In fact, I
think if you grab the comments (more or less) from existing (and
duplicated) code/comments from lib{virtd|logd|lockd} and move it here,
massage it to fit the reality you're creating, then that would be fine.
> +int
> +virLogSetDefaultOutput(virLogDestination dest, bool privileged)
This could just be how daemonSetupLoggingDefaults gets changed by your
patch 3 (although I've kept isatty - reason in patch 3)
Thus you'd have:
virLogSetDefaultOutput(const char *fname,
bool godaemon,
bool privileged)
{
if (!godaemon)
return virLogSetDefaultOutputToStderr(privileged);
if (!isatty(STDIN_FILENO) &&
access("/run/systemd/journal/socket", W_OK) >= 0))
return virLogSetDefaultOutputToJournald(privileged);
return virLogSetDefaultOutputToFile(fname, privileged);
}
> +{
> + switch (dest) {
> + case VIR_LOG_TO_STDERR:
> + return virLogSetDefaultOutputToStderr();
> + case VIR_LOG_TO_JOURNALD:
> + return virLogSetDefaultOutputToJournald();
> + case VIR_LOG_TO_FILE:
> + return virLogSetDefaultOutputToFile(privileged);
> + case VIR_LOG_TO_SYSLOG:
> + case VIR_LOG_TO_OUTPUT_LAST:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +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..d6eb693 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(virLogDestination dest, bool privileged);
>
> /*
> * Internal logging API
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list