+ + VIR_DEBUG("outputs=%s", src); + + if (!(strings = virStringSplit(src, " ", 0)))
You could use the Count version and then...
+ goto cleanup; + + for (i = 0; strings[i]; i++) {
...rather than strings[i], it's < count
Well, this way we spared one unnecessary variable, but whatever, I can always add it there to make it more readable I guess.
+ /* virStringSplit may return empty strings */ + if (STREQ(strings[i], "")) + continue; + + if (!(output = virLogParseOutput(strings[i]))) + goto cleanup; + + /* let's check if a duplicate output does not already exist in which + * case we replace it with its last occurrence + */ + if ((at = virLogFindOutput(list, noutputs, output->dest, + output->name)) >= 0) { + virLogOutputFree(list[at]); + VIR_DELETE_ELEMENT(list, at, noutputs); + } + + if (VIR_APPEND_ELEMENT(list, noutputs, output) < 0) { + virLogOutputFree(output);
In this case, the old one is also gone... So we've effectively removed it. Is that intentional? or should the DELETE of 'at' occur after this successfully adds a new one?
IOW: at = virLogFindOutput() if (VIR_APPEND_ELEMENT() < 0) ... } if (at >= 0) { virLogOutputFree(list[at]); VIR_DELETE_ELEMENT(); }
Ouch, perfect catch, thanks, we would definitely lose the original if the append failed.
+ goto cleanup; + } + + virLogOutputFree(output);
Is this right? I don't think it's necessary unless you change to using the _COPY append macro
I suppose you're right, since it issues memmove, I'll try some scenarios to compare the behaviour though.
+ } + + ret = noutputs; + *outputs = list;
If you then set "list = NULL"...
+ cleanup: + if (ret < 0)
... the (ret < 0) is not necessary
+ virLogOutputListFree(list, noutputs); + virStringFreeList(strings); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index e7f6b85..ed60c00 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -247,5 +247,6 @@ virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, virLogOutputPtr virLogNewOutputToJournald(int priority); virLogOutputPtr virLogParseOutput(const char *src); virLogFilterPtr virLogParseFilter(const char *src); +int virLogParseOutputs(const char *src, virLogOutputPtr **outputs);
s/;/ATTRIBUTE_NONNULL(1);
Conditional ACK - guess I'm curious how the duplication and error path issue falls out...
Thanks a lot. Erik
John
#endif