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 <eskul...@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;
>  }
>  
> 

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

Reply via email to