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