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

Jakub Hrozek jhrozek at redhat.com
Fri Oct 9 12:10:07 UTC 2015


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.


More information about the sssd-devel mailing list