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| }
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
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