On 12/03/2012 04:11 PM, Pavel Březina wrote:
> 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.