On Wed, Jan 20, 2016 at 11:47:53AM +0100, Pavel Březina wrote:
On 01/20/2016 11:31 AM, Jakub Hrozek wrote:
>On Thu, Jan 07, 2016 at 04:08:48PM +0100, Pavel Březina wrote:
>>https://fedorahosted.org/sssd/ticket/2849
>>
>>You can use Pavel's CI "intg - test regr 2849" test which he posted
to the
>>list today.
>
>> From c78ae1b550794548518a083870443b29fa5d12bf Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Wed, 6 Jan 2016 12:08:18 +0100
>>Subject: [PATCH 1/3] cache_req: simplify cache_req_cache_check()
>
>ACK to the code, but I think we can improve the names..
>
>>
>>---
>> src/responder/common/responder_cache_req.c | 80 ++++++++++++++++++------------
>> 1 file changed, 49 insertions(+), 31 deletions(-)
>>
>>diff --git a/src/responder/common/responder_cache_req.c
b/src/responder/common/responder_cache_req.c
>>index
4ab52b8188859f1143ba1ffa3de03d14ecc028c2..91f50cb308de250eca786b474845426687821189 100644
>>--- a/src/responder/common/responder_cache_req.c
>>+++ b/src/responder/common/responder_cache_req.c
>>@@ -568,6 +568,47 @@ static bool cache_req_bypass_cache(struct cache_req_input
*input)
>> return false;
>> }
>>
>>+static errno_t cache_req_cache_state(struct cache_req_input *input,
>>+ struct ldb_result *result,
>>+ time_t cache_refresh_percent)
>
>We already have a structure with this name. What about something like
>cache_req_check_entry or something like that?
How about:
* cache_req_validate_state
* cache_req_validate
* cache_req_expiration_status (to avoid the word state)
* cache_req_needs_refresh
I like both of the last two.
>
>>+{
>>+ time_t expire;
>>+
>>+ if (result == NULL || result->count == 0 ||
cache_req_bypass_cache(input)) {
>>+ return ENOENT;
>>+ }
>>+
>>+ if (input->type == CACHE_REQ_INITGROUPS) {
>>+ expire = ldb_msg_find_attr_as_uint64(result->msgs[0],
>>+ SYSDB_INITGR_EXPIRE, 0);
>>+ } else {
>>+ expire = ldb_msg_find_attr_as_uint64(result->msgs[0],
>>+ SYSDB_CACHE_EXPIRE, 0);
>>+ }
>>+
>>+ return sss_cmd_check_cache(result->msgs[0], cache_refresh_percent,
expire);
>>+}
>>+
>>+static void cache_req_provider_params(struct cache_req_input *input,
>>+ const char **_string,
>>+ uint32_t *_id,
>>+ const char **_flag)
>
>Are these provider params or request params?
How about:
* cache_req_dpreq_params
Sounds good.
>
>> From 8b7da2a5542c4c16ea0491ad2d24fd9614901cbc Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Wed, 6 Jan 2016 12:45:38 +0100
>>Subject: [PATCH 2/3] cache_req: do not lookup views if possible
>>
>>This is needed for LOCAL view but also creates a shortcut for
>>server side overrides.
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2849
[...]
>Maybe I don't remember the flow exactly anymore but the NSS
lookup code
>also contains:
> ldb_msg_find_attr_as_string(dctx->res->msgs[0],
> OVERRIDE_PREFIX SYSDB_NAME,
> NULL) != NULL
>
>We don't handle that condition here..what does that do? Do we need to
>include the condition in cache_req, too?
This is not needed, since we always search by original name if possible with
cache_req which also includes this condition.
Ah, OK. Then the code looks good to me. I will run some tests, then..