Added new parameter to split_on_separator that allows to skip empty values.
https://fedorahosted.org/sssd/ticket/1484
Patch is in attachment.
Michal
On 10/15/2012 06:30 AM, 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
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I do not see the whole code in front of me. Does it trim spaces? I mean does it handle values "a,,b" and "a , , b" in the same way?
On 10/15/2012 03:47 PM, Dmitri Pal wrote:
On 10/15/2012 06:30 AM, 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
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I do not see the whole code in front of me. Does it trim spaces? I mean does it handle values "a,,b" and "a , , b" in the same way?
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Dmitri, if parameters 'trim' and 'ignore_empty' are both set to 'true', then 'a,,b' and 'a, ,b' give both the same result ("a" and "b" are stored to the list of strings and the empty value is ignored).
Michal
On 10/15/2012 10:04 AM, Michal Židek wrote:
On 10/15/2012 03:47 PM, Dmitri Pal wrote:
On 10/15/2012 06:30 AM, 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
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I do not see the whole code in front of me. Does it trim spaces? I mean does it handle values "a,,b" and "a , , b" in the same way?
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Dmitri, if parameters 'trim' and 'ignore_empty' are both set to 'true', then 'a,,b' and 'a, ,b' give both the same result ("a" and "b" are stored to the list of strings and the empty value is ignored).
Michal
Good, this is what I wanted to confirm.
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)@@ -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.
Handling last element should be rewritten so we treat it the same way as all previous elements i.e. trim, check if we will put the element in, realloc, strdup.
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)@@ -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
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)@@ -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?
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)@@ -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
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)@@ -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?
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'?
/* 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.
if (substr_len == 0) { list[num_strings] = talloc_strdup(list, "");
^
} else { list[num_strings] = talloc_strndup(list, substr_begin, substr_len);
^
}
Indentation...
talloc_free(list); return ENOMEM;
Can you use "fail" pattern instead of calling the former each time allocation fails?
Otherwise it looks good. I like the function.
On Tue, 2012-11-27 at 15:18 +0100, Pavel Březina wrote:
Can you use "fail" pattern instead of calling the former each time allocation fails?
Otherwise it looks good. I like the function.
I would like to vouch for always using 'done:' as a label and not other arbitrary words.
Whether it is a failure or not should be determined by testing the error value. This way we always use the same pattern, which helps in reading the code.
Simo.
On Tue 27 Nov 2012 09:33:00 AM EST, Simo Sorce wrote:
On Tue, 2012-11-27 at 15:18 +0100, Pavel Březina wrote:
Can you use "fail" pattern instead of calling the former each time allocation fails?
Otherwise it looks good. I like the function.
I would like to vouch for always using 'done:' as a label and not other arbitrary words.
Whether it is a failure or not should be determined by testing the error value. This way we always use the same pattern, which helps in reading the code.
There are three labels that we've used fairly consistently in the code in the past:
done: Used when the exit can be shared between success and failure modes, possibly with some minor conditionals.
fail: Used when some part of the exit processing requires overly-complicated differences from the success case. I think 99% of these could be merged to done: cases (and should be, where appropriate), but I suspect there are a few situations where this may need to remain. It should be the exception, though.
immediate: Used in tevent_req routines where some part of the first phase of the request (the one that initially creates the request object) fails after the object is created. In this case, we need to return the object and call tevent_req_immediate(), which I think should definitely remain a separate label from "done:" to indicate to future maintainers that exiting via this label is cutting directly to the completion callback.
On Tue, 2012-11-27 at 11:49 -0500, Stephen Gallagher wrote:
On Tue 27 Nov 2012 09:33:00 AM EST, Simo Sorce wrote:
On Tue, 2012-11-27 at 15:18 +0100, Pavel Březina wrote:
Can you use "fail" pattern instead of calling the former each time allocation fails?
Otherwise it looks good. I like the function.
I would like to vouch for always using 'done:' as a label and not other arbitrary words.
Whether it is a failure or not should be determined by testing the error value. This way we always use the same pattern, which helps in reading the code.
There are three labels that we've used fairly consistently in the code in the past:
done: Used when the exit can be shared between success and failure modes, possibly with some minor conditionals.
fail: Used when some part of the exit processing requires overly-complicated differences from the success case. I think 99% of these could be merged to done: cases (and should be, where appropriate), but I suspect there are a few situations where this may need to remain. It should be the exception, though.
immediate: Used in tevent_req routines where some part of the first phase of the request (the one that initially creates the request object) fails after the object is created. In this case, we need to return the object and call tevent_req_immediate(), which I think should definitely remain a separate label from "done:" to indicate to future maintainers that exiting via this label is cutting directly to the completion callback.
I think these things should be best left to a comment in the code, rather than labels we use for exceptions. Because those labels become inconsistent quickly if you have to change a long function that used done but now you would like it to return immediately for one case.
I particularly do not want to see these cases turn into having
...
done: ... some stuff here ... return foo; immediated: return tevent_req_immediate(); }
I think double labels are quite nasty.
I would prefer:
done: if (immediate) { return tevent_req_immediate() } ... some stuff here ... return foo; }
And just use one lable and make it always be done.
Same for fail, I came to realize it is really annoying to have to rename a bunch of labels because after changing the code you do not have just fail conditions but either fail or done.
If we use only 'done' then we leave fail conditions to an error code test (forces us to use proper error codes too) and immdiate cases to an explicit condition enforced by a boolean.
I know this is all personal preference, just laying out mine, if people feel strongly for the current mutli-label system please speak up.
Simo.
On 11/27/2012 05:57 PM, Simo Sorce wrote:
On Tue, 2012-11-27 at 11:49 -0500, Stephen Gallagher wrote:
On Tue 27 Nov 2012 09:33:00 AM EST, Simo Sorce wrote:
On Tue, 2012-11-27 at 15:18 +0100, Pavel Březina wrote:
Can you use "fail" pattern instead of calling the former each time allocation fails?
Otherwise it looks good. I like the function.
I would like to vouch for always using 'done:' as a label and not other arbitrary words.
Whether it is a failure or not should be determined by testing the error value. This way we always use the same pattern, which helps in reading the code.
There are three labels that we've used fairly consistently in the code in the past:
done: Used when the exit can be shared between success and failure modes, possibly with some minor conditionals.
fail: Used when some part of the exit processing requires overly-complicated differences from the success case. I think 99% of these could be merged to done: cases (and should be, where appropriate), but I suspect there are a few situations where this may need to remain. It should be the exception, though.
immediate: Used in tevent_req routines where some part of the first phase of the request (the one that initially creates the request object) fails after the object is created. In this case, we need to return the object and call tevent_req_immediate(), which I think should definitely remain
tevent_req_post() to be precise
a separate label from "done:" to indicate to future maintainers that exiting via this label is cutting directly to the completion callback.
I think these things should be best left to a comment in the code, rather than labels we use for exceptions. Because those labels become inconsistent quickly if you have to change a long function that used done but now you would like it to return immediately for one case.
I particularly do not want to see these cases turn into having
...
done: ... some stuff here ... return foo; immediated: return tevent_req_immediate(); }
I didn't come across this code in SSSD yet. We use immediate only to stress the fact that we are working with tevent_req and we want to mark it as done or as error. Otherwise it's basically "done" label.
The templates we are using are as follows:
*_out = talloc_steal() ret = EOK;
done: if (ret != EOK) { ... } talloc_free(tmp_ctx); return ret;
If we don't free resources for EOK branch we use:
return ok;
fail: ... talloc_free(tmp_ctx); return error;
For tevent_req:
return req;
immediate: if (ret == EOK) { tevent_req_post(req) } else { tevent_req_error(req, ret) } return req;
I think double labels are quite nasty.
I would prefer:
done: if (immediate) { return tevent_req_immediate() } ... some stuff here ... return foo; }
And just use one lable and make it always be done.
Same for fail, I came to realize it is really annoying to have to rename a bunch of labels because after changing the code you do not have just fail conditions but either fail or done.
If we use only 'done' then we leave fail conditions to an error code test (forces us to use proper error codes too) and immdiate cases to an explicit condition enforced by a boolean.
I know this is all personal preference, just laying out mine, if people feel strongly for the current mutli-label system please speak up.
Simo.
I don't have a strong opinion on the fail label. It can be translated into done, it is annoying to change it when necessary, but also it may make the code flow more clear.
But I would definitely leave immediate label in the game.
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)@@ -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
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 },
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.
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 :P0, 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
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 :P0, 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)
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 :P0, 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
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 :P0, 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
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 :P0, 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.
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 :-)
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.
On 10/15/12 06:30, Michal Židek wrote:
Added new parameter to split_on_separator that allows to skip empty values.
I think this may break the work around I mentioned here:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-October/011906.html
Stef
On Tue, Oct 16, 2012 at 06:06:04AM -0400, Stef Walter wrote:
On 10/15/12 06:30, Michal Židek wrote:
Added new parameter to split_on_separator that allows to skip empty values.
I think this may break the work around I mentioned here:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-October/011906.html
Stef
Michal, can you test your patch together with Stef's workaround patch?
Also it would be nice to include a test case in the failover unit tests.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Michal Židek -
Pavel Březina -
Simo Sorce -
Stef Walter -
Stephen Gallagher