[libvirt] [PATCH 1/8] daemon: Introduce daemonSetLoggingDefaults

[John Ferlan]

On 11/01/2016 06:27 AM, Erik Skultety wrote:
> This patch moves the code responsible for setting up logging defaults to a
> separate function to enhance the readability a bit more. This code movement
> is also meant as a preparation phase for a future refactor of the affected
> hunks.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  daemon/libvirtd.c | 127 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 68 insertions(+), 59 deletions(-)
> 

Changes in patch 2 resolve a couple of things I noticed below regarding
leaked 'logdir' (if MakeFilePath fails) and usage of == -1 rather than <
0 for virAsprintf calls.

While I understand it's "outside" the scope of what you're trying to do
by adding new admin commands; however, lock_daemon and log_daemon have a
lot of "similarities" that I think should use the same code - IOW: I
think it's worthwhile to adjust them all.

After doing some thinking perhaps patch2 should become patch1 with one
adjustment - see my comments there. Then patch2 would adjust libvirtd,
lock_daemon, and log_daemon to call virLogSetDefaultOutput as done in
patch3 (passing fname of libvirtd.log, virtlogd.log, or virtlockd.log)

> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index cd25b50..9a5f193 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -657,6 +657,70 @@ daemonSetupNetworking(virNetServerPtr srv,
>  }
>  
>  
> +static int
> +daemonSetupLoggingDefaults(bool godaemon, bool privileged)

See my comments in patch 2 w/r/t virLogSetDefaultOutput

> +{
> +    if (virLogGetOutputs() == 0 &&

FWIW:
virLogGetOutputs returns char*...  This used to be a virLogGetNbOutputs
call too...

In any case, "currently" the only way into this helper is "if
virLogGetNbOutputs() == 0", so in a way one could say this check is
unnecessary...

> +        (godaemon || !isatty(STDIN_FILENO))) {
> +        char *tmp;
> +        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);

Interesting... If this is successful, that means the next hunk doesn't
need to be run... thus we could have just returned 0 here.

> +        }
> +    }
> +
> +    if (virLogGetOutputs() == 0) {

If my assumption above is true, then no need to get the number of
outputs...  although now moot with my other suggestions...

> +        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);
> +            }
> +        } else {
> +            if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
> +                goto error;
> +        }
> +        virLogSetOutputs(tmp);
> +        VIR_FREE(tmp);
> +    }
> +
> +    return 0;
> + error:
> +    return -1;
> +}
> +
>  /*
>   * Set up the logging environment
>   * By default if daemonized all errors go to the logfile libvirtd.log,
> @@ -706,67 +770,12 @@ daemonSetupLogging(struct daemonConfig *config,
>       * 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)....
> +     * (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 &&
> -        (godaemon || !isatty(STDIN_FILENO))) {
> -        char *tmp;
> -        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);
> -        }
> -    }
> -
> -    /*
> -     * otherwise direct to libvirtd.log when running
> -     * as daemon. Otherwise the default output is stderr.
> -     */
> -    if (virLogGetNbOutputs() == 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);
> -            }
> -        } else {
> -            if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
> -                goto error;
> -        }
> -        virLogSetOutputs(tmp);
> -        VIR_FREE(tmp);
> -    }

[1] Although moot w/ my latest suggestion, there's a hunk of comments
just before this that should have been moved prior to the new function
as they make no sense here any more and they get deleted in patch 3.
These comments (in some form) would be usable w/ my new suggestion for
how virLogSetDefaultOutput could be implemented. The one thing to watch
for is changing the 'libvirtd.log' in the comment to @fname (you'll note
virtlogd.c and virtlockd.c has essentially copied the comments without
changing the file name in the comment ironically).

> +        daemonSetupLoggingDefaults(godaemon, privileged) < 0)
> +        goto error;

Although moot now, but

s/goto error/return -1/

>  
>      return 0;
>  
> 

and remove error: here obviously.


John