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

Michal Židek mzidek at redhat.com
Wed Sep 16 13:56:04 UTC 2015


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Update-get_next_domain-s-interface.patch
Type: text/x-patch
Size: 53680 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150916/98f2ecac/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-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/20150916/98f2ecac/attachment-0003.bin>


More information about the sssd-devel mailing list