Re: [libvirt] [PATCH v2 15/20] daemon: Split output parsing and output defining
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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;
>> }
>>
>>
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 15/20] daemon: Split output parsing and output defining
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.
>
> Signed-off-by: Erik Skultety
> ---
> daemon/libvirtd.c | 6 +++---
> src/locking/lock_daemon.c | 6 +++---
> src/logging/log_daemon.c | 6 +++---
> src/util/virlog.c | 2 +-
> tests/testutils.c | 11 +--
> tests/virlogtest.c| 10 ++
> 6 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 8efa69d..1251208 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -694,7 +694,7 @@ daemonSetupLogging(struct daemonConfig *config,
> virLogParseAndDefineFilters(config->log_filters);
>
> if (virLogGetNbOutputs() == 0)
> -virLogParseAndDefineOutputs(config->log_outputs);
> +virLogSetOutputs(config->log_outputs);
>
> /*
> * Command line override for --verbose
> @@ -721,7 +721,7 @@ daemonSetupLogging(struct daemonConfig *config,
>
> if (virAsprintf(&tmp, "%d:journald", priority) < 0)
> goto error;
> -virLogParseAndDefineOutputs(tmp);
> +virLogSetOutputs(tmp);
> VIR_FREE(tmp);
> }
> }
> @@ -764,7 +764,7 @@ daemonSetupLogging(struct daemonConfig *config,
> if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) <
> 0)
> goto error;
> }
> -virLogParseAndDefineOutputs(tmp);
> +virLogSetOutputs(tmp);
> VIR_FREE(tmp);
> }
>
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index 84c3029..9e736af 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -479,7 +479,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
> virLogParseAndDefineFilters(config->log_filters);
>
> if (virLogGetNbOutputs() == 0)
> -virLogParseAndDefineOutputs(config->log_outputs);
> +virLogSetOutputs(config->log_outputs);
>
> /*
> * Command line override for --verbose
> @@ -499,7 +499,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
> if (access("/run/systemd/journal/socket", W_OK) >= 0) {
> if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority())
> < 0)
> goto error;
> -virLogParseAndDefineOutputs(tmp);
> +virLogSetOutputs(tmp);
> VIR_FREE(tmp);
> }
> }
> @@ -543,7 +543,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
> if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) <
> 0)
> goto error;
> }
> -virLogParseAndDefineOutputs(tmp);
> +virLogSetOutputs(tmp);
> VIR_FREE(tmp);
> }
>
> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> index 48eece9..08ad6e0 100644
> --- a/src/logging/log_daemon.c
> +++ b/src/logging/log_daemon.c
> @@ -407,7 +407,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
> virLogParseAndDefineFilters(config->log_filters);
>
> if (virLogGetNbOutputs() == 0)
> -virLogParseAndDefineOutputs(config->log_outputs);
> +virLogSetOutputs(config->log_outputs);
>
> /*
> * Command line override for --verbose
> @@ -427,7 +427,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
> if (access("/run/systemd/journal/socket", W_OK) >= 0) {
> if (virAsprintf(&tmp, "%d:journald", virLogGetDefaultPriority())
> < 0)
> goto error;
> -virLogParseAndDefineOutputs(tmp);
> +virLogSetOutputs(tmp);
> VIR_FREE(tmp);
> }
> }
> @@ -471,7 +471,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
> if (virAsprintf(&tmp, "%d:stderr", virLogGetDefaultPriority()) <
> 0)
> goto error;
> }
> -virLogParseAndDefineOutputs(tmp);
> +virLogSetOutputs(tmp);
> VIR_FREE(tmp);
> }
>
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index b1d2543..b336529 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1630,7 +1630,7 @@ virLogSetFromEnv(void)
> virLogParseAndDefineFilters(debugEnv);
> debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS");
> if (debugEnv && *d
