On 10/06/2016 06:42 AM, Erik Skultety wrote:
On 21/09/16 21:14, John Ferlan wrote:
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.
My original intention was to make it clear in a separate patch that that was the specific moment where everything would switch to the new versions transparently, I can squash it to 13 I just wanted to make it more clear and more easier for the reviewer, that's all.
Understood - the ordering and separation did make review easier. I think in 20/20 hindsight though - since nothing calls SetOutputs until now that the duplicity and bisect concern I noted cannot happen. Similarly for patch 16's comments... Keep the separation as is - that's fine. John
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...
Actually, not just the output but outputs as well if define fails. In that case APPEND without _COPY still might be a better solution, since when APPEND fails, I can free the output, if APPEND succeeds, it clears output, so that when define fails afterwards, I can still free both the output and outputs (the list), since one of those will always be NULL in any case of failure, but thanks anyway.
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)
Oops, dunno how that hunk got there :/ (probably multiple interactive rebases as usual...)
Erik
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; }