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..