Commit 94c465d0 refactored the logging setup phase but introduced an
issue, where the daemon ignores verbose mode when there are no outputs
defined and the default must be used. The problem is that the default
output was determined too early, thus ignoring the potential '--verbose'
option taking effect. This patch postpones the creation of the default
output to the very last moment when nothing else can change. Since the
default output is only created during the init phase, it's safe to leave
the pointer as NULL for a while, but it will be set eventually, thus not
affecting runtime.
Patch also adjusts both the other daemons.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1457193

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 daemon/libvirtd.c         | 21 +++++++++++++--------
 src/locking/lock_daemon.c | 21 +++++++++++++--------
 src/logging/log_daemon.c  | 21 +++++++++++++--------
 src/util/virlog.c         |  7 +++++++
 4 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 7e5d7af69..589b32192 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -615,19 +615,15 @@ daemonSetupLogging(struct daemonConfig *config,
      * Libvirtd's order of precedence is:
      * cmdline > environment > config
      *
-     * The default output is applied only if there was no setting from either
-     * the config or the environment. Because we don't have a way to determine
-     * if the log level has been set, we must process variables in the opposite
+     * Given the precedence, we must process the variables in the opposite
      * order, each one overriding the previous.
      */
     if (config->log_level != 0)
         virLogSetDefaultPriority(config->log_level);
 
-    if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0)
-        return -1;
-
-    /* In case the config is empty, the filters become empty and outputs will
-     * be set to default
+    /* In case the config is empty, both filters and outputs will become empty,
+     * however we can't start with empty outputs, thus we'll need to define and
+     * setup a default one.
      */
     ignore_value(virLogSetFilters(config->log_filters));
     ignore_value(virLogSetOutputs(config->log_outputs));
@@ -641,6 +637,15 @@ daemonSetupLogging(struct daemonConfig *config,
     if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
         virLogSetDefaultPriority(VIR_LOG_INFO);
 
+    /* Define the default output. This is only applied if there was no setting
+     * from either the config or the environment.
+     */
+    if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0)
+        return -1;
+
+    if (virLogGetNbOutputs() == 0)
+        virLogSetOutputs(virLogGetDefaultOutput());
+
     return 0;
 }
 
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 12485e966..6fbbf4b3d 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -460,19 +460,15 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
      * Libvirtd's order of precedence is:
      * cmdline > environment > config
      *
-     * The default output is applied only if there was no setting from either
-     * the config or the environment. Because we don't have a way to determine
-     * if the log level has been set, we must process variables in the opposite
+     * Given the precedence, we must process the variables in the opposite
      * order, each one overriding the previous.
      */
     if (config->log_level != 0)
         virLogSetDefaultPriority(config->log_level);
 
-    if (virLogSetDefaultOutput("virtlockd.log", godaemon, privileged) < 0)
-        return -1;
-
-    /* In case the config is empty, the filters become empty and outputs will
-     * be set to default
+    /* In case the config is empty, both filters and outputs will become empty,
+     * however we can't start with empty outputs, thus we'll need to define and
+     * setup a default one.
      */
     ignore_value(virLogSetFilters(config->log_filters));
     ignore_value(virLogSetOutputs(config->log_outputs));
@@ -486,6 +482,15 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
     if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
         virLogSetDefaultPriority(VIR_LOG_INFO);
 
+    /* Define the default output. This is only applied if there was no setting
+     * from either the config or the environment.
+     */
+    if (virLogSetDefaultOutput("virtlockd.log", godaemon, privileged) < 0)
+        return -1;
+
+    if (virLogGetNbOutputs() == 0)
+        virLogSetOutputs(virLogGetDefaultOutput());
+
     return 0;
 }
 
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index d878efa63..5a136c59d 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -388,19 +388,15 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
      * Libvirtd's order of precedence is:
      * cmdline > environment > config
      *
-     * The default output is applied only if there was no setting from either
-     * the config or the environment. Because we don't have a way to determine
-     * if the log level has been set, we must process variables in the opposite
+     * Given the precedence, we must process the variables in the opposite
      * order, each one overriding the previous.
      */
     if (config->log_level != 0)
         virLogSetDefaultPriority(config->log_level);
 
-    if (virLogSetDefaultOutput("virtlogd.log", godaemon, privileged) < 0)
-        return -1;
-
-    /* In case the config is empty, the filters become empty and outputs will
-     * be set to default
+    /* In case the config is empty, both filters and outputs will become empty,
+     * however we can't start with empty outputs, thus we'll need to define and
+     * setup a default one.
      */
     ignore_value(virLogSetFilters(config->log_filters));
     ignore_value(virLogSetOutputs(config->log_outputs));
@@ -414,6 +410,15 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
     if ((verbose) && (virLogGetDefaultPriority() > VIR_LOG_INFO))
         virLogSetDefaultPriority(VIR_LOG_INFO);
 
+    /* Define the default output. This is only applied if there was no setting
+     * from either the config or the environment.
+     */
+    if (virLogSetDefaultOutput("virtlogd.log", godaemon, privileged) < 0)
+        return -1;
+
+    if (virLogGetNbOutputs() == 0)
+        virLogSetOutputs(virLogGetDefaultOutput());
+
     return 0;
 }
 
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 2228cf645..d45a451a7 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1840,6 +1840,13 @@ virLogSetOutputs(const char *src)
     if (src && *src)
         outputstr = src;
 
+    /* This can only happen during daemon init when the default output is not
+     * determined yet. It's safe to do, since it's the only place setting the
+     * default output.
+     */
+    if (!outputstr)
+        return 0;
+
     if ((noutputs = virLogParseOutputs(outputstr, &outputs)) < 0)
         goto cleanup;
 
-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Aug 28, 2017 at 10:39:15AM +0200, Erik Skultety wrote:
>Commit 94c465d0 refactored the logging setup phase but introduced an
>issue, where the daemon ignores verbose mode when there are no outputs
>defined and the default must be used. The problem is that the default
>output was determined too early, thus ignoring the potential '--verbose'
>option taking effect. This patch postpones the creation of the default
>output to the very last moment when nothing else can change. Since the
>default output is only created during the init phase, it's safe to leave
>the pointer as NULL for a while, but it will be set eventually, thus not
>affecting runtime.
>Patch also adjusts both the other daemons.
>

Maybe we need a daemon template and a perl script to generate the code
to avoid this kind of duplication?

>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1457193
>
>Signed-off-by: Erik Skultety <eskultet@redhat.com>
>---
> daemon/libvirtd.c         | 21 +++++++++++++--------
> src/locking/lock_daemon.c | 21 +++++++++++++--------
> src/logging/log_daemon.c  | 21 +++++++++++++--------
> src/util/virlog.c         |  7 +++++++
> 4 files changed, 46 insertions(+), 24 deletions(-)
>

ACK

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Aug 28, 2017 at 03:32:31PM +0200, Ján Tomko wrote:
> On Mon, Aug 28, 2017 at 10:39:15AM +0200, Erik Skultety wrote:
> > Commit 94c465d0 refactored the logging setup phase but introduced an
> > issue, where the daemon ignores verbose mode when there are no outputs
> > defined and the default must be used. The problem is that the default
> > output was determined too early, thus ignoring the potential '--verbose'
> > option taking effect. This patch postpones the creation of the default
> > output to the very last moment when nothing else can change. Since the
> > default output is only created during the init phase, it's safe to leave
> > the pointer as NULL for a while, but it will be set eventually, thus not
> > affecting runtime.
> > Patch also adjusts both the other daemons.
> >
>
> Maybe we need a daemon template and a perl script to generate the code
> to avoid this kind of duplication?

Maybe, are you openly volunteering? If so, have fun :).

>
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1457193
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> > daemon/libvirtd.c         | 21 +++++++++++++--------
> > src/locking/lock_daemon.c | 21 +++++++++++++--------
> > src/logging/log_daemon.c  | 21 +++++++++++++--------
> > src/util/virlog.c         |  7 +++++++
> > 4 files changed, 46 insertions(+), 24 deletions(-)
> >
>
> ACK

Thanks,

Erik

>
> Jan


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Aug 28, 2017 at 03:32:31PM +0200, Ján Tomko wrote:
> On Mon, Aug 28, 2017 at 10:39:15AM +0200, Erik Skultety wrote:
> > Commit 94c465d0 refactored the logging setup phase but introduced an
> > issue, where the daemon ignores verbose mode when there are no outputs
> > defined and the default must be used. The problem is that the default
> > output was determined too early, thus ignoring the potential '--verbose'
> > option taking effect. This patch postpones the creation of the default
> > output to the very last moment when nothing else can change. Since the
> > default output is only created during the init phase, it's safe to leave
> > the pointer as NULL for a while, but it will be set eventually, thus not
> > affecting runtime.
> > Patch also adjusts both the other daemons.
> >
>
> Maybe we need a daemon template and a perl script to generate the code
> to avoid this kind of duplication?
>
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1457193
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> > daemon/libvirtd.c         | 21 +++++++++++++--------
> > src/locking/lock_daemon.c | 21 +++++++++++++--------
> > src/logging/log_daemon.c  | 21 +++++++++++++--------
> > src/util/virlog.c         |  7 +++++++
> > 4 files changed, 46 insertions(+), 24 deletions(-)
> >
>
> ACK

I switched the BZ to https://bugzilla.redhat.com/show_bug.cgi?id=1442947
because I'd closed the original as duplicate of this one, solely because this
one had been filed earlier. Anyhow, the patch is now pushed.

Thanks,
Erik

>
> Jan


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list