[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