[SSSD] [PATCH] util: Include disabled domains in link_forest_roots

Michal Židek mzidek at redhat.com
Tue Oct 13 12:56:35 UTC 2015


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 at 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 at 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.

>>  From df78c59a241241b9586f5d701e8a789d777bfb21 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at 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 at 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.

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.

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Update-get_next_domain-s-interface.patch
Type: text/x-patch
Size: 54400 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/217eb439/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-tests-Add-get_next_domain_flags-test.patch
Type: text/x-patch
Size: 5351 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/217eb439/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sysdb-Include-disabled-domains-in-link_forest_roots.patch
Type: text/x-patch
Size: 887 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/217eb439/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-sysdb-Use-get_next_domain-instead-of-dom-next.patch
Type: text/x-patch
Size: 1170 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151013/217eb439/attachment-0007.bin>


More information about the sssd-devel mailing list