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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel