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

[Erik Skultety]

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161006/b08280a7/attachment-0001.sig>