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 :-)
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
