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