[SSSD] [PATCH] failover: Protect against empty host names
Pavel Březina
pbrezina at redhat.com
Tue Nov 27 14:18:16 UTC 2012
On 11/26/2012 02:40 PM, Michal Židek wrote:
> On 11/23/2012 10:11 AM, Pavel Březina wrote:
>> On 10/17/2012 04:01 PM, Michal Židek wrote:
>>> On 10/16/2012 11:36 AM, Pavel Březina wrote:
>>>> On 10/15/2012 12:30 PM, Michal Židek wrote:
>>>>> Added new parameter to split_on_separator that allows to skip
>>>>> empty values.
>>>>>
>>>>> https://fedorahosted.org/sssd/ticket/1484
>>>>>
>>>>> Patch is in attachment.
>>>>>
>>>>> Michal
>>>>
>>>> Nack. See below.
>>>>
>>>>> --- a/src/util/util.c
>>>>> +++ b/src/util/util.c
>>>>> @@ -31,7 +31,8 @@
>>>>> * the separator is a string, and is case-sensitive.
>>>>> * optionally single values can be trimmed of of spaces and tabs */
>>>>> int split_on_separator(TALLOC_CTX *mem_ctx, const char *str,
>>>>> - const char sep, bool trim, char ***_list, int
>>>>> *size)
>>>>> + const char sep, bool trim, bool skip_empty,
>>>>> + char ***_list, int *size)
>>>>> {
>>>>> const char *t, *p, *n;
>>>>> size_t l, len;
>>>>> @@ -80,6 +81,11 @@ int split_on_separator(TALLOC_CTX *mem_ctx, const
>>>>> char *str,
>>>>> }
>>>>>
>>>>> if (len == 0) {
>>>>> + if (skip_empty) {
>>>>> + /* Move to next string and continue */
>>>>> + t = n;
>>>>> + continue;
>>>>> + }
>>>>
>>>> Move this^ condition before talloc_realloc.
>>>>
>>>
>>> I did not like the function at all, it seemed to be written for
>>> different purpose but slightly modified to fit our needs. I wrote a new
>>> version in this patch and added test for it.
>>>
>>> New patch attached.
>>>
>>> Michal
>>
>> Hi,
>> can you rebase it atop the current master please?
>>
>
> Sure, here is the new patch.
>
> Thanks
> Michal
Thanks.
Nack.
Test issues:
> + {
> + ", ,,",
> + (const char*[]){NULL,},
> + true, true,
> + 0, 0
> + },
What is the meaning of the comma after NULL?
> + for (i = 0; str_ref = sts[a].expected_list[i], str_out = list[i]; i++) {
> + fail_unless(!strcmp(str_ref, str_out),
> + "Expected:%s Got:%s\n", str_ref, str_out);
> + }
Please use strcmp() == 0.
Function issues:
> char **r;
Please, choose a better name for this variable. Something that explains
its meaning. Maybe 'reallocated_list'?
> /* Trim leading whitespace if needed */
> while (trim && isspace(*substr_begin) &&
> substr_begin < substr_end) {
> substr_begin++;
> substr_len--;
> }
>
> /* Trim trailing whitespace if needed */
> while (trim && substr_end - 1 > substr_begin &&
> isspace(*(substr_end-1))) {
> substr_end--;
> substr_len--;
> }
Put the condition on one line. And separate trim from while please.
if (trim) {
while () // leading
while () // trailing
}
> if (!(skip_empty && substr_len == 0)) {
if (!skip_empty || substr_len == 0) is more readable in my opinion.
> if (substr_len == 0) {
> list[num_strings] = talloc_strdup(list, "");
^
> } else {
> list[num_strings] = talloc_strndup(list, substr_begin,
> substr_len);
^
> }
Indentation...
> talloc_free(list);
> return ENOMEM;
Can you use "fail" pattern instead of calling the former each time
allocation fails?
Otherwise it looks good. I like the function.
More information about the sssd-devel
mailing list