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

Jakub Hrozek jhrozek at redhat.com
Tue Oct 13 15:09:18 UTC 2015


On Tue, Oct 13, 2015 at 04:19:22PM +0200, Jakub Hrozek wrote:
> On Tue, Oct 13, 2015 at 02:56:35PM +0200, Michal Židek wrote:
> > On 10/12/2015 10:31 AM, Jakub Hrozek wrote:
> > >On Fri, Oct 09, 2015 at 03:14:52PM +0200, Michal Židek wrote:
> > >>On 10/09/2015 02:10 PM, Jakub Hrozek wrote:
> > >>>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.
> > >>
> > >>Thank you, rebased patches are attached.
> > >>
> > >>Michal
> > >>
> > >
> > >> From 30af59fc71f09cbc613380df9d5c62b5c0f9a8b5 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 1/3] util: Update get_next_domain's interface
> > >>
> > >>Update get next domain to be able to
> > >>include disbled domains and change the
> > >>interface to accept flags instead of
> > >>multiple booleans.
> > >>
> > >>Ticket:
> > >>https://fedorahosted.org/sssd/ticket/2673
> > >>diff --git a/src/responder/common/responder_cache_req.c b/src/responder/common/responder_cache_req.c
> > >>index ab73401..3c93492 100644
> > >>--- a/src/responder/common/responder_cache_req.c
> > >>+++ b/src/responder/common/responder_cache_req.c
> > >>@@ -983,7 +983,7 @@ static errno_t cache_req_next_domain(struct tevent_req *req)
> > >>          while (state->domain != NULL && state->check_next
> > >>                  && state->domain->fqnames
> > >>                  && !cache_req_input_is_upn(state->input)) {
> > >>-            state->domain = get_next_domain(state->domain, false);
> > >>+            state->domain = get_next_domain(state->domain, SSS_GND_DESCEND);
> > >
> > >Looks like there is a behaviour change here, previously we used false,
> > >now we use descend..
> > >
> > >Is it intentional?
> > >
> > 
> > It was not intentional. Thanks for catching this.
> 
> ACK
> 
> > 
> > >> From df78c59a241241b9586f5d701e8a789d777bfb21 Mon Sep 17 00:00:00 2001
> > >>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
> > >>Date: Fri, 2 Oct 2015 14:26:02 +0200
> > >>Subject: [PATCH 2/3] tests: Add get_next_domain_flags test
> > >
> > >ACK
> > >
> > >> From 44518ccbf53aff5f8bc7c8d51aa6067321253431 Mon Sep 17 00:00:00 2001
> > >>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
> > >>Date: Wed, 16 Sep 2015 15:33:10 +0200
> > >>Subject: [PATCH 3/3] sysdb: Include disabled domains in link_forest_roots
> > >
> > >ACK
> > >
> > >Can you also change sysdb_update_subdomains() so that instead of:
> > >     /* explicitly use dom->next as we need to check 'disabled' domains */
> > >     for (dom = domain->subdomains; dom; dom = dom->next) {
> > >it calls get_next_domain with the disabled flag now?
> > 
> > Ok, but I added it in a separate patch, because it did not fit
> > in any of the existing ones IMO.
> 
> Fine.
> 
> > 
> > I also did one small change for convenience. I added new
> > macro that is combination of SSS_GND_DESCEND and
> > SSS_GND_INCLUDE_DISABLED which is called in SSS_GND_ALL_DOMAINS,
> > to avoid typing for the case when we want to include both
> > subdomains and disabled domains. For now it is only used
> > on one place (the third patch), but I think it will be
> > useful in the future.
> 
> Fine by me as well.
> 
> > From 253cc65b8ff3e5509e891f29a29b0196d2ac68fe Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
> > Date: Tue, 13 Oct 2015 14:25:08 +0200
> > Subject: [PATCH 4/4] sysdb: Use get_next_domain instead of dom->next
> 
> ACK
> 
> I'll just wait for CI and Coverity before pushing.

Hmm Coverity complains:
sssd-1.13.90/src/responder/autofs/autofssrv_cmd.c:872: returned_null: "get_next_domain" returns null (checked 104 out of 124 times).
sssd-1.13.90/src/util/domain_info_utils.c:45:5: var_tested_null: "dom" is null.
sssd-1.13.90/src/util/domain_info_utils.c:67:5: return_null_var: Returning "dom", which is null.
sssd-1.13.90/src/confdb/confdb.c:1461: example_assign: Example 1: Assigning: "dom" = return value from "get_next_domain(dom, 0U)".
sssd-1.13.90/src/confdb/confdb.c:1461: example_checked: Example 1 (cont.): "dom" has its value checked in "dom".
sssd-1.13.90/src/db/sysdb_subdomains.c:200: example_assign: Example 2: Assigning: "d" = return value from "get_next_domain(d, gnd_flags)".
sssd-1.13.90/src/db/sysdb_subdomains.c:200: example_checked: Example 2 (cont.): "d" has its value checked in "d".
sssd-1.13.90/src/db/sysdb_subdomains.c:273: example_assign: Example 3: Assigning: "dom" = return value from "get_next_domain(dom, 0U)".
sssd-1.13.90/src/db/sysdb_subdomains.c:273: example_checked: Example 3 (cont.): "dom" has its value checked in "dom".
sssd-1.13.90/src/db/sysdb_subdomains.c:316: example_assign: Example 4: Assigning: "dom" = return value from "get_next_domain(dom, 2U)".
sssd-1.13.90/src/db/sysdb_subdomains.c:315: example_checked: Example 4 (cont.): "dom" has its value checked in "dom".
sssd-1.13.90/src/monitor/monitor.c:808: example_assign: Example 5: Assigning: "other" = return value from "get_next_domain(dom, 0U)".
sssd-1.13.90/src/monitor/monitor.c:815: example_checked: Example 5 (cont.): "other" has its value checked in "other".
sssd-1.13.90/src/responder/autofs/autofssrv_cmd.c:872: var_assigned: Assigning: "dctx->domain" = null return value from "get_next_domain".
sssd-1.13.90/src/responder/autofs/autofssrv_cmd.c:873: dereference: Dereferencing a null pointer "dctx->domain".
#  871|           if (dctx->cmd_ctx->check_next && get_next_domain(dctx->domain, 0)) {
#  872|               dctx->domain = get_next_domain(dctx->domain, 0);
#  873|->             dctx->check_provider = NEED_CHECK_PROVIDER(dctx->domain->provider);
#  874|           }
#  875|       }

Error: NULL_RETURNS (CWE-476):
sssd-1.13.90/src/responder/nss/nsssrv_netgroup.c:650: returned_null: "get_next_domain" returns null (checked 104 out of 124 times).
sssd-1.13.90/src/util/domain_info_utils.c:45:5: var_tested_null: "dom" is null.
sssd-1.13.90/src/util/domain_info_utils.c:67:5: return_null_var: Returning "dom", which is null.
sssd-1.13.90/src/confdb/confdb.c:1461: example_assign: Example 1: Assigning: "dom" = return value from "get_next_domain(dom, 0U)".
sssd-1.13.90/src/confdb/confdb.c:1461: example_checked: Example 1 (cont.): "dom" has its value checked in "dom".
sssd-1.13.90/src/db/sysdb_subdomains.c:200: example_assign: Example 2: Assigning: "d" = return value from "get_next_domain(d, gnd_flags)".
sssd-1.13.90/src/db/sysdb_subdomains.c:200: example_checked: Example 2 (cont.): "d" has its value checked in "d".
sssd-1.13.90/src/db/sysdb_subdomains.c:273: example_assign: Example 3: Assigning: "dom" = return value from "get_next_domain(dom, 0U)".
sssd-1.13.90/src/db/sysdb_subdomains.c:273: example_checked: Example 3 (cont.): "dom" has its value checked in "dom".
sssd-1.13.90/src/db/sysdb_subdomains.c:316: example_assign: Example 4: Assigning: "dom" = return value from "get_next_domain(dom, 2U)".
sssd-1.13.90/src/db/sysdb_subdomains.c:315: example_checked: Example 4 (cont.): "dom" has its value checked in "dom".
sssd-1.13.90/src/monitor/monitor.c:808: example_assign: Example 5: Assigning: "other" = return value from "get_next_domain(dom, 0U)".
sssd-1.13.90/src/monitor/monitor.c:815: example_checked: Example 5 (cont.): "other" has its value checked in "other".
sssd-1.13.90/src/responder/nss/nsssrv_netgroup.c:650: var_assigned: Assigning: "dctx->domain" = null return value from "get_next_domain".
sssd-1.13.90/src/responder/nss/nsssrv_netgroup.c:651: dereference: Dereferencing a null pointer "dctx->domain".
#  649|           if (cmdctx->check_next && get_next_domain(dctx->domain, 0)) {
#  650|               dctx->domain = get_next_domain(dctx->domain, 0);
#  651|->             dctx->check_provider = NEED_CHECK_PROVIDER(dctx->domain->provider);
#  652|           }
#  653|       }


More information about the sssd-devel mailing list