[libvirt] [PATCH 2/8] virlog: Introduce virLog{Get, Set}DefaultOutput

[John Ferlan]

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 at 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
>