On Mon, 2012-06-11 at 13:35 +0200, Jan Zelený wrote: > > Sending patches in two parts. These first five are (I believe) ready for > > a complete review. I will send three more in a [PRELIMINARY] thread as > > well, since they require some discussion. > > > > > > Patch 0001: Fix the debug levels for some sysdb user and group lookups. > > Success was too noisy and failure was not noisy enough. > > Ack > > > Patch 0002: Remove block of code that appears twice in the same > > function. Looks like a copy-paste error. > > Nack, > there is another block of code just below which should be removed as well > > > Patch 0003: Fix incorrect switch statement in sdap_get_initgr_done() > > SDAP_SCHEMA_AD needs to be calling sdap_initgr_rfc2307bis_recv(), > > not sdap_initgr_nested_recv(). By coincidence both recv functions > > happened to be identical, but if one or the other changed, this > > would break unexpectedly. > > Ack, > > > Patch 0004: Add helper function to get list of a user's groups from > > sysdb > > This is being made into a function so it can be reused in one of the AD > > patches I'll be sending shortly. > > Nack, > the first DEBUG string in get_sysdb_grouplist() is missing one argument > > > Patch 0005: Make sdap_initgr_common_store() non-static > > Move it to a private header so it can be reused by other > > initgroups C files. This is being made public so it can be reused in my > > AD patches. > > Ack
Thanks for the review. All comments addressed in the attached patches.
From bcba1c02d1ca9066d9ff66226b3adaa1f75cd1fa Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Fri, 8 Jun 2012 13:10:01 -0400
Subject: [PATCH 1/8] SYSDB: Reduce noise level of debug messages in lookups
---
src/db/sysdb_ops.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 2d2244bb3abda3b0a71203bd7648a45975cde513..528b84f1ae7fd9d075a6364e878fa1a644c2cae6 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2252,7 +2252,8 @@ int sysdb_search_users(TALLOC_CTX *mem_ctx,
goto fail;
}
- DEBUG(6, ("Search users with filter: %s\n", filter));
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("Search users with filter: %s\n", filter));
ret = sysdb_search_entry(mem_ctx, sysdb, basedn,
LDB_SCOPE_SUBTREE, filter, attrs,
@@ -2266,10 +2267,10 @@ int sysdb_search_users(TALLOC_CTX *mem_ctx,
fail:
if (ret == ENOENT) {
- DEBUG(SSSDBG_TRACE_FUNC, ("No such entry\n"));
+ DEBUG(SSSDBG_TRACE_INTERNAL, ("No such entry\n"));
}
else if (ret) {
- DEBUG(SSSDBG_TRACE_FUNC, ("Error: %d (%s)\n", ret, strerror(ret)));
+ DEBUG(SSSDBG_MINOR_FAILURE, ("Error: %d (%s)\n", ret, strerror(ret)));
}
talloc_zfree(tmp_ctx);
return ret;
@@ -2410,7 +2411,8 @@ int sysdb_search_groups(TALLOC_CTX *mem_ctx,
goto fail;
}
- DEBUG(6, ("Search groups with filter: %s\n", filter));
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("Search groups with filter: %s\n", filter));
ret = sysdb_search_entry(mem_ctx, sysdb, basedn,
LDB_SCOPE_SUBTREE, filter, attrs,
@@ -2424,10 +2426,10 @@ int sysdb_search_groups(TALLOC_CTX *mem_ctx,
fail:
if (ret == ENOENT) {
- DEBUG(SSSDBG_TRACE_FUNC, ("No such entry\n"));
+ DEBUG(SSSDBG_TRACE_INTERNAL, ("No such entry\n"));
}
else if (ret) {
- DEBUG(SSSDBG_TRACE_FUNC, ("Error: %d (%s)\n", ret, strerror(ret)));
+ DEBUG(SSSDBG_MINOR_FAILURE, ("Error: %d (%s)\n", ret, strerror(ret)));
}
talloc_zfree(tmp_ctx);
return ret;
--
1.7.10.2
From 23970df9a94663b9758ef3181206c66b329e23fd Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Wed, 6 Jun 2012 08:34:27 -0400
Subject: [PATCH 2/8] LDAP: Remove redundant check
The same block appeared earlier in the function and neither
variable could have changed values since.
---
src/providers/ldap/sdap_async_groups.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index 2a079228ce778f185545bb76cc897aa81de98ad6..70e656bd7ad4619ea2521369643c6f302aede02a 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -1409,17 +1409,6 @@ static void sdap_get_groups_process(struct tevent_req *subreq)
state->groups[state->count] = NULL;
}
- if (!state->enumeration && count > 1) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("Individual group search returned multiple results\n"));
- tevent_req_error(req, EINVAL);
- return;
- }
-
- if (state->enumeration || count == 0) {
- next_base = true;
- }
-
if (next_base) {
state->base_iter++;
if (state->search_bases[state->base_iter]) {
--
1.7.10.2
From e9f4f12ed60790dc68879dc0769cd0c399b86a05 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Sun, 10 Jun 2012 13:34:39 -0400
Subject: [PATCH 3/8] LDAP: Fix incorrect switch statement in
sdap_get_initgr_done()
SDAP_SCHEMA_AD needs to be calling sdap_initgr_rfc2307bis_recv(),
not sdap_initgr_nested_recv(). By coincidence both recv functions
happened to be identical, but if one or the other changed, this
would break unexpectedly.
---
src/providers/ldap/sdap_async_initgroups.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index 50347b2bf8e6e6fd3cd4d1b255f2a7b76f871f36..ae7e63b874f335289c7add0cca5a7f623840f751 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -2760,11 +2760,11 @@ static void sdap_get_initgr_done(struct tevent_req *subreq)
break;
case SDAP_SCHEMA_RFC2307BIS:
+ case SDAP_SCHEMA_AD:
ret = sdap_initgr_rfc2307bis_recv(subreq);
break;
case SDAP_SCHEMA_IPA_V1:
- case SDAP_SCHEMA_AD:
ret = sdap_initgr_nested_recv(subreq);
break;
--
1.7.10.2
From c4695b933047a4f84f797f7a0a632423d63147a8 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Sun, 10 Jun 2012 14:44:32 -0400
Subject: [PATCH 4/8] LDAP: Add helper function to get list of a user's groups
from sysdb
---
src/providers/ldap/sdap_async_initgroups.c | 97 ++++++++++++++++++----------
src/providers/ldap/sdap_async_private.h | 5 ++
2 files changed, 69 insertions(+), 33 deletions(-)
diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index ae7e63b874f335289c7add0cca5a7f623840f751..34a8e36a393984a3f1ca5a123e69b1799643f5cd 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -444,10 +444,7 @@ static void sdap_initgr_rfc2307_process(struct tevent_req *subreq)
struct sdap_initgr_rfc2307_state *state;
struct sysdb_attrs **ldap_groups;
char **sysdb_grouplist = NULL;
- struct ldb_message *msg;
- struct ldb_message_element *groups;
size_t count;
- const char *attrs[2];
int ret;
int i;
@@ -499,41 +496,13 @@ static void sdap_initgr_rfc2307_process(struct tevent_req *subreq)
}
/* Search for all groups for which this user is a member */
- attrs[0] = SYSDB_MEMBEROF;
- attrs[1] = NULL;
-
- ret = sysdb_search_user_by_name(state, state->sysdb, state->name,
- attrs, &msg);
+ ret = get_sysdb_grouplist(state, state->sysdb, state->name,
+ &sysdb_grouplist);
if (ret != EOK) {
tevent_req_error(req, ret);
return;
}
- groups = ldb_msg_find_element(msg, SYSDB_MEMBEROF);
- if (!groups || groups->num_values == 0) {
- /* No groups for this user in sysdb currently */
- sysdb_grouplist = NULL;
- } else {
- sysdb_grouplist = talloc_array(state, char *, groups->num_values+1);
- if (!sysdb_grouplist) {
- tevent_req_error(req, ENOMEM);
- return;
- }
-
- /* Get a list of the groups by groupname only */
- for (i=0; i < groups->num_values; i++) {
- ret = sysdb_group_dn_name(state->sysdb,
- sysdb_grouplist,
- (const char *)groups->values[i].data,
- &sysdb_grouplist[i]);
- if (ret != EOK) {
- tevent_req_error(req, ret);
- return;
- }
- }
- sysdb_grouplist[groups->num_values] = NULL;
- }
-
/* There are no nested groups here so we can just update the
* memberships */
ret = sdap_initgr_common_store(state->sysdb, state->opts,
@@ -2891,3 +2860,65 @@ int sdap_get_initgr_recv(struct tevent_req *req)
return EOK;
}
+errno_t get_sysdb_grouplist(TALLOC_CTX *mem_ctx,
+ struct sysdb_ctx *sysdb,
+ const char *name,
+ char ***grouplist)
+{
+ errno_t ret;
+ const char *attrs[2];
+ struct ldb_message *msg;
+ TALLOC_CTX *tmp_ctx;
+ struct ldb_message_element *groups;
+ char **sysdb_grouplist = NULL;
+ unsigned int i;
+
+ attrs[0] = SYSDB_MEMBEROF;
+ attrs[1] = NULL;
+
+ tmp_ctx = talloc_new(NULL);
+ if (!tmp_ctx) return ENOMEM;
+
+ ret = sysdb_search_user_by_name(tmp_ctx, sysdb, name,
+ attrs, &msg);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error searching user [%s] by name: [%s]\n",
+ name, strerror(ret)));
+ goto done;
+ }
+
+ groups = ldb_msg_find_element(msg, SYSDB_MEMBEROF);
+ if (!groups || groups->num_values == 0) {
+ /* No groups for this user in sysdb currently */
+ sysdb_grouplist = NULL;
+ } else {
+ sysdb_grouplist = talloc_array(tmp_ctx, char *, groups->num_values+1);
+ if (!sysdb_grouplist) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ /* Get a list of the groups by groupname only */
+ for (i=0; i < groups->num_values; i++) {
+ ret = sysdb_group_dn_name(sysdb,
+ sysdb_grouplist,
+ (const char *)groups->values[i].data,
+ &sysdb_grouplist[i]);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not determine group name from [%s]: [%s]\n",
+ (const char *)groups->values[i].data, strerror(ret)));
+ goto done;
+ }
+ }
+ sysdb_grouplist[groups->num_values] = NULL;
+ }
+
+ *grouplist = talloc_steal(mem_ctx, sysdb_grouplist);
+
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h
index f6ed680051d6a7fe8badfdf006e23947547b855a..c107a838628ddf4108e80d08668566f83731abd4 100644
--- a/src/providers/ldap/sdap_async_private.h
+++ b/src/providers/ldap/sdap_async_private.h
@@ -105,4 +105,9 @@ int sdap_save_users(TALLOC_CTX *memctx,
struct sysdb_attrs **users,
int num_users,
char **_usn_value);
+
+errno_t get_sysdb_grouplist(TALLOC_CTX *mem_ctx,
+ struct sysdb_ctx *sysdb,
+ const char *name,
+ char ***grouplist);
#endif /* _SDAP_ASYNC_PRIVATE_H_ */
--
1.7.10.2
From 915d2223d685d92c070a4d52045166a00bd905be Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Sun, 10 Jun 2012 14:49:58 -0400
Subject: [PATCH 5/8] LDAP: Make sdap_initgr_common_store() non-static
Move it to a private header so it can be reused by other
initgroups C files.
---
src/providers/ldap/sdap_async_initgroups.c | 14 +++++++-------
src/providers/ldap/sdap_async_private.h | 8 ++++++++
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index 34a8e36a393984a3f1ca5a123e69b1799643f5cd..8524b1374f5e274b5d74b0d9c430b4844e2bcb08 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -196,13 +196,13 @@ fail:
return ret;
}
-static int sdap_initgr_common_store(struct sysdb_ctx *sysdb,
- struct sdap_options *opts,
- const char *name,
- enum sysdb_member_type type,
- char **sysdb_grouplist,
- struct sysdb_attrs **ldap_groups,
- int ldap_groups_count)
+int sdap_initgr_common_store(struct sysdb_ctx *sysdb,
+ struct sdap_options *opts,
+ const char *name,
+ enum sysdb_member_type type,
+ char **sysdb_grouplist,
+ struct sysdb_attrs **ldap_groups,
+ int ldap_groups_count)
{
TALLOC_CTX *tmp_ctx;
char **ldap_grouplist = NULL;
diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h
index c107a838628ddf4108e80d08668566f83731abd4..c0faab50e25411c2adf44bd5f853f2cc8868fc95 100644
--- a/src/providers/ldap/sdap_async_private.h
+++ b/src/providers/ldap/sdap_async_private.h
@@ -106,6 +106,14 @@ int sdap_save_users(TALLOC_CTX *memctx,
int num_users,
char **_usn_value);
+int sdap_initgr_common_store(struct sysdb_ctx *sysdb,
+ struct sdap_options *opts,
+ const char *name,
+ enum sysdb_member_type type,
+ char **sysdb_grouplist,
+ struct sysdb_attrs **ldap_groups,
+ int ldap_groups_count);
+
errno_t get_sysdb_grouplist(TALLOC_CTX *mem_ctx,
struct sysdb_ctx *sysdb,
const char *name,
--
1.7.10.2
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
