On Fri, Oct 09, 2015 at 03:14:52PM +0200, Michal Židek wrote:
On 10/09/2015 02:10 PM, Jakub Hrozek wrote:
>On Fri, Oct 02, 2015 at 03:34:30PM +0200, Michal Židek wrote:
>>On 10/02/2015 07:44 AM, Lukas Slebodnik wrote:
>>>On (01/10/15 15:41), Michal Židek wrote:
>>>>On 10/01/2015 03:08 PM, Lukas Slebodnik wrote:
>>>>>On (01/10/15 13:56), Michal Židek wrote:
>>>>>>On 09/16/2015 03:56 PM, Michal Židek wrote:
>>>>>>>On 09/15/2015 04:03 PM, Jakub Hrozek wrote:
>>>>>>>>On Tue, Sep 15, 2015 at 03:53:41PM +0200, Michal Židek
wrote:
>>>>>>>>>On 09/14/2015 05:43 PM, Jakub Hrozek wrote:
>>>>>>>>>>On Wed, Sep 09, 2015 at 02:43:59PM +0200, Michal
Židek wrote:
>>>>>>>>>>>Hi,
>>>>>>>>>>>
>>>>>>>>>>>patch for ticket
>>>>>>>>>>>https://fedorahosted.org/sssd/ticket/2673
>>>>>>>>>>>is in the attachment.
>>>>>>>>>>>
>>>>>>>>>>>Thanks.
>>>>>>>>>>>Michal
>>>>>>>>>>
>>>>>>>>>>> From
7c454bc2a737be05068418a5eef7fe9446bb5fa8 Mon Sep 17 00:00:00
>>>>>>>>>>>2001
>>>>>>>>>>>From: =?UTF-8?q?Michal=20=C5=BDidek?=
<mzidek(a)redhat.com>
>>>>>>>>>>>Date: Wed, 9 Sep 2015 14:37:48 +0200
>>>>>>>>>>>Subject: [PATCH] util: Include disabled
domains in link_forest_roots
>>>>>>>>>>>
>>>>>>>>>>>Ticket:
>>>>>>>>>>>https://fedorahosted.org/sssd/ticket/2673
>>>>>>>>>>>---
>>>>>>>>>>> src/db/sysdb_subdomains.c | 6 +++---
>>>>>>>>>>> src/tests/cmocka/test_utils.c | 3 +++
>>>>>>>>>>> src/util/domain_info_utils.c | 21
++++++++++++++++++---
>>>>>>>>>>> src/util/util.h | 3 +++
>>>>>>>>>>> 4 files changed, 27 insertions(+), 6
deletions(-)
>>>>>>>>>>
>>>>>>>>>>The patch looks good but whenever I see us adding
more and more boolean
>>>>>>>>>>switches, I wonder if we should just use flags
instead?
>>>>>>>>>>
>>>>>>>>>>This:
>>>>>>>>>> get_next_domain_ex(d, GND_USE_DISABLED |
GND_DESCEND);
>>>>>>>>>>Reads quite a bit easier to me than:
>>>>>>>>>> get_next_domain_ex(d, true, false);
>>>>>>>>>>
>>>>>>>>>>Also, bonus point are acquired next time we add a
new flag, because not
>>>>>>>>>>all callers of the function must be converted..
>>>>>>>>>>
>>>>>>>>>>What do you think?
>>>>>>>>>
>>>>>>>>>I think this is very good point. It will also give us
implicit
>>>>>>>>>boolean parameter value (if not set, it is
automatically false)
>>>>>>>>>which improves readability a lot. I wander if it is
good to add
>>>>>>>>>the _ex function in this case. Would you agree if I
changed the
>>>>>>>>>original get_next_domain to use flags? I know, it
needs to be changed
>>>>>>>>>on more places, but I think such small refactoring
makes more
>>>>>>>>>sence than adding _ex function with flags.
>>>>>>>>>
>>>>>>>>>Or is this what you were saying? I am not sure
because
>>>>>>>>>you used the _ex function in your example of
"nice"
>>>>>>>>>usage.
>>>>>>>>
>>>>>>>>If we have a test for different usages of the flag-based
function, then
>>>>>>>>I guess it would be better than keeping the existing
get_next_domain
>>>>>>>>function.
>>>>>>>>
>>>>>>>>We can also split master from sssd-1-13 already..
>>>>>>>>
>>>>>>>>btw the flag names were completely pulled out of thin
air, feel free to
>>>>>>>>suggest better ones.
>>>>>>>
>>>>>>>I split the refactoring of get_next_domain to separate
patch.
>>>>>>>The second patch is just one liner that switches on the
>>>>>>>disabled domains in link_forest_roots.
>>>>>>>
>>>>>>>Michal
>>>>>>>
>>>>>> ____ _ _ __ __ ____
>>>>>>__/\_| __ )| | | | \/ | _ \__/\__
>>>>>>\ / _ \| | | | |\/| | |_) \ /
>>>>>>/_ _\ |_) | |_| | | | | __//_ _\
>>>>>> \/ |____/ \___/|_| |_|_| \/
>>>>>>
>>>>>>(low priority, just keeping the thread alive :) )
>>>>>Would you mind to write unit tests for get_next_domain?
>>>>>
>>>>>LS
>>>>
>>>>It does have unit tests.
>>>>
>>>Let me rephrase your answer.
>>>The tests has beed changed.
>>>
>>>But new functionality is not test.
>>>There is a change in test_utils.s
>>>>- dom = get_next_domain(test_ctx->dom_list, true);
>>>>+ dom = get_next_domain(test_ctx->dom_list, SSS_GND_DESCEND);
>>>> assert_null(dom);
>>>>+
>>>>+ dom = get_next_domain(test_ctx->dom_list,
>>>>+ SSS_GND_DESCEND | SSS_GND_INCLUDE_DISABLED);
>>>>+ assert_non_null(dom);
>>>But assert_non_null cannot be considered as a valid test for new feature.
>>>
>>>So would you mind to extend tests for SSS_GND_INCLUDE_DISABLED?
>>>It ould be god to se what happed if SSS_GND_INCLUDE_DISABLED is used without
>>>the flag SSS_GND_DESCEND. Or which domain will be returned in different
cases.
>>>
>>>You should consider a new unit test as an additional documentation.
>>>So if new developer will not be sure hot to use it it will be obvious from
>>>unit test. This is a reason why it might be better to write separate
>>>test for new feature in separate patch. Because the current patch
>>>mostly convert old code to the new style 's/true/SSS_GND_DESCEND/'
's/false/0/'
>>>(In most cases, I prefer unit test to be part of new small features but
>>>sometimes it's better to write unit test in separate patch.
>>>
>>>LS
>>
>>Ok. I removed the test changes from the first patch and added
>>enhanced tests in the new patch.
>>
>>Michal
>>
>
>Can you rebase the patches atop master, please?
>
>I will then review them. But from casual scrolling from the patches, the
>changes look OK to me.
Thank you, rebased patches are attached.
Michal
From 30af59fc71f09cbc613380df9d5c62b5c0f9a8b5 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
Date: Wed, 9 Sep 2015 14:37:48 +0200
Subject: [PATCH 1/3] util: Update get_next_domain's interface
Update get next domain to be able to
include disbled domains and change the
interface to accept flags instead of
multiple booleans.
Ticket:
https://fedorahosted.org/sssd/ticket/2673
diff --git a/src/responder/common/responder_cache_req.c
b/src/responder/common/responder_cache_req.c
index ab73401..3c93492 100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -983,7 +983,7 @@ static errno_t cache_req_next_domain(struct tevent_req *req)
while (state->domain != NULL && state->check_next
&& state->domain->fqnames
&& !cache_req_input_is_upn(state->input)) {
- state->domain = get_next_domain(state->domain, false);
+ state->domain = get_next_domain(state->domain, SSS_GND_DESCEND);
Looks like there is a behaviour change here, previously we used false,
now we use descend..
Is it intentional?
From df78c59a241241b9586f5d701e8a789d777bfb21 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
Date: Fri, 2 Oct 2015 14:26:02 +0200
Subject: [PATCH 2/3] tests: Add get_next_domain_flags test
ACK
From 44518ccbf53aff5f8bc7c8d51aa6067321253431 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
Date: Wed, 16 Sep 2015 15:33:10 +0200
Subject: [PATCH 3/3] sysdb: Include disabled domains in link_forest_roots
ACK
Can you also change sysdb_update_subdomains() so that instead of:
/* explicitly use dom->next as we need to check 'disabled' domains */
for (dom = domain->subdomains; dom; dom = dom->next) {
it calls get_next_domain with the disabled flag now?