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
From 5ec29debd7914869bb92222dd89120fedb6c0493 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Mon, 15 Oct 2012 12:21:00 +0200
Subject: [PATCH] failover: Protect against empty host names
Added new parameter to split_on_separator that allows to skip
empty values.
The whole function was rewritten. Unit test case was added to
check the new implementation.
https://fedorahosted.org/sssd/ticket/1484
---
src/confdb/confdb.c | 2 +-
src/providers/ad/ad_common.c | 2 +-
src/providers/ipa/ipa_common.c | 2 +-
src/providers/krb5/krb5_common.c | 2 +-
src/providers/ldap/ldap_common.c | 4 +-
src/providers/ldap/ldap_init.c | 2 +-
src/providers/ldap/sdap_async_sudo_hostinfo.c | 4 +-
src/responder/common/responder_common.c | 3 +-
src/tests/util-tests.c | 91 +++++++++++++++
src/util/util.c | 159 +++++++++++---------------
src/util/util.h | 3 +-
11 files changed, 173 insertions(+), 101 deletions(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 3707f18b54641948ea0c166621ac140a30b1bac9..600d423d0d074b098055a8a15346b1cd7b34c819 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -599,7 +599,7 @@ int confdb_get_string_as_list(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
goto done;
}
- ret = split_on_separator(ctx, values[0], ',', true, result, NULL);
+ ret = split_on_separator(ctx, values[0], ',', true, true, result, NULL);
done:
talloc_free(values);
diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 8600dab22a6392367fd6a4465177a49bffb81f8e..dff1071dd7dca99969f912d07184cc3ca401b9b8 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -156,7 +156,7 @@ ad_servers_init(TALLOC_CTX *mem_ctx,
if (!tmp_ctx) return ENOMEM;
/* Split the server list */
- ret = split_on_separator(tmp_ctx, servers, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, servers, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index eb384a1f33c7a0552d38c2327a15175a608299e9..be1bd1d2f8c42143d7ff1fe205ffb774ffe19dd0 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -770,7 +770,7 @@ errno_t ipa_servers_init(struct be_ctx *ctx,
}
/* split server parm into a list */
- ret = split_on_separator(tmp_ctx, servers, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, servers, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c
index ed2fffae1bcaef9274e0f54278f9e4d9a17c41c3..c6865c09960463b8648957cb777d0d2b4e7b1dde 100644
--- a/src/providers/krb5/krb5_common.c
+++ b/src/providers/krb5/krb5_common.c
@@ -486,7 +486,7 @@ errno_t krb5_servers_init(struct be_ctx *ctx,
return ENOMEM;
}
- ret = split_on_separator(tmp_ctx, servers, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, servers, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index f8b921adfe0aabc8f7ab70caab4b0201f7959c46..a97dc34e28e7e07d93026a855eb2d45b3e142b66 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -561,7 +561,7 @@ errno_t common_parse_search_base(TALLOC_CTX *mem_ctx,
goto done;
}
- ret = split_on_separator(tmp_ctx, unparsed_base, '?', false,
+ ret = split_on_separator(tmp_ctx, unparsed_base, '?', false, false,
&split_bases, &count);
if (ret != EOK) goto done;
@@ -1214,7 +1214,7 @@ errno_t sdap_urls_init(struct be_ctx *ctx,
/* split server parm into a list */
- ret = split_on_separator(tmp_ctx, urls, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, urls, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(1, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/ldap/ldap_init.c b/src/providers/ldap/ldap_init.c
index 52bd233f16521a9f119104a508bc00292c39782f..807526b88c939a926a25ee68782af2d3520a7503 100644
--- a/src/providers/ldap/ldap_init.c
+++ b/src/providers/ldap/ldap_init.c
@@ -294,7 +294,7 @@ int sssm_ldap_access_init(struct be_ctx *bectx,
order = "filter";
}
- ret = split_on_separator(access_ctx, order, ',', true,
+ ret = split_on_separator(access_ctx, order, ',', true, true,
&order_list, &order_list_len);
if (ret != EOK) {
DEBUG(1, ("split_on_separator failed.\n"));
diff --git a/src/providers/ldap/sdap_async_sudo_hostinfo.c b/src/providers/ldap/sdap_async_sudo_hostinfo.c
index 0a695cdbf1742f1e9fae1d5463a08f964924b90b..f47e98651fc79e2b56fe70f18b72bdce5a426892 100644
--- a/src/providers/ldap/sdap_async_sudo_hostinfo.c
+++ b/src/providers/ldap/sdap_async_sudo_hostinfo.c
@@ -89,7 +89,7 @@ struct tevent_req * sdap_sudo_get_hostinfo_send(TALLOC_CTX *mem_ctx,
conf_ip_addr = dp_opt_get_string(opts->basic, SDAP_SUDO_IP);
if (conf_hostnames != NULL) {
- ret = split_on_separator(state, conf_hostnames, ' ', true,
+ ret = split_on_separator(state, conf_hostnames, ' ', true, true,
&state->hostnames, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
@@ -102,7 +102,7 @@ struct tevent_req * sdap_sudo_get_hostinfo_send(TALLOC_CTX *mem_ctx,
}
if (conf_ip_addr != NULL) {
- ret = split_on_separator(state, conf_ip_addr, ' ', true,
+ ret = split_on_separator(state, conf_ip_addr, ' ', true, true,
&state->ip_addr, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 50705a3f227d38f7789d4adc5a69d246f4b1be29..811778624f9460a33adf3074ba67bcc61ee1da7a 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -167,7 +167,8 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *cvs_string,
char *endptr;
struct passwd *pwd;
- ret = split_on_separator(mem_ctx, cvs_string, ',', true, &list, &list_size);
+ ret = split_on_separator(mem_ctx, cvs_string, ',', true, false,
+ &list, &list_size);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, ("split_on_separator failed [%d][%s].\n",
ret, strerror(ret)));
diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index ff809a5a70d0401bf6672951c248fb05d7bbfb4d..e7e04e6574f02c47f45de7bd36168c6d5d8bcd74 100644
--- a/src/tests/util-tests.c
+++ b/src/tests/util-tests.c
@@ -718,6 +718,96 @@ START_TEST(test_atomicio_read_from_empty_file)
}
END_TEST
+struct split_data {
+ const char *input;
+ const char **expected_list;
+ bool trim;
+ bool skip_empty;
+ int expected_size;
+ int expected_ret;
+};
+
+START_TEST(test_split_on_separator)
+{
+ TALLOC_CTX *mem = global_talloc_context;
+ errno_t ret;
+ char **list = NULL;
+ int size;
+ const char *str_ref;
+ const char *str_out;
+ int i;
+ int a;
+ int num_of_tests;
+ struct split_data sts[] = {
+ {
+ "one,two,three", /* input string */
+ (const char *[]){"one", "two", "three", NULL}, /* expec. output list */
+ false, false, /* trim, skip_empty */
+ 3, 0 /* expec. size, expec. retval */
+ },
+ {
+ "one,two,three",
+ (const char *[]){"one", "two", "three", NULL},
+ true, true,
+ 3, 0
+ },
+ {
+ " one, two ,three ",
+ (const char*[]){"one", "two", "three", NULL},
+ true, true,
+ 3, 0
+ },
+ {
+ /* If skip empty is false, single comma means "empty,empty" */
+ ",",
+ (const char*[]){"", "", NULL, NULL},
+ false, false,
+ 2, 0
+ },
+ {
+ "one, ,",
+ (const char*[]){"one", " ", "NULL", "NULL"},
+ false, true,
+ 2, 0
+ },
+ {
+ ", ,,",
+ (const char*[]){NULL},
+ true, true,
+ 0, 0
+ },
+ {
+ NULL,
+ NULL,
+ false, false,
+ 0, EINVAL
+ },
+ };
+ num_of_tests = sizeof(sts) / sizeof(struct split_data);
+
+ for (a = 0; a < num_of_tests; a++) {
+ ret = split_on_separator(mem, sts[a].input, ',', sts[a].trim,
+ sts[a].skip_empty, &list, &size);
+
+ fail_unless(ret == sts[a].expected_ret,
+ "split_on_separator failed [%d]: %s\n", ret,
+ strerror(ret));
+ if (ret) {
+ continue;
+ }
+ fail_unless(size == sts[a].expected_size, "Returned wrong size %d "
+ "(expected %d).\n", size, sts[a].expected_size);
+
+ for (i = 0; str_ref = sts[a].expected_list[i], str_out = list[i]; i++) {
+ fail_unless(strcmp(str_ref, str_out) == 0,
+ "Expected:%s Got:%s\n", str_ref, str_out);
+ }
+ talloc_free(list);
+ list = NULL;
+ }
+}
+END_TEST
+
Suite *util_suite(void)
{
Suite *s = suite_create("util");
@@ -733,6 +823,7 @@ Suite *util_suite(void)
tcase_add_test (tc_util, test_parse_args);
tcase_add_test (tc_util, test_add_string_to_list);
tcase_add_test (tc_util, test_string_in_list);
+ tcase_add_test (tc_util, test_split_on_separator);
tcase_set_timeout(tc_util, 60);
TCase *tc_utf8 = tcase_create("utf8");
diff --git a/src/util/util.c b/src/util/util.c
index ab980775a1e4c87b16d32220bccda6cb644e0756..b035e2319f22bc692cd9fc8c83ef5f04b39610e8 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -27,118 +27,97 @@
#include "util/sss_utf8.h"
#include "dhash.h"
-/* split a string into an allocated array of strings.
- * 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;
- char **list, **r;
- const char sep_str[2] = { sep, '\0'};
+ int ret;
+ const char *substr_end = str;
+ const char *substr_begin = str;
+ const char *sep_pos = NULL;
+ size_t substr_len;
+ char **list = NULL;
+ int num_strings = 0;
+ TALLOC_CTX *tmp_ctx = NULL;
- if (!str || !*str || !_list) return EINVAL;
-
- t = str;
+ if (str == NULL || *str == '\0' || _list == NULL) {
+ return EINVAL;
+ }
- list = NULL;
- l = 0;
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
- /* trim leading whitespace */
- if (trim)
- while (isspace(*t)) t++;
+ do {
+ substr_len = 0;
- /* find substrings separated by the separator */
- while (t && (p = strpbrk(t, sep_str))) {
- len = p - t;
- n = p + 1; /* save next string starting point */
- if (trim) {
- /* strip whitespace after the separator
- * so it's not in the next token */
- while (isspace(*t)) {
- t++;
- len--;
- if (len == 0) break;
- }
- p--;
- /* strip whitespace before the separator
- * so it's not in the current token */
- while (len > 0 && (isspace(*p))) {
- len--;
- p--;
- }
+ /* If this is not the first substring, then move from the separator. */
+ if (sep_pos != NULL) {
+ substr_end = sep_pos + 1;
+ substr_begin = sep_pos + 1;
}
- /* Add the token to the array, +2 b/c of the trailing NULL */
- r = talloc_realloc(mem_ctx, list, char *, l + 2);
- if (!r) {
- talloc_free(list);
- return ENOMEM;
- } else {
- list = r;
+ /* Find end of the first substring */
+ while (*substr_end != sep && *substr_end != '\0') {
+ substr_end++;
+ substr_len++;
}
- if (len == 0) {
- list[l] = talloc_strdup(list, "");
- } else {
- list[l] = talloc_strndup(list, t, len);
- }
- if (!list[l]) {
- talloc_free(list);
- return ENOMEM;
- }
- l++;
+ sep_pos = substr_end;
- t = n; /* move to next string */
- }
+ if (trim) {
+ /* Trim leading whitespace */
+ while (isspace(*substr_begin) && substr_begin < substr_end) {
+ substr_begin++;
+ substr_len--;
+ }
- /* Handle the last remaining token */
- if (t) {
- r = talloc_realloc(mem_ctx, list, char *, l + 2);
- if (!r) {
- talloc_free(list);
- return ENOMEM;
- } else {
- list = r;
+ /* Trim trailing whitespace */
+ while (substr_end - 1 > substr_begin && isspace(*(substr_end-1))) {
+ substr_end--;
+ substr_len--;
+ }
}
- if (trim) {
- /* trim leading whitespace */
- len = strlen(t);
- while (isspace(*t)) {
- t++;
- len--;
- if (len == 0) break;
- }
- /* trim trailing whitespace */
- p = t + len - 1;
- while (len > 0 && (isspace(*p))) {
- len--;
- p--;
+ /* Copy the substring to the output list of strings */
+ if (skip_empty == false || substr_len > 0) {
+ list = talloc_realloc(tmp_ctx, list, char*, num_strings + 2);
+ if (list == NULL) {
+ ret = ENOMEM;
+ goto done;
}
- if (len == 0) {
- list[l] = talloc_strdup(list, "");
- } else {
- list[l] = talloc_strndup(list, t, 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;
}
- } else {
- list[l] = talloc_strdup(list, t);
+ num_strings++;
}
- if (!list[l]) {
- talloc_free(list);
- return ENOMEM;
+
+ } while (*sep_pos != '\0');
+
+ if (list == NULL) {
+ /* No allocations were done, make space for the NULL */
+ list = talloc(tmp_ctx, char *);
+ if (list == NULL) {
+ ret = ENOMEM;
+ goto done;
}
- l++;
}
+ list[num_strings] = NULL;
- list[l] = NULL; /* terminate list */
-
- if (size) *size = l;
- *_list = list;
+ if (size) {
+ *size = num_strings;
+ }
- return EOK;
+ *_list = talloc_steal(mem_ctx, list);
+ ret = EOK;
+done:
+ talloc_free(tmp_ctx);
+ return ret;
}
static void free_args(char **args)
diff --git a/src/util/util.h b/src/util/util.h
index c15ca668392105447d073c40666953a0145d375a..e4cb1a8658203f7357142a9601727613cb4462ec 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -476,7 +476,8 @@ errno_t check_and_open_readonly(const char *filename, int *fd, const uid_t uid,
/* from util.c */
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);
char **parse_args(const char *str);
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
