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;
> }
>
>
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)