Re: [PATCH 6/8] util: Check for errors in virLogSetFromEnv

[Martin Kletzander]

On Wed, Jan 05, 2022 at 12:29:18PM +0100, Erik Skultety wrote:

On Tue, Jan 04, 2022 at 02:47:10PM +0100, Martin Kletzander wrote:

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.

Logging settings are also something that we want to report
errors on for example when it is being done over admin API.

True in general, but slightly off-topic wrt to the patch itself as
virLogSetFromEnv is irrelevant to admin API usage.

Yeah, this was supposed to be a part of another commit message, I just
wanted to guard against someone suggesting we do not report errors at
all (which would be another solution, but a wrong one I think).

...

-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;

          ^^^ indentation

Thanks for catching that!

+    }
     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;
 }

Reviewed-by: Erik Skultety <eskul...@redhat.com>

signature.asc
Description: PGP signature