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

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.

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