On 10/13/2015 05:09 PM, Jakub Hrozek wrote:
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?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
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?= <[email protected]>
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| }
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
This is false positive. If this:
cmdctx->check_next && get_next_domain(dctx->domain, 0)
Is true, than get_next_domain(dctx->domain)
will not return NULL.
Maybe coverity treats boolean parameters differently
than integer parameters and does not recognize that
the call in the "if" condition is the same as the call
inside the block? I can't think of other reason why
it would complain now.
Anyway it is false positive.
Michal
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
