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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list