Re: [libvirt] [PATCH v2 11/20] virlog: Introduce virLogParseOutputs

> + > + 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

> + /* 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(); }

> + goto cleanup; > + } > + > + virLogOutputFree(output); Is this right? I don't think it's necessary unless you change to using the _COPY append macro

> + } > + > + 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...

John > > #endif >