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@xxxxxxxxxx>
> ---
> 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;
> }
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list