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