[PATCH] failover: Protect against empty host names - sssd-devel - Fedora Mailing-Lists

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH] failover: Protect against empty host names

let krb5_backup_kpasswd failover...

[PATCH] set ret to EOK after for...

Michal Židek

Monday, 15 October 2012 Mon, 15 Oct '12

5:30 a.m.

Added new parameter to split_on_separator that allows to skip empty values. https://fedorahosted.org/sssd/ticket/1484 Patch is in attachment. Michal

Attachments:

0001-failover-Protect-against-empty-host-names.patch (text/x-patch — 8.5 KB)

Reply

Show replies by date

Dmitri Pal

Monday, 15 October Mon, 15 Oct

8:47 a.m.

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(a)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/

Reply

Michal Židek

9:04 a.m.

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(a)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(a)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

Reply

Dmitri Pal

9:41 a.m.

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(a)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(a)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. -- Thank you, Dmitri Pal Sr. Engineering Manager for IdM portfolio Red Hat Inc. ------------------------------- Looking to carve out IT costs? www.redhat.com/carveoutcosts/

Reply

Pavel Březina

Tuesday, 16 October Tue, 16 Oct

4:36 a.m.

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

Reply

Michal Židek

Wednesday, 17 October Wed, 17 Oct

9:01 a.m.

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

Reply

Pavel Březina

Friday, 23 November Fri, 23 Nov

3:11 a.m.

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?

Reply

Michal Židek

Monday, 26 November Mon, 26 Nov

7:40 a.m.

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

Reply

Pavel Březina

Tuesday, 27 November Tue, 27 Nov

8:18 a.m.

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?

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

Reply

Simo Sorce

8:33 a.m.

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. -- Simo Sorce * Red Hat, Inc * New York

Reply

Stephen Gallagher

10:49 a.m.

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.

Reply

Simo Sorce

10:57 a.m.

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. -- Simo Sorce * Red Hat, Inc * New York

Reply

Pavel Březina

11:57 a.m.

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.

Reply

Michal Židek

Thursday, 29 November Thu, 29 Nov

10:08 a.m.

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

Reply

Pavel Březina

Friday, 30 November Fri, 30 Nov

4:35 a.m.

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.

Reply

Michal Židek

6:32 a.m.

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

Reply

Pavel Březina

Monday, 3 December Mon, 3 Dec

6:35 a.m.

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

Reply

Michal Židek

8:02 a.m.

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

Reply

Pavel Březina

9:11 a.m.

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

Reply

Pavel Březina

Wednesday, 2 January Wed, 2 Jan

5:07 a.m.

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.

Reply

Pavel Březina

5:08 a.m.

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

Reply

Jakub Hrozek

10:52 a.m.

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.

Reply

Stef Walter

Tuesday, 16 October Tue, 16 Oct

5:06 a.m.

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

Reply

Jakub Hrozek

7:10 a.m.

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.

Reply

4132

days inactive

4211

days old

sssd-devel@lists.fedorahosted.org

Manage subscription

23 comments

7 participants

tags (0)

participants (7)

Dmitri Pal
Jakub Hrozek
Michal Židek
Pavel Březina
Simo Sorce
Stef Walter
Stephen Gallagher