URL: https://github.com/SSSD/sssd/pull/154
Title: #154: Confront caches first
pbrezina commented:
"""
Hi, thank you for the patch.
```c
@@ -469,6 +476,8 @@ static void cache_req_input_parsed(struct tevent_req
*subreq)
return;
}
+ cache_req_setup_search_approach(state);
+
ret = cache_req_select_domains(req, domain);
if (ret != EAGAIN) {
tevent_req_error(req, ret);
```
The search approach needs to be set in `cache_req_create()`, since you don't
always parse the username. I also prefer the search approach to be set as a
return values instead of setting it inside function. Somethink like this:
```c
static enum cache_req_search_approach
cache_req_get_search_approach(bool confront_cache_first, bool
plugin_bypass_cache, bool request_bypass_cache)
cr->search_approach = cache_req_get_search_approach(rctx->confront_cache,
plugin->bypass_cache, data->bypass_cache)
```
If we search cache first and we have a cache hit you just return it regardless
its expiration status. I also don't like this procedural style here at all such
as `cache_req_evaluate_search_approach()` etc. It is quite difficult to
understand what is happening there.
What I would like to see and I personally think that it is much cleaner is
something like this:
1) `cache_req_search()` will get two parameters: `bool skip_cache`, `bool
skip_dp` or similar in meaning. This way you can select whichever search
approach you want:
* (1a) `skip_cache := false`, `skip_dp := false` => current approach
* (1b) `skip_cache := true`, `skip_dp := false` => bypass_cache is true
* (1c) `skip_cache := false`, `skip_dp := true` => search cache first (first
iteration)
* (1d) `skip_cache := true`, `skip_dp := false` => now search dp (second
iteration)
2) Decide whether `confront_cache_first` and `bypass_cache` is set and store it
into `cr` in `cache_req_create()`
3) If `confront_cache_first` is `false`, you will use
`cache_req_search_send(..., skip_cache := bypass_cache, skip_dp := false)`,
i.e. (1a) or (1b).
4) If `confront_cache_first` is `true` you need to perform two iterations first
with (1c) and then with (1d). With (1c) you need to return `ENOENT` when the
object is expired.
5) To support two iterations over all domains I highly recommend to move the
whole iteration logic into separate tevent request, e.g.
`cache_req_search_domains(ev, cr, domains, domain, skip_cache, skip_dp)` where
domains is list of `rctx->domains` and `domain` is domain from input if
specified. This way you will gain better control over the whole logic.
I know there is some glue code required for tevent but don't be scared of those
requests. It make the code quite clean and sequential when done right.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/154#issuecomment-281068933
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
