10:22 a.m.
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(a)redhat.com>
---
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