On Tue, Oct 13, 2015 at 02:56:35PM +0200, Michal Židek wrote:
On 10/12/2015 10:31 AM, Jakub Hrozek wrote:
>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?
>
It was not intentional. Thanks for catching this.
ACK
>> 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?
Ok, but I added it in a separate patch, because it did not fit
in any of the existing ones IMO.
Fine.
I also did one small change for convenience. I added new
macro that is combination of SSS_GND_DESCEND and
SSS_GND_INCLUDE_DISABLED which is called in SSS_GND_ALL_DOMAINS,
to avoid typing for the case when we want to include both
subdomains and disabled domains. For now it is only used
on one place (the third patch), but I think it will be
useful in the future.
Fine by me as well.
From 253cc65b8ff3e5509e891f29a29b0196d2ac68fe Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
Date: Tue, 13 Oct 2015 14:25:08 +0200
Subject: [PATCH 4/4] sysdb: Use get_next_domain instead of dom->next
ACK
I'll just wait for CI and Coverity before pushing.