On Tue, Jun 02, 2015 at 08:31:56AM +0200, Lukas Slebodnik wrote:
On (01/06/15 20:56), Jakub Hrozek wrote:
>There is a strange construct in our sss_domain_info handling code:
> while (dom && dom->disabled) {
> dom = get_next_domain(dom, true);
> }
>
>I wasn't sure if this code was valid at all, so I wrote a test. Turns
>out the code is semi-valid -- if the domain you pass in is disabled,
>which is technically possible. It could be shortened to "if" instead of
>"while", but meh.
>
>At least we have one more test :-)
>From a622e79e7ffd02a5035ce9e61c42f7079c52c054 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Tue, 26 May 2015 21:11:43 +0200
>Subject: [PATCH] TESTS: Add test for get_next_domain
>
>---
> src/tests/cmocka/test_utils.c | 138 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 138 insertions(+)
>
>diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c
>index
0cfbaa5043e2a36551208b9da23d7423a35e60a4..5f0ca117688246844450c29f1a3182200725f6ae 100644
>--- a/src/tests/cmocka/test_utils.c
>+++ b/src/tests/cmocka/test_utils.c
>@@ -454,6 +454,137 @@ void test_find_domain_by_sid_disabled(void **state)
> }
> }
>
>+static struct sss_domain_info *named_domain(TALLOC_CTX *mem_ctx,
>+ const char *name,
>+ struct sss_domain_info *parent)
>+{
>+ struct sss_domain_info *dom = NULL;
>+
>+ dom = talloc_zero(mem_ctx, struct sss_domain_info);
>+ assert_non_null(dom);
>+
>+ dom->name = talloc_strdup(dom, name);
>+ assert_non_null(dom->name);
>+
>+ dom->parent = parent;
>+
>+ return dom;
>+}
>+
>+/*
>+ * dom1 -> sub1a
>+ * |
>+ * dom2 -> sub2a -> sub2b
>+ *
>+ */
>+static int setup_dom_tree(void **state)
>+{
>+ struct dom_list_test_ctx *test_ctx;
>+ struct sss_domain_info *head = NULL;
>+ struct sss_domain_info *dom = NULL;
>+
>+ assert_true(leak_check_setup());
>+
>+ test_ctx = talloc_zero(global_talloc_context, struct dom_list_test_ctx);
>+ assert_non_null(test_ctx);
>+
>+ dom = named_domain(test_ctx, "dom1", NULL);
>+ assert_non_null(dom);
>+ head = dom;
>+
>+ dom = named_domain(test_ctx, "sub1a", head);
>+ assert_non_null(dom);
>+ head->subdomains = dom;
>+
>+ dom = named_domain(test_ctx, "dom2", NULL);
>+ assert_non_null(dom);
>+ head->next = dom;
>+
>+ dom = named_domain(test_ctx, "sub2a", head->next);
>+ assert_non_null(dom);
>+ head->next->subdomains = dom;
>+
>+ dom = named_domain(test_ctx, "sub2b", head->next);
>+ assert_non_null(dom);
>+ head->next->subdomains->next = dom;
>+
>+ test_ctx->dom_count = 2;
>+ test_ctx->dom_list = head;
>+
>+ check_leaks_push(test_ctx);
>+ *state = test_ctx;
>+ return 0;
>+}
>+
>+static int teardown_dom_tree(void **state)
>+{
>+ struct dom_list_test_ctx *test_ctx = talloc_get_type(*state,
>+ struct dom_list_test_ctx);
>+ if (test_ctx == NULL) {
>+ DEBUG(SSSDBG_CRIT_FAILURE, "Type mismatch\n");
>+ return 1;
>+ }
>+
>+ assert_true(check_leaks_pop(test_ctx) == true);
check_leaks_pop already return boolean.
So you needn't copare to true.
OK, fixed.
>+ talloc_free(test_ctx);
>+ assert_true(leak_check_teardown());
>+ return 0;
>+}
>+
>+static void test_get_next_domain(void **state)
>+{
>+ struct dom_list_test_ctx *test_ctx = talloc_get_type(*state,
>+ struct dom_list_test_ctx);
>+ struct sss_domain_info *dom = NULL;
>+
>+ dom = get_next_domain(test_ctx->dom_list, false);
>+ assert_non_null(dom);
>+ assert_string_equal(dom->name, "dom2");
>+
>+ dom = get_next_domain(dom, false);
>+ assert_null(dom);
>+}
>+
>+static void test_get_next_domain_descend(void **state)
>+{
>+ struct dom_list_test_ctx *test_ctx = talloc_get_type(*state,
>+ struct dom_list_test_ctx);
>+ struct sss_domain_info *dom = NULL;
>+
>+ dom = get_next_domain(test_ctx->dom_list, true);
>+ assert_non_null(dom);
>+ assert_string_equal(dom->name, "sub1a");
>+
>+ dom = get_next_domain(dom, true);
>+ assert_non_null(dom);
>+ assert_string_equal(dom->name, "dom2");
>+
>+ dom = get_next_domain(dom, true);
>+ assert_non_null(dom);
>+ assert_string_equal(dom->name, "sub2a");
>+
>+ dom = get_next_domain(dom, true);
>+ assert_non_null(dom);
>+ assert_string_equal(dom->name, "sub2b");
>+
>+ dom = get_next_domain(dom, false);
>+ assert_null(dom);
>+}
>+
>+static void test_get_next_domain_disabled(void **state)
>+{
>+ struct dom_list_test_ctx *test_ctx = talloc_get_type(*state,
>+ struct dom_list_test_ctx);
>+ struct sss_domain_info *dom = NULL;
>+
>+ for (dom = test_ctx->dom_list; dom; dom = get_next_domain(dom, true)) {
>+ dom->disabled = true;
>+ }
it would be good to have more test with combination of some disabled domains
and with or without subdomain.
There I have a question.
If main domain is disabled should we return its enabled subdomains?
Does it make sense?
I'm not sure. So far the main domain cannot be disabled, only offline.
Only subdomains can be disabled when processing newly updated subdomain
list. I also think the disabled flag should only be readable by the
domain utility module and maybe the subdomains code, not the deamon.
I think this question will be more valid once we start supporting drop-in
configurations. Then, during processing I guess we would disable even main
existing domains and start enabling the new ones.
But I'm not sure we need to solve this problem now, maybe we can add a
FIXME to the test?