On Thu, Jul 18, 2013 at 03:45:35PM +0200, Pavel Březina wrote:
On 07/18/2013 02:38 PM, Pavel Březina wrote:
>Hi,
>this is the first unit test for nested groups. It covers only the most
>basic situation when we are trying to resolve one group with no members.
>
>Even though it is only one test, the patch set is quite big. This is
>because it creates the possibility to mock providers related modules.
>Most of the patches are just a preparation for unit testing providers.
>
>Patches 1-5
>Moves the code around to reduce number of dependencies. (E.g. you do not
>want to load fail over when you are testing nested groups.)
>
>Patch 6
>Mocks basic SDAP interface.
>
>Patch 7
>Mocks sysdb objects - currently user and rfc2307bis group. You can
>decide what set of attributes the object should posses. For example,
>creating a user requires only basedn and name parameter, to construct
>originalDN and name attributes. The rest is provided by (attrname,
>value) pairs via variadic function.
>
>E.g.:
>mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
>
>get_attr_type() translates the sysdb attribute name to proper data type.
>This should be extended as needed.
>
>Patch 8
>Adds provider tests related common object files and cflags in makefile
>
>Patch 9
>New macro sss_will_return_always(fn, value). This can be used to mock
>function data in such way that any call of mock() will return the value.
>
>It was just pushed also to cmocka upstream as will_return_always().
>
>Patch 10-11
>Unit test.
>
>Patch 12
>Removes a noisy debug message.
>
>I would like to get this reviewed before I continue with more test
>cases, so the framework is tuned enough.
>
>Off topic:
>I also created new macro called fail_msg, which will make the test fail
>printing a message. I didn't use this macro in the end, but it made its
>way to cmocka upstream.
Oops, I did some reorganization while writing this mail but sent
patches generated sooner, so the numbers does not match. Reordered
patches are attached.
The patches no longer apply (not suprising after such a long time on the
list..)
I will add some quick comments inline without trying out the patches.
From 9b370ed83ec5c85819781145de92c39977fd19eb Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 16 Jul 2013 11:21:02 +0200
Subject: [PATCH 01/12] sdap: move non async functions from sdap_async.c to
sdap_utils.c
---
Makefile.am | 1 +
src/providers/ldap/sdap_async.c | 107 --------------------------------
src/providers/ldap/sdap_utils.c | 131 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 132 insertions(+), 107 deletions(-)
create mode 100644 src/providers/ldap/sdap_utils.c
diff --git a/Makefile.am b/Makefile.am
index 515843420834bbbd0e5b4c86c114294c89afa752..6cfc363cea9eae833a8a4bdff12309286475b89a
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1450,6 +1450,7 @@ libsss_ldap_common_la_SOURCES = \
src/providers/ldap/sdap_reinit.c \
src/providers/ldap/sdap_dyndns.c \
src/providers/ldap/sdap_refresh.c \
+ src/providers/ldap/sdap_utils.c \
src/providers/ldap/sdap.c
libsss_ldap_common_la_LDFLAGS = \
-avoid-version
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index bdd3c3c8b2ed4cc7246c008ddbfe531b82d880bc..6d29c1a566f82c2ce0dead2ca8073ace15afbf56
100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -23,26 +23,8 @@
#include "util/util.h"
#include "providers/ldap/sdap_async_private.h"
-#define REALM_SEPARATOR '@'
#define REPLY_REALLOC_INCREMENT 10
-void make_realm_upper_case(const char *upn)
This function seems to be called from a single location only. Can we
either ditch it in favor of get_uppercase_realm() from usertools.c or
make it static?
-{
- char *c;
-
- c = strchr(upn, REALM_SEPARATOR);
- if (c == NULL) {
- DEBUG(9, ("No realm delimiter found in upn [%s].\n", upn));
- return;
- }
-
- while(*(++c) != '\0') {
- c[0] = toupper(*c);
- }
-
- return;
-}
-
From 4f76600299c31489c98f960d162ab7c1d2a29126 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 17 Jul 2013 13:20:43 +0200
Subject: [PATCH 02/12] sdap: move non async functions from
sdap_async_connection.c to sdap_utils.c
Looks good to me.
From dab6d9221bafe0e18f599b88de10b7cc57a57b8e Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 17 Jul 2013 14:28:26 +0200
Subject: [PATCH 03/12] sdap: move sdap_get_id_specific_filter() to
sdap_utils.c
I think this makes sense.
Since we started talking about LDAP provider refactoring on IRC
recently, I would like to bring up the ldap_*.c vs. sdap_*.c confusion.
In my opinion, we should separate libsss_ldap.la as discussed on #sssd
the other day and keep its sources in sdap*.c. Only code specific to the
initialization of the LDAP provider should be kept in ldap*.c
From a97267f35a598e7f944cf993b37cd6b43db7a572 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 17 Jul 2013 13:50:34 +0200
Subject: [PATCH 04/12] ldap: move options related content from ldap_common.c
to ldap_options.c
---
src/providers/ldap/ldap_common.c | 886 --------------------
.../ldap/{ldap_common.c => ldap_options.c} | 921 ---------------------
2 files changed, 1807 deletions(-)
copy src/providers/ldap/{ldap_common.c => ldap_options.c} (53%)
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index 5782c4615ddd3778d16e52986eae065c3c61b8e5..7a8bf93d2366a517e545f1faecdc3aefa2be36e7
100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -39,892 +39,6 @@
/* a fd the child process would log into */
int ldap_child_debug_fd = -1;
-int
-sdap_domain_destructor(void *mem)
-{
- struct sdap_domain *dom =
- talloc_get_type(mem, struct sdap_domain);
- DLIST_REMOVE(*(dom->head), dom);
- return 0;
-}
-
-struct sdap_domain *
-sdap_domain_get(struct sdap_options *opts,
- struct sss_domain_info *dom)
-{
- struct sdap_domain *sditer = NULL;
-
- DLIST_FOR_EACH(sditer, opts->sdom) {
- if (sditer->dom == dom) {
- break;
- }
- }
-
- return sditer;
-}
-
-errno_t
-sdap_domain_add(struct sdap_options *opts,
- struct sss_domain_info *dom,
- struct sdap_domain **_sdom)
-{
- struct sdap_domain *sdom;
-
- sdom = talloc_zero(opts, struct sdap_domain);
- if (sdom == NULL) {
- return ENOMEM;
- }
- sdom->dom = dom;
- sdom->head = &opts->sdom;
-
- if (opts->sdom) {
- /* Only allow subdomains of the parent domain */
- if (dom->parent == NULL ||
- dom->parent != opts->sdom->dom) {
- DEBUG(SSSDBG_OP_FAILURE, ("Domain %s is not a subdomain of %s\n",
- dom->name, opts->sdom->dom->name));
- return EINVAL;
- }
- }
-
- talloc_set_destructor((TALLOC_CTX *)sdom, sdap_domain_destructor);
- DLIST_ADD_END(opts->sdom, sdom, struct sdap_domain *);
-
- if (_sdom) *_sdom = sdom;
- return EOK;
-}
-
-errno_t
-sdap_domain_subdom_add(struct sdap_id_ctx *sdap_id_ctx,
- struct sdap_domain *sdom_list,
- struct sss_domain_info *parent)
-{
- struct sss_domain_info *dom;
- struct sdap_domain *sdom, *sditer;
- char *basedn;
- errno_t ret;
-
- for (dom = get_next_domain(parent, true);
- dom && IS_SUBDOMAIN(dom); /* if we get back to a parent, stop */
- dom = get_next_domain(dom, false)) {
-
- DLIST_FOR_EACH(sditer, sdom_list) {
- if (sditer->dom == dom) {
- break;
- }
- }
-
- if (sditer == NULL) {
- /* New sdap domain */
- DEBUG(SSSDBG_TRACE_FUNC, ("subdomain %s is a new one, will "
- "create a new sdap domain object\n", dom->name));
-
- ret = sdap_domain_add(sdap_id_ctx->opts, dom, &sdom);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE,
- ("Cannot add new sdap domain for domain %s [%d]: %s\n",
- parent->name, ret, strerror(ret)));
- return ret;
- }
- } else {
- sdom = sditer;
- }
-
- /* Convert the domain name into search base */
- ret = domain_to_basedn(sdom, sdom->dom->name, &basedn);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE,
- ("Cannot convert domain name [%s] to base DN [%d]: %s\n",
- dom->name, ret, strerror(ret)));
- talloc_free(basedn);
- return ret;
- }
-
- /* Update search bases */
- talloc_zfree(sdom->search_bases);
- sdom->search_bases = talloc_array(sdom, struct sdap_search_base *, 2);
- if (sdom->search_bases == NULL) {
- return ret;
- }
- sdom->search_bases[1] = NULL;
-
- ret = sdap_create_search_base(sdom, basedn, LDAP_SCOPE_SUBTREE, NULL,
- &sdom->search_bases[0]);
- talloc_free(basedn);
- if (ret) {
- DEBUG(SSSDBG_OP_FAILURE, ("Cannot create new sdap search
base\n"));
- return ret;
- }
-
- sdom->user_search_bases = sdom->search_bases;
- sdom->group_search_bases = sdom->search_bases;
- sdom->netgroup_search_bases = sdom->search_bases;
- sdom->sudo_search_bases = sdom->search_bases;
- sdom->service_search_bases = sdom->search_bases;
- sdom->autofs_search_bases = sdom->search_bases;
- }
-
- return EOK;
-}
-
-void
-sdap_domain_remove(struct sdap_options *opts,
- struct sss_domain_info *dom)
-{
- struct sdap_domain *sdom;
-
- sdom = sdap_domain_get(opts, dom);
- if (sdom == NULL) return;
-
- DLIST_REMOVE(*(sdom->head), sdom);
-}
-
Would it make sense to separate sdap_domain_* functions to an sdap
module instead?
Moving ldap_get_$provider_options makes sense to me.
From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 17 Jul 2013 13:51:37 +0200
Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip when unit
testing
I agree with Lukas, I would prefer not to add ifdefs to the code unless
needed.
From d1e5b04f0cc7ece9bf7fccd564626e488d532951 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 26 Mar 2013 15:07:17 +0100
Subject: [PATCH 06/12] tests: mock SDAP
Looks good to me.
From e64bce82249f1b8be7bd5eb8bd4fd01d7a5fb5dd Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 26 Mar 2013 15:18:21 +0100
Subject: [PATCH 07/12] tests: mock sysdb users and groups
I think the approach is fine for unit tests. I don't see us change the
types of sysdb attributes frequently.
From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 17 Jul 2013 13:58:54 +0200
Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
tests
Looks good to me.
From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 18 Jul 2013 13:24:26 +0200
Subject: [PATCH 09/12] tests: new macro sss_will_return_always
Is this feature available in all cmocka releases (IIRC you wrote it) ?
From 65da81b7fe5418f44ec743fc11520c5e68a85435 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 26 Mar 2013 15:06:44 +0100
Subject: [PATCH 10/12] tests: add confdb_path to sss_test_ctx
Looks good to me.
From 03e318c1319f8b3545195d7deee3d55793fc941a Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 17 Jul 2013 13:59:36 +0200
Subject: [PATCH 11/12] tests: nested groups unit test
I only scrolled through this one, but in general looks good.
From 53ff2f0812b6ae81642513aee693284ad134ba47 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 18 Jul 2013 14:05:05 +0200
Subject: [PATCH 12/12] tests: don't print debug message when test dir does
not exist
Looks good to me.