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.