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

Michal Židek mzidek at redhat.com
Thu Oct 15 14:06:38 UTC 2015


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 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|       }
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at 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


More information about the sssd-devel mailing list