>> +
>> + 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
>>
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list