On 01/31/2014 02:37 PM, Pavel Březina wrote:
On 11/05/2013 12:22 PM, Jakub Hrozek wrote:
> 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.
Rebased version with all issues addressed is attached.
Rebased again atop Nikolai's DEBUG patches. I hope it is the last time :-)