Re: [libvirt] [PATCH v2 15/20] daemon: Split output parsing and output defining

21 Sep 2016

On 08/18/2016 07:47 AM, Erik Skultety wrote:
...
Since virLogParseAndDefineOutputs is going to be stripped from 'output defining'
logic, replace all relevant occurrences with virLogSetOutputs call to make the
change transparent to all original callers (daemons mostly).
I think the commit message needs to be adjusted... What's really
happening is that now that you have a "real" virLogParseOutputs and a
virLogSetOutputs, the "ParseAndDefine" is being duplicitous so you're
replacing them with the Set call.

Personally, I think this patch should be combined with Patch 13 so we
don't have that duplicity and/or any other "strangeness" as a result of
having the ParseAndDefine being called as well as the Parse.
...
Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 daemon/libvirtd.c         |  6 +++---
 src/locking/lock_daemon.c |  6 +++---
 src/logging/log_daemon.c  |  6 +++---
 src/util/virlog.c         |  2 +-
 tests/testutils.c         | 11 +++++++++--
 tests/virlogtest.c        | 10 ++++++----
 6 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 8efa69d..1251208 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -694,7 +694,7 @@ daemonSetupLogging(struct daemonConfig *config,
         virLogParseAndDefineFilters(config->log_filters);
if (virLogGetNbOutputs() == 0)
-        virLogParseAndDefineOutputs(config->log_outputs);
+        virLogSetOutputs(config->log_outputs);
/*
      * Command line override for --verbose
@@ -721,7 +721,7 @@ daemonSetupLogging(struct daemonConfig *config,
if (virAsprintf(&tmp, "%d:journald", priority) < 0)
                 goto error;
-            virLogParseAndDefineOutputs(tmp);
+            virLogSetOutputs(tmp);
             VIR_FREE(tmp);
         }
     }
@@ -764,7 +764,7 @@ daemonSetupLogging(struct daemonConfig *config,
             if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
                 goto error;
         }
-        virLogParseAndDefineOutputs(tmp);
+        virLogSetOutputs(tmp);
         VIR_FREE(tmp);
     }
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 84c3029..9e736af 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -479,7 +479,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
         virLogParseAndDefineFilters(config->log_filters);
if (virLogGetNbOutputs() == 0)
-        virLogParseAndDefineOutputs(config->log_outputs);
+        virLogSetOutputs(config->log_outputs);
/*
      * Command line override for --verbose
@@ -499,7 +499,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
         if (access("/run/systemd/journal/socket", W_OK) >= 0) {
             if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0)
                 goto error;
-            virLogParseAndDefineOutputs(tmp);
+            virLogSetOutputs(tmp);
             VIR_FREE(tmp);
         }
     }
@@ -543,7 +543,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
             if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
                 goto error;
         }
-        virLogParseAndDefineOutputs(tmp);
+        virLogSetOutputs(tmp);
         VIR_FREE(tmp);
     }
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index 48eece9..08ad6e0 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -407,7 +407,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
         virLogParseAndDefineFilters(config->log_filters);
if (virLogGetNbOutputs() == 0)
-        virLogParseAndDefineOutputs(config->log_outputs);
+        virLogSetOutputs(config->log_outputs);
/*
      * Command line override for --verbose
@@ -427,7 +427,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
         if (access("/run/systemd/journal/socket", W_OK) >= 0) {
             if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority()) < 0)
                 goto error;
-            virLogParseAndDefineOutputs(tmp);
+            virLogSetOutputs(tmp);
             VIR_FREE(tmp);
         }
     }
@@ -471,7 +471,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
             if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) < 0)
                 goto error;
         }
-        virLogParseAndDefineOutputs(tmp);
+        virLogSetOutputs(tmp);
         VIR_FREE(tmp);
     }
diff --git a/src/util/virlog.c b/src/util/virlog.c
index b1d2543..b336529 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1630,7 +1630,7 @@ virLogSetFromEnv(void)
         virLogParseAndDefineFilters(debugEnv);
     debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS");
     if (debugEnv && *debugEnv)
-        virLogParseAndDefineOutputs(debugEnv);
+        virLogSetOutputs(debugEnv);
 }
diff --git a/tests/testutils.c b/tests/testutils.c
index 8af8707..21687e5 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -871,6 +871,9 @@ int virTestMain(int argc,
 #ifdef TEST_OOM
     char *oomstr;
 #endif
+    size_t noutputs = 0;
+    virLogOutputPtr output = NULL;
+    virLogOutputPtr *outputs = NULL;
if (getenv("VIR_TEST_FILE_ACCESS"))
         VIRT_TEST_PRELOAD(TEST_MOCK);
@@ -910,8 +913,11 @@ int virTestMain(int argc,
virLogSetFromEnv();
     if (!getenv("LIBVIRT_DEBUG") && !virLogGetNbOutputs()) {
-        if (virLogDefineOutput(virtTestLogOutput, virtTestLogClose, &testLog,
-                               VIR_LOG_DEBUG, VIR_LOG_TO_STDERR, NULL, 0) < 0)
+        if (!(output = virLogOutputNew(virtTestLogOutput, virtTestLogClose,
+                                       &testLog, VIR_LOG_DEBUG,
+                                       VIR_LOG_TO_STDERR, NULL)) ||
+            VIR_APPEND_ELEMENT(outputs, noutputs, output) < 0 ||
+            virLogDefineOutputs(outputs, noutputs) < 0)
I know - just a test, but you could leak output if the APPEND fails.
You could use the _COPY macro, then do the Free in both the error and
success case...
...
return EXIT_FAILURE;
     }
@@ -987,6 +993,7 @@ int virTestMain(int argc,
             fprintf(stderr, "%*s", 40 - (int)(testCounter % 40), "");
         fprintf(stderr, " %-3zu %s\n", testCounter, ret == 0 ? "OK" : "FAIL");
     }
+    virLogReset();
     VIR_FREE(perl);
     return ret;
 }
diff --git a/tests/virlogtest.c b/tests/virlogtest.c
index afcd84a..b2c66f7 100644
--- a/tests/virlogtest.c
+++ b/tests/virlogtest.c
@@ -48,9 +48,10 @@ testLogParseOutputs(const void *opaque)
 {
     int ret = -1;
     int noutputs;
+    virLogOutputPtr *outputs = NULL;
     const struct testLogData *data = opaque;
-    noutputs = virLogParseAndDefineOutputs(data->str);
+    noutputs = virLogParseOutputs(data->str, &outputs);
     if (noutputs < 0) {
         if (!data->pass) {
             VIR_TEST_DEBUG("Got expected error: %s\n",
@@ -70,7 +71,7 @@ testLogParseOutputs(const void *opaque)
ret = 0;
  cleanup:
-    virLogReset();
+    virLogOutputListFree(outputs, noutputs);
     return ret;
 }
Shouldn't the rest of this go with the "next" patch which does the
Filter magic (it's going to have a similar merge with patch comment)

Conditional ACK on moving/merging with patch 13. Beyond that just a
small justification for the separation...

w/r/t: memory leak on output, that will probably raise the ire of
Coverity or running with valgrind.

John
...
@@ -79,9 +80,10 @@ testLogParseFilters(const void *opaque)
 {
     int ret = -1;
     int nfilters;
+    virLogFilterPtr *filters = NULL;
     const struct testLogData *data = opaque;
-    nfilters = virLogParseAndDefineFilters(data->str);
+    nfilters = virLogParseFilters(data->str, &filters);
     if (nfilters < 0) {
         if (!data->pass) {
             VIR_TEST_DEBUG("Got expected error: %s\n",
@@ -101,7 +103,7 @@ testLogParseFilters(const void *opaque)
ret = 0;
  cleanup:
-    virLogReset();
+    virLogFilterListFree(filters, nfilters);
     return ret;
 }


    

Re: [libvirt] [PATCH v2 15/20] daemon: Split output parsing and output defining

John Ferlan