On Wed, Nov 09, 2016 at 11:22:34AM -0500, John Ferlan wrote:
>
>
> 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...
>
Sigh, I had it on my mind and then the very next day - puff - I completely
forgot about it compiled the patches one last time and sent patches right away
:/
> >
> > 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 ;-)!
>
Well, I'm only doing this because we have to stay consistent with the config
file. The defaults for filters on the other hand do not support any defaults,
you don't set any, you don't have any. I can imagine we could have the same for
the filters as you suggest if libvirt supported something like 'save as default'
which then would make sense for both. But since the outputs defaults are
hard-coded, the difference in the interpretation of the default in both cases
could end up IMHO slightly confusing, but maybe I'm looking at it from the wrong
angle.
> > 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
Well I can rewrite it, no problem whatsoever, I just looked at it as if I wrote
any other function, disregarding the fact that it's going to be called once and
once only completely and instead treated it like it's going to be called
regularly and so we should not touch caller's reference if we're not 100% sure
nothing can wrong anymore.
>
> Also since virLogDefaultPriority is owned here, do you really need the
> virLogGetDefaultPriority() helper?
>
Yeah, I certainly don't, true that :).
> > +}
> > +
> > +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);
> }
>
I was looking at this^^ for quite some time thinking what I'd missed, why hadn't
I written it this elegantly and then I realized that unfortunately it is not
equivalent to how the daemon behaves now. With your change, the fact STDIN is a
tty is only considered when we know we're running as daemon, which isn't the
case with the current behaviour where we consider it in the opposite case when
we're not running as a daemon but we don't have a tty (some other process
spawned us and passed the fds). So eventually, with your proposal you'd go for
stderr every time we're not running as daemon which is essentially what I'm
proposing, we should really only consider @godaemon, because if someone decides
to pass some exotic FDs, it shouldn't really be our problem, the calling
process should handle extracting our logging from the FD it passed to us.
I also spent a fair amount of time figuring out why commit @eba36a38 didn't
consider just @godaemon and I have to say I wasn't successful at all.
Erik
> > +{
> > + 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
> >
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list