[libvirt] [PATCH 3/8] daemon: Hook up the virLog{Get, Set}DefaultOutput to the daemon's init routine

[John Ferlan]

On 11/01/2016 06:27 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. As part of this process,
> refactor the daemonSetupLoggingDefaults method, since the code isn't
> particularly easy to read (due to the condition below). However, this refactor
> changes the logic of the selection of the default logging output in the way
> demonstrated below. Long story short, we should really only care if we're
> running daemonized or not, disregarding the fact of (not) having a TTY
> completely as that should be of the libvirtd's parent concern (what FD it will
> pass to it).

See commit id 'eba36a3878' regarding !isatty(STDIN_FILENO) - it's not
100% clear to me whether we could just remove it. I think we should keep 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 at redhat.com>
> ---
>  daemon/libvirtd.c | 86 ++++++++++++++-----------------------------------------
>  1 file changed, 21 insertions(+), 65 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 9a5f193..3af4ea1 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c

Since you added configmake.h to virlog.c in patch 2 and remove what was
necessary for here, it's no longer needed here...

> @@ -660,65 +660,24 @@ daemonSetupNetworking(virNetServerPtr srv,
>  static int
>  daemonSetupLoggingDefaults(bool godaemon, bool privileged)
>  {

This ends up not being needed

> -    if (virLogGetOutputs() == 0 &&
> -        (godaemon || !isatty(STDIN_FILENO))) {


> -        char *tmp;
> +    /* If we're running as a daemon, the try to direct the output to systemd
> +     * journal first (if it exists), otherwise fallback to libvirtd.log. If not
> +     * running as a daemon, use stderr as default.
> +     */
> +    if (!godaemon) {
> +        if (virLogSetDefaultOutput(VIR_LOG_TO_STDERR, privileged) < 0)
> +            return -1;
> +    } else {
>          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);
> -        }
> -    }
> -
> -    if (virLogGetOutputs() == 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);
> -            }
> +            if (virLogSetDefaultOutput(VIR_LOG_TO_JOURNALD, privileged) < 0)
> +                return -1;
>          } else {
> -            if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
> -                goto error;
> +            if (virLogSetDefaultOutput(VIR_LOG_TO_FILE, privileged) < 0)
> +                return -1;
>          }
> -        virLogSetOutputs(tmp);
> -        VIR_FREE(tmp);
>      }
>  
>      return 0;
> - error:
> -    return -1;
>  }
>  
>  /*
> @@ -733,6 +692,9 @@ daemonSetupLogging(struct daemonConfig *config,
>                     bool verbose,
>                     bool godaemon)
>  {
> +    if (daemonSetupLoggingDefaults(godaemon, privileged) < 0)
> +        return -1;
> +

s/(godaemon/("libvirtd.log", godaemon/

Although I don't think this is the exact right place... I think it
should be moved to just before the virLogSetFromEnv().

Also I think the command line override for --verbose should be moved up
as well before the call to set the log defaults... Thus it'd be:

    if (config->log_level != 0)
        virLogSetDefaultPriority(config->log_level);

    /*
     * Command line override for --verbose
     */
    if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
        virLogSetDefaultPriority(VIR_LOG_INFO);

    if (daemonSetupLoggingDefaults("libvirtd.log", godaemon,
                                   privileged) < 0)
        return -1;

This would be repeated for libvirtd.c, lock_daemon.c, and log_daemon.c

>      virLogReset();
>  
>      /*
> @@ -767,20 +729,14 @@ daemonSetupLogging(struct daemonConfig *config,
>          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), otherwise fallback to libvirtd.log. If both not running
> -     * as daemon and having a tty, use stderr as default.
> -     */
> -    if (virLogGetNbOutputs() == 0 &&
> -        daemonSetupLoggingDefaults(godaemon, privileged) < 0)
> -        goto error;
> +     * If there are no outputs defined, use the default one */
> +    if (!virLogGetNbOutputs()) {

I'd rather see "if (virLogGetNbOutputs() == 0)"

Save the ! for NULL ..

> +        char *tmp = virLogGetDefaultOutput();
> +        virLogSetOutputs(tmp);

just go direct:

virLogSetOutputs(virLogGetDefaultOutput())


John
> +        VIR_FREE(tmp);
> +    }
>  
>      return 0;
> -
> - error:
> -    return -1;
>  }
>  
>  
>