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(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.
> >>
> >>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(a)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(a)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(a)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(a)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| }