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).
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)