On Wed, Jan 02, 2013 at 12:08:31PM +0100, Pavel Březina wrote:
On 01/02/2013 12:07 PM, Pavel Březina wrote:
>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.
s/remainder/reminder :-)
I agree with how you amended Michal's patch. I haven't reviewed the
patch fully, though, I trust your good judgement there :-)
Pushed to master.