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

Michal Židek mzidek at redhat.com
Thu Oct 1 13:41:31 UTC 2015


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.

Michal



More information about the sssd-devel mailing list