[sssd PR#34][opened] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: opened

PR body:
"""
cache_req grown quite big from the original code and it turned out
that using switch statements to branch code for different cases
makes the code quite hard to read and further extend and any
modification to the logic itself is difficult.

This patch changes the switch statements to plugins with small
functions and separates logic into multiple modules. This gives
us better control over the code and improves readability and
maintainability while keeping code duplication to minimum.

At the moment only cache req user by name, id and upn is
implemented as a proof of concept. If you like it, I will
finish the rest, it won't take much time.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[Fabiano Fidêncio]

Pavel,

On Tue, Sep 27, 2016 at 1:25 PM, pbrezina
 wrote:
> URL: https://github.com/SSSD/sssd/pull/34
> Author: pbrezina
>  Title: #34: cache_req: move from switch to plugins
> Action: opened
>
> PR body:
> """
> cache_req grown quite big from the original code and it turned out
> that using switch statements to branch code for different cases
> makes the code quite hard to read and further extend and any
> modification to the logic itself is difficult.
>
> This patch changes the switch statements to plugins with small
> functions and separates logic into multiple modules. This gives
> us better control over the code and improves readability and
> maintainability while keeping code duplication to minimum.
>
> At the moment only cache req user by name, id and upn is
> implemented as a proof of concept. If you like it, I will
> finish the rest, it won't take much time.
> """

+1 for the idea.

Best Regards,
--
Fabiano Fidêncio

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
I'm fine with the approach but I didn't read the code line-by-line.

We should make sure the code coverage remains high.
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-250677362

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

pbrezina commented:
"""
On 09/30/2016 09:21 AM, Jakub Hrozek wrote:
> I'm fine with the approach but I didn't read the code line-by-line.
>
> We should make sure the code coverage remains high.

Good thing is that API is untouched thus existing unit tests should 
remain functional without any modification.

"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-250686933

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
Great. The next thing is...is there a way to split the commit a bit more? I have no idea how to review a 1800+ lines commit..
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-250700286

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
on another note, this commit might be a reason to split 1.14 and master..
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-250700349

[sssd PR#34][synchronized] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

pbrezina commented:
"""
I finished the code and split it into four commits:
1. Implements logic ... it is the same as in responder_cache_req.c but with added support for plugins.
2. Implements plugins ... add all existing functionality.
3. Switch to new code in makefile and #includes.
4. Remove the old code.
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-251083960

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
Well, it's still a lot of code :) but at least I started the review now. For starters, Coverity found some warnings:
```
Error: FORWARD_NULL (CWE-476):
sssd-1.14.2/src/responder/common/cache_req/cache_req_search.c:104: var_compare_op: Comparing "*_result" to null implies that "*_result" might be null.
sssd-1.14.2/src/responder/common/cache_req/cache_req_search.c:114: var_deref_op: Dereferencing null pointer "*_result".
#  112|                           cr->debugobj, ret, sss_strerror(ret));
#  113|           return ret;
#  114|->     } else if (cr->plugin->only_one_result && (*_result)->count > 1) {
#  115|           CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
#  116|                           "Multiple objects were found when "

Error: UNINIT (CWE-457):
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c:34: var_decl: Declaring variable "tmp_ctx" without initializer.
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c:68: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free".
#   66|   
#   67|   done:
#   68|->     talloc_free(tmp_ctx);
#   69|       return ret;
#   70|   }

Error: UNINIT (CWE-457):
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_group_by_name.c:34: var_decl: Declaring variable "tmp_ctx" without initializer.
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_group_by_name.c:74: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free".
#   72|   
#   73|   done:
#   74|->     talloc_free(tmp_ctx);
#   75|       return ret;
#   76|   }

Error: UNINIT (CWE-457):
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c:34: var_decl: Declaring variable "tmp_ctx" without initializer.
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c:74: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free".
#   72|   
#   73|   done:
#   74|->     talloc_free(tmp_ctx);
#   75|       return ret;
#   76|   }

Error: UNINIT (CWE-457):
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c:34: var_decl: Declaring variable "tmp_ctx" without initializer.
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c:68: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free".
#   66|   
#   67|   done:
#   68|->     talloc_free(tmp_ctx);
#   69|       return ret;
#   70|   }

Error: UNINIT (CWE-457):
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_user_by_name.c:34: var_decl: Declaring variable "tmp_ctx" without initializer.
sssd-1.14.2/src/responder/common/cache_req/plugins/cache_req_user_by_name.c:74: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free".
#   72|   
#   73|   done:
#   74|->     talloc_free(tmp_ctx);
#   75|       return ret;
#   76|   }
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-252864641

[sssd PR#34][synchronized] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

pbrezina commented:
"""
Updated patches pushed.
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-253188271

[sssd PR#34][synchronized] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
Apart from the nitpicks, the code looks better to me (I like that the lookup functionality is abstracted, so adding new lookups will be easy).

I tested several kinds of lookups, including UPN, overrides (only local overrides using the sss_override tool..), byname, byuid and negative cache. Everything seems to work OK.

There is also a high code coverage.
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-253780951

[sssd PR#34][+Changes requested] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: +Changes requested

[sssd PR#34][synchronized] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[sssd PR#34][-Changes requested] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: -Changes requested

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
Actually, sorry, one more Coverity warning:
```
Error: FORWARD_NULL (CWE-476):
sssd-1.14.2/src/responder/common/cache_req/cache_req_search.c:105: var_compare_op: Comparing "result" to null implies that "result" might be null.
sssd-1.14.2/src/responder/common/cache_req/cache_req_search.c:111: var_deref_op: Dereferencing null pointer "result".
#  109|       switch (ret) {
#  110|       case EOK:
#  111|->         if (cr->plugin->only_one_result && result->count > 1) {
#  112|               CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
#  113|                               "Multiple objects were found when "
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-253792257

[sssd PR#34][+Changes requested] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: +Changes requested

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

pbrezina commented:
"""
Updated patch pushed. Diff:

```c
diff --git a/src/responder/common/cache_req/cache_req_search.c b/src/responder/common/cache_req/cache_req_search.c
index 3c11efd..a36a900 100644
--- a/src/responder/common/cache_req/cache_req_search.c
+++ b/src/responder/common/cache_req/cache_req_search.c
@@ -102,7 +102,7 @@ static errno_t cache_req_search_cache(TALLOC_CTX *mem_ctx,
                     cr->debugobj);

ret = cr->plugin->lookup_fn(mem_ctx, cr, cr->data, cr->domain, &result);
-    if (ret == EOK && result != NULL && result->count == 0) {
+    if (ret == EOK && (result == NULL || result->count == 0)) {
         ret = ENOENT;
     }
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-254139867

[sssd PR#34][synchronized] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[sssd PR#34][+Accepted] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: +Accepted

[sssd PR#34][-Changes requested] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: -Changes requested

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
Thank you, ACK. I sent the patch to CI and I will push the patch when CI finishes.
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-254766112

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
On Wed, Oct 19, 2016 at 02:48:57AM -0700, Jakub Hrozek wrote:
> Thank you, ACK. I sent the patch to CI and I will push the patch when CI finishes.

passed: http://sssd-ci.duckdns.org/logs/job/55/31/summary.html

"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-254768629

[sssd PR#34][comment] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

jhrozek commented:
"""
Pushed in ef39016..e083a6b
"""

See the full comment at https://github.com/SSSD/sssd/pull/34#issuecomment-255036044

[sssd PR#34][closed] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Author: pbrezina
 Title: #34: cache_req: move from switch to plugins
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/34/head:pr34
git checkout pr34

[sssd PR#34][+Pushed] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: +Pushed

[sssd PR#34][-Accepted] cache_req: move from switch to plugins

[fidencio]

URL: https://github.com/SSSD/sssd/pull/34
Title: #34: cache_req: move from switch to plugins

Label: -Accepted