URL:
https://github.com/SSSD/sssd/pull/154
Title: #154: Confront caches first
pbrezina commented:
"""
Hi, this version looks much better. `cache_req: cinfront caches first` has some smaller
issues that will be easily fix. Let's focus on the first patch to get the tevent
request good and then we willl fix the second patch. In general, this change is good. We
just tent to stick to few coding styles guidelines to make it simpler to read and
understand.
1. Each tevent request has to have its own state structure that share name with the
request, do not reuse state of another request please. That is:
```c
struct tevent_req *cache_req_search_domains_send(TALLOC_CTX *mem_ctx, ...
struct tevent_req *req;
struct cache_req_state *state = NULL;
errno_t ret;
req = tevent_req_create(mem_ctx, &state, struct cache_req_state);
if (req == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n");
return NULL;
}
```
Instead of reusing state of `cache_req_send`, create and use a new `struct
cache_req_search_domains_state`. Put fields that are used by this request to this state
and remove fields that are not used by `cache_req` requests from `cache_req_state`.
2. Never reuse `_recv` function of another request, even though they are the same.
```c
static void cache_req_done(struct tevent_req *subreq)
{
struct cache_req_state *state;
struct tevent_req *req;
errno_t ret;
req = tevent_req_callback_data(subreq, struct tevent_req);
state = tevent_req_data(req, struct cache_req_state);
ret = cache_req_recv(state, subreq, &state->results);
talloc_zfree(subreq);
```
should be
```c
static void cache_req_done(struct tevent_req *subreq)
{
struct cache_req_state *state;
struct tevent_req *req;
errno_t ret;
req = tevent_req_callback_data(subreq, struct tevent_req);
state = tevent_req_data(req, struct cache_req_state);
ret = cache_req__search_domains_recv(state, subreq, &state->results);
talloc_zfree(subreq);
```
3. Each tevent requests should be a stand-alone working block of code that can be read
from the beginning of `_send` to the end of `_recv` without any jumps. I.e. whole code
should be together:
```c
struct request_state {}
struct tevent_req *request_send()
errno_t request_step1()
errno_t request_step2()
...
void request_done()
errno_t request_recv()
```
Please move `cache_req_search_domains` out of `cache_req` request code. The current code
read like:
```c
struct tevent_req *cache_req_send()
static errno_t cache_req_process_input()
static void cache_req_input_parsed()
static errno_t cache_req_select_domains()
static errno_t cache_req_search_domains()
struct tevent_req *cache_req_search_domains_send()
static errno_t cache_req_next_domain()
static errno_t cache_req_add_result()
static errno_t cache_req_create_and_add_result()
static void cache_req_search_domains_done()
static void cache_req_done()
```
It should read as:
```c
static errno_t cache_req_search_domains_add_result()
static errno_t cache_req_search_domains_create_and_add_result()
struct tevent_req *cache_req_search_domains_send()
static errno_t cache_req_search_domains_next()
static void cache_req_search_domains_done()
static void cache_req_search_domains_recv()
struct tevent_req *cache_req_send()
static errno_t cache_req_process_input()
static void cache_req_input_parsed()
static errno_t cache_req_select_domains()
static errno_t cache_req_search_domains()
static void cache_req_done()
```
4. `dp_success` parameter is not an input parameter that is required. It is a result of
`cache_req_search` and used only internally here.
```c
struct tevent_req *cache_req_search_domains_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
struct cache_req *cr,
struct sss_domain_info *domain,
bool check_next,
bool dp_success,
```
5. I know this seems like nitpicking and sometimes even unnecessary, but tevent requests
are not done right it become chaos that is hard to read, understand and work with if not
done right, so please stick with me. I had troubles learning it myself. :-)
"""
See the full comment at
https://github.com/SSSD/sssd/pull/154#issuecomment-284717277