util: Check for errors in virLogSetFromEnv

And make callers check the return value as well.  This helps error out early for
invalid environment variables.

That is desirable because it could lead to deadlocks.  This can happen when
resetting logging after fork() reports translated errors because gettext
functions are not reentrant.  Well, it is not limited to resetting logging after
fork(), it can be any translation at that phase, but parsing environment
variables is easy to make fail on purpose to show the result, it can also happen
just due to a typo.

Before this commit it is possible to deadlock the daemon on startup
with something like:

LIBVIRT_LOG_FILTERS='1:*' LIBVIRT_LOG_OUTPUTS=1:stdout libvirtd

where filters are used to enable more logging and hence make the race less rare
and outputs are set to invalid

Combined with the previous patches this changes
the following from:

...
<deadlock>

to:

...
libvirtd: initialisation failed

The error message is improved in future commits and is also possible thanks to
this patch.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>

diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index a3164a2395cf0c0a90ad9b4ce1dc9bb0a90205c7..01546a7bc270a2e6b840b080dd1ffa614d95c69d 100644 (file)
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -55,7 +55,8 @@ virAdmGlobalInit(void)
     if (virErrorInitialize() < 0)
         goto error;
 
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        goto error;
 
 #ifdef WITH_LIBINTL_H
     if (!bindtextdomain(PACKAGE, LOCALEDIR))

diff --git a/src/libvirt.c b/src/libvirt.c
index ef9fc403d083ffedbf30e870fa4d76f3107f7077..35fd74fe08da06fae3733906c99b700c8bb2abaf 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -232,7 +232,8 @@ virGlobalInit(void)
         goto error;
     }
 
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        goto error;
 
     virNetTLSInit();
 

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 67ebed361ae0a56b1c217947ac1adf667ef413d3..3c930eaacdee0daee47d5d0a2547205e24935225 100644 (file)
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2503,7 +2503,8 @@ int main(int argc, char *argv[])
     }
 
     /* Initialize logging */
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        exit(EXIT_FAILURE);
 
     while (1) {
         int c;

diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
index 4f4dbff7d0ad4279843a5d442e1914dc21d3601e..78f02020a7454a816afb9769845eabbf903cd623 100644 (file)
--- a/src/remote/remote_ssh_helper.c
+++ b/src/remote/remote_ssh_helper.c
@@ -402,7 +402,8 @@ int main(int argc, char **argv)
     virFileActivateDirOverrideForProg(argv[0]);
 
     /* Initialize the log system */
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        exit(EXIT_FAILURE);
 
     uri_str = argv[1];
     VIR_DEBUG("Using URI %s", uri_str);

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index b7ffb5e2c3245d30d1156b4323343b6d3d8f29b8..28717b7e38b8b8b5001c16a76b62190c0f67ccc2 100644 (file)
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1449,7 +1449,8 @@ main(int argc, char **argv)
     virFileActivateDirOverrideForProg(argv[0]);
 
     /* Initialize the log system */
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        exit(EXIT_FAILURE);
 
     /* clear the environment */
     environ = NULL;

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index ba5a2090768b62b9c5d2f14ec90e41de7d9a06e6..3b8c1c65c160b82aee987873932e18f2a40b65fd 100644 (file)
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -751,7 +751,8 @@ virExec(virCommand *cmd)
     VIR_FORCE_CLOSE(null);
 
     /* Initialize full logging for a while */
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        goto fork_error;
 
     if (cmd->pidfile &&
         virPipe(pipesync) < 0)

diff --git a/src/util/virdaemon.c b/src/util/virdaemon.c
index 795206301227791ccce78b1bae9ddb7ce685313f..5c0ca534cf16688e9a14dc0213b89c60e89eadf5 100644 (file)
--- a/src/util/virdaemon.c
+++ b/src/util/virdaemon.c
@@ -183,7 +183,8 @@ virDaemonSetupLogging(const char *daemon_name,
         return -1;
 
     /* If there are some environment variables defined, use those instead */
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        return -1;
 
     /*
      * Command line override for --verbose

diff --git a/src/util/virlog.c b/src/util/virlog.c
index 5848940c6ccbe08d7970931c435cd00be5b291be..139dce8d0220818c746cd720e8e4c600ea2de871 100644 (file)
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1200,24 +1200,34 @@ virLogParseDefaultPriority(const char *priority)
  *
  * Sets virLogDefaultPriority, virLogFilters and virLogOutputs based on
  * environment variables.
+ *
+ * This function might fail which should also make the caller fail.
  */
-void
+int
 virLogSetFromEnv(void)
 {
     const char *debugEnv;
 
     if (virLogInitialize() < 0)
-        return;
+        return -1;
 
     debugEnv = getenv("LIBVIRT_DEBUG");
-    if (debugEnv && *debugEnv)
-        virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv));
+    if (debugEnv && *debugEnv) {
+        int priority = virLogParseDefaultPriority(debugEnv);
+        if (priority < 0 ||
+            virLogSetDefaultPriority(priority) < 0)
+            return -1;
+    }
     debugEnv = getenv("LIBVIRT_LOG_FILTERS");
-    if (debugEnv && *debugEnv)
-        virLogSetFilters(debugEnv);
+    if (debugEnv && *debugEnv &&
+        virLogSetFilters(debugEnv))
+        return -1;
     debugEnv = getenv("LIBVIRT_LOG_OUTPUTS");
-    if (debugEnv && *debugEnv)
-        virLogSetOutputs(debugEnv);
+    if (debugEnv && *debugEnv &&
+        virLogSetOutputs(debugEnv))
+        return -1;
+
+    return 0;
 }
 
 

diff --git a/src/util/virlog.h b/src/util/virlog.h
index a04811e4083cd01de26ad83ce8a786d11057f28a..4f755c543b787eaaf03eb6f8cdd3c9e1ec31416e 100644 (file)
--- a/src/util/virlog.h
+++ b/src/util/virlog.h
@@ -146,7 +146,7 @@ char *virLogGetFilters(void);
 char *virLogGetOutputs(void);
 virLogPriority virLogGetDefaultPriority(void);
 int virLogSetDefaultPriority(virLogPriority priority);
-void virLogSetFromEnv(void);
+int virLogSetFromEnv(void) G_GNUC_WARN_UNUSED_RESULT;
 void virLogOutputFree(virLogOutput *output);
 void virLogOutputListFree(virLogOutput **list, int count);
 void virLogFilterFree(virLogFilter *filter);

diff --git a/tests/testutils.c b/tests/testutils.c
index 65f02e023199f66a44d19b3abc59be62155ddc84..322c3b94004c302825ca00463d3401198ef5d468 100644 (file)
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -835,7 +835,9 @@ int virTestMain(int argc,
     if (virErrorInitialize() < 0)
         return EXIT_FAILURE;
 
-    virLogSetFromEnv();
+    if (virLogSetFromEnv() < 0)
+        return EXIT_FAILURE;
+
     if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) {
         if (!(output = virLogOutputNew(virtTestLogOutput, virtTestLogClose,
                                        &testLog, VIR_LOG_DEBUG,