On 12/03/2012 03:02 PM, Michal Židek wrote:
> On 12/03/2012 01:35 PM, Pavel Březina wrote:
>> On 11/30/2012 01:32 PM, Michal Židek wrote:
>>> On 11/30/2012 11:35 AM, Pavel Březina wrote:
>>>> On 11/29/2012 05:08 PM, Michal Židek wrote:
>>>>> On 11/27/2012 03:18 PM, Pavel Březina wrote:
>>>>>> 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?
>>>>>>
>>>>>
>>>>> It had no meaning. Deleted.
>>>>>
>>>>>>> + 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'?
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>>> /* 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.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>>>> if (substr_len == 0) {
>>>>>>> list[num_strings] = talloc_strdup(list,
"");
>>>>>> ^
>>>>>>> } else {
>>>>>>> list[num_strings] = talloc_strndup(list,
>>>>>>> substr_begin,
>>>>>>> substr_len);
>>>>>> ^
>>>>>>> }
>>>>>>
>>>>>> Indentation...
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>>> talloc_free(list);
>>>>>>> return ENOMEM;
>>>>>>
>>>>>> Can you use "fail" pattern instead of calling the
former each time
>>>>>> allocation fails?
>>>>>
>>>>> Label "done:" added.
>>>>>
>>>>>>
>>>>>> Otherwise it looks good. I like the function.
>>>>>>
>>>>>>
>>>>>
>>>>> New patch attached.
>>>>>
>>>>> Thanks
>>>>> Michal
>>>>
>>>> Hi,
>>>> I have few more nitpicks.
>>>>
>>>> In tests:
>>>>> {
>>>>> NULL,
>>>>> NULL,
>>>>> false,false,
>>>> ^ missing comma :P
>>>>> 0, EINVAL
>>>>> },
>>>
>>> I can see only missing space :P
>>>
>>>>
>>>> Function:
>>>>> while (isspace(*substr_begin) &&
>>>>> substr_begin < substr_end) {
>>>>> substr_begin++;
>>>>> substr_len--;
>>>>> }
>>>>>
>>>>> /* Trim trailing whitespace */
>>>>> while (substr_end - 1 > substr_begin &&
>>>>> isspace(*(substr_end-1))) {
>>>>> substr_end--;
>>>>> substr_len--;
>>>>> }
>>>>
>>>> Can you put those conditions on one line? Both fit into 80 char limit
>>>> perfectly and separating them doesn't really help clarity.
>>>>
>>>>> reallocated_list = talloc_realloc(mem_ctx, list, char*,
>>>>> num_strings + 2);
>>>> ^ indentation :)
>>>> and
>>>>> list = talloc(mem_ctx, char *);
>>>>
>>>> Allocate first on NULL and then talloc_steal() it to mem_ctx. This way
>>>> we can catch memory leak. (I know it's obvious there are not any at
>>>> the
>>>> moment, but still...)
>>>>
>>>> And last comment - since the function was completely rewritten, can
>>>> you
>>>> pick either ptr == NULL or !ptr so it is consistent?
>>>>
>>>> I'm sorry, I should have came with these earlier.
>>>>
>>>> Thanks.
>>>>
>>>
>>> New patch attached.
>>>
>>> Thanks
>>> Michal
>>
>> Hi,
>> we're almost there, but still nack :(
>>
>> tmp_ctx is always NULL this way, so it doesn't work as a talloc context
>> at all. You could just use:
>>
>> talloc_realloc(NULL, list, ...);
>> talloc_free(list);
>>
>> or initialize tmp_ctx with talloc_new(NULL).
>
> Yes, that is what I thought I did :-D . I see I didn't.
>
>>
>> But I like it better with tmp context after all, because you don't have
>> to use reallocated_list then and the code is more straightforward.
>>
>> So you can use:
>> TALLOC_CTX *tmp_ctx = NULL;
>>
>> tmp_ctx = talloc_new(NULL);
>> if (tmp_ctx == NULL) {...}
>>
>> ...
>> list = talloc_realloc(tmp_ctx, list, char*, num_strings + 2);
>> if (list == NULL) {
>> ret = ENOMEM;
>> goto done;
>> }
>> ...
>> done:
>> talloc_free(tmp_ctx)
>
> You are right, with the tmp context, the reallocated_list is not needed.
>
>
> New patch attached.
>
> Thanks
> Michal
Hi,
I dared to amend your patch with following changes:
- if (substr_len == 0) {
- list[num_strings] = talloc_strdup(list, "");
- } else {
- list[num_strings] = talloc_strndup(list, substr_begin,
- substr_len);
- }
+ /* empty string is stored for substr_len == 0 */
+ list[num_strings] = talloc_strndup(list, substr_begin,
substr_len);
if (list[num_strings] == NULL) {
ret = ENOMEM;
goto done;
talloc_strndup(list, substr_begin, 0) == talloc_strdup(list, "")
so we don't need to branch here
-107,7 +102,7 @@ int split_on_separator(TALLOC_CTX *mem_ctx, const
char *str,
/* No allocations were done, make space for the NULL */
list = talloc(tmp_ctx, char *);
if (list == NULL) {
- ret ENOMEM;
+ ret = ENOMEM;
goto done;
}
}
You've somehow managed to delete = which was present in the previous
iteration.
I tried this patch with various weird ldap uris and it works splendidly.
ACK from me
Just a remainder. This patch has been waiting for some time.