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@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?
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@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.
Michal
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@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.
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@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
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@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 :) )
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@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
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@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.
Michal
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@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
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@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
On 10/02/2015 03:34 PM, 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@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
bump
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@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.
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@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
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@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@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?
From df78c59a241241b9586f5d701e8a789d777bfb21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@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@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?
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@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@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.
From df78c59a241241b9586f5d701e8a789d777bfb21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@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@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.
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.
Michal
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@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@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@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@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@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.
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@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@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@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@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@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| }
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@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@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@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@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@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@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
On 10/15/2015 04:06 PM, Michal Židek wrote:
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@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@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@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@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@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@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
On Wed, Oct 21, 2015 at 04:27:36PM +0200, Michal Židek wrote:
If you agree that these Coverity complains are false positives it would be good to push the patches. So far they still apply.
Michal
The code change we did yesterday silenced the Coverity warning, so if you can update the patches also in the autofs responder and re-send I'll ACK and push.
On 10/22/2015 11:12 AM, Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 04:27:36PM +0200, Michal Židek wrote:
If you agree that these Coverity complains are false positives it would be good to push the patches. So far they still apply.
Michal
The code change we did yesterday silenced the Coverity warning, so if you can update the patches also in the autofs responder and re-send I'll ACK and push.
Thanks Jakub!
I added new patch to silence the Coverity warnings. Other patches are unchanged.
Michal
On Thu, Oct 22, 2015 at 01:00:36PM +0200, Michal Židek wrote:
On 10/22/2015 11:12 AM, Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 04:27:36PM +0200, Michal Židek wrote:
If you agree that these Coverity complains are false positives it would be good to push the patches. So far they still apply.
Michal
The code change we did yesterday silenced the Coverity warning, so if you can update the patches also in the autofs responder and re-send I'll ACK and push.
Thanks Jakub!
I added new patch to silence the Coverity warnings. Other patches are unchanged.
Michal
ACK to all.
On Fri, Oct 23, 2015 at 10:32:38AM +0200, Jakub Hrozek wrote:
On Thu, Oct 22, 2015 at 01:00:36PM +0200, Michal Židek wrote:
On 10/22/2015 11:12 AM, Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 04:27:36PM +0200, Michal Židek wrote:
If you agree that these Coverity complains are false positives it would be good to push the patches. So far they still apply.
Michal
The code change we did yesterday silenced the Coverity warning, so if you can update the patches also in the autofs responder and re-send I'll ACK and push.
Thanks Jakub!
I added new patch to silence the Coverity warnings. Other patches are unchanged.
Michal
ACK to all.
master: * e563de9203be581acc30c7794f568ae40d22bee0 * 2bbc9d6f8d5f2c1b07fd6968314b7f530b7f3a4d * f191a6f9f3313df88eaf3debf52eebfe5d3dee59 * c84dcaa58449c53cf038311ce63bb2c304081b9d * 877b92e80bde510d5cd9f03dbf01e2bcf73ab072
Even though the ticket was originally in 1.13.2, I moved it to 1.14 alpha and only pushed the patches to master. We can always backport them later if we see conflicts, but in general I would prefer to push few patches to sssd-1-13..
On (23/10/15 10:37), Jakub Hrozek wrote:
On Fri, Oct 23, 2015 at 10:32:38AM +0200, Jakub Hrozek wrote:
On Thu, Oct 22, 2015 at 01:00:36PM +0200, Michal Židek wrote:
On 10/22/2015 11:12 AM, Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 04:27:36PM +0200, Michal Židek wrote:
If you agree that these Coverity complains are false positives it would be good to push the patches. So far they still apply.
Michal
The code change we did yesterday silenced the Coverity warning, so if you can update the patches also in the autofs responder and re-send I'll ACK and push.
Thanks Jakub!
I added new patch to silence the Coverity warnings. Other patches are unchanged.
Michal
ACK to all.
master:
- e563de9203be581acc30c7794f568ae40d22bee0
- 2bbc9d6f8d5f2c1b07fd6968314b7f530b7f3a4d
- f191a6f9f3313df88eaf3debf52eebfe5d3dee59
- c84dcaa58449c53cf038311ce63bb2c304081b9d
- 877b92e80bde510d5cd9f03dbf01e2bcf73ab072
Even though the ticket was originally in 1.13.2, I moved it to 1.14 alpha and only pushed the patches to master. We can always backport them later if we see conflicts, but in general I would prefer to push few patches to sssd-1-13..
Michal's patches touched many files. src/confdb/confdb.c | 2 +- src/db/sysdb_subdomains.c | 11 +++++----- src/monitor/monitor.c | 10 ++++----- src/providers/ad/ad_subdomains.c | 4 ++-- src/providers/dp_refresh.c | 2 +- src/providers/ipa/ipa_subdomains.c | 4 ++-- src/providers/ipa/ipa_subdomains_server.c | 4 ++-- src/providers/ldap/sdap_domain.c | 4 ++-- src/responder/autofs/autofssrv_cmd.c | 6 +++--- src/responder/common/negcache.c | 8 +++---- src/responder/common/responder_cache_req.c | 7 ++++--- src/responder/common/responder_common.c | 8 ++++--- src/responder/common/responder_get_domains.c | 9 ++++---- src/responder/ifp/ifp_cache.c | 2 +- src/responder/ifp/ifp_domains.c | 9 ++++---- src/responder/ifp/ifp_groups.c | 2 +- src/responder/ifp/ifp_users.c | 2 +- src/responder/nss/nsssrv_cmd.c | 85 +++++++++++++++++++++++++++++++++++++------------------------------------ src/responder/nss/nsssrv_netgroup.c | 8 +++---- src/responder/nss/nsssrv_services.c | 20 +++++++++--------- src/responder/pam/pamsrv_cmd.c | 6 +++--- src/responder/sudo/sudosrv_get_sudorules.c | 6 +++--- src/tests/cmocka/test_utils.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------- src/tools/common/sss_tools.c | 6 ++++-- src/tools/sss_cache.c | 5 +++-- src/tools/sss_debuglevel.c | 4 ++-- src/tools/sss_override.c | 4 ++-- src/util/domain_info_utils.c | 34 ++++++++++++++++++------------ src/util/usertools.c | 4 ++-- src/util/util.h | 5 ++++- 30 files changed, 297 insertions(+), 138 deletions(-)
So there was high chance they would cause conflicts when backporting patches.
And it was required for #2736
sssd-1-13: * 5b8f64fea67f449c0cb7a62ffb6dc5420141ea5d * 3dc864a931a8ea9b1fd6b2f37a4773924c3b96b0 * d78a21bb54ff302660a632e23422089b1202ca2c * 4a3153e8b40b00d6b7aa3f54589d1527e9e138c9 * 2a385185e0c57bebda38b769579a012c6d38eb23
LS
sssd-devel@lists.fedorahosted.org