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