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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]