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

Michal Židek mzidek at redhat.com
Fri Oct 2 13:34:30 UTC 2015


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Update-get_next_domain-s-interface.patch
Type: text/x-patch
Size: 53697 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151002/751fd131/attachment-0003.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/20151002/751fd131/attachment-0004.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: 910 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151002/751fd131/attachment-0005.bin>


More information about the sssd-devel mailing list