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?= <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
If you agree that these Coverity complains are false positives
it would be good to push the patches. So far they still apply.
Michal