On 01/19/2016 01:13 PM, Michal Židek wrote:
> On 01/19/2016 12:28 PM, Pavel Březina wrote:
>> On 01/19/2016 12:24 PM, Michal Židek wrote:
>>> On 01/19/2016 11:56 AM, Pavel Březina wrote:
>>>> On 01/19/2016 02:08 AM, Michal Židek wrote:
>>>>> Hello,
>>>>>
>>>>> this patch applies on top of the
>>>>> patch from thred:
>>>>> [SSSD] [PATCH] NSS: Refresh also netgroup cache if needed
>>>>>
>>>>> It (re-)fixes the ticket:
>>>>>
https://fedorahosted.org/sssd/ticket/2102
>>>>>
>>>>> I separated them to give this one special attention :)
>>>>> and also because I am not sure if it is a good solution or
>>>>> not.
>>>>>
>>>>> I would like pbrezina or preichl to review
>>>>> this one, because they were the original
>>>>> reviewer/author for the fix, so maybe they
>>>>> still remember why it was done that way.
>>>>>
>>>>> Michal
>>>>
>>>> Nack.
>>>>
>>>>> if (ret == EOK) {
>>>>> DEBUG(SSSDBG_TRACE_FUNC, "Cached entry is valid,
>>>>> returning..\n");
>>>>> return EOK;
>>>>> } else if (ret == EAGAIN &&
>>>>> dctx->domain->refresh_expired_interval
>>>>> && req_type == SSS_DP_NETGR) {
>>>>> /* Skip midpoint refresh if background refresh is
enabled
>>>>> * (for netgroups only)
>>>>> */
>>>>> return EOK;
>>>>> } else if (ret != EAGAIN && ret != ENOENT) {
>>>>> DEBUG(SSSDBG_CRIT_FAILURE, "Error checking cache:
%d\n",
>>>>> ret);
>>>>> goto error;
>>>>> }
>>>>
>>>> Please, return is_refresh_on_bg function and extend it to return true
>>>> also for users and groups and merge the condition with the first
>>>> one so
>>>> debug message is visible, like so:
>>>>
>>>> if (ret == EOK || (ret == EAGAIN && is_refreshed_on_bg(...)) {
>>>> DEBUG(SSSDBG_TRACE_FUNC, "Cached entry is valid,
returning..\n");
>>>> return EOK;
>>>> }
>>>>
>>>> Maybe you can merge those two patches together..
>>>
>>> I wanted this one only in master. Do you think it should be
>>> backported as well? I wanted only necessary changes in downstream.
>>>
>>> Michal
>>
>> I think it should be backported since "Tests showed that having midpoint
>> refresh enabled may actually slow down netgroup request occasionally."
>> See:
>>
https://fedorahosted.org/sssd/ticket/2102
>>
>> I don't remember thought on which tests is this statement based on...
>
> Ok, I merged the two then. Attached is the merged patch for master and
> 1.13.
>
> Michal
>
> 0001-NSS-do-not-skip-cache-check-for-netgoups.patch
>
>
> From c4209629af439f3ead805b7c89ee9ac36c6264a6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?=<mzidek(a)redhat.com>
> Date: Mon, 18 Jan 2016 22:02:55 +0100
> Subject: [PATCH] NSS: do not skip cache check for netgoups
>
> When refresh_expired_interval was not zero,
> the NSS responder only refreshed netgroup cache
> using background periodic task and ignored
> SYSDB_CACHE_EXPIRE attribute.
>
> With this behaviour it was impossible to
> get new netgroup from remote server even
> after sss_cache tool was used to expire
> existing entry in the cache.
> ---
> src/responder/nss/nsssrv_cmd.c | 48
> +++++++++++++++++++++---------------------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/src/responder/nss/nsssrv_cmd.c
> b/src/responder/nss/nsssrv_cmd.c
> index d6ac9dc..1f24d1b 100644
> --- a/src/responder/nss/nsssrv_cmd.c
> +++ b/src/responder/nss/nsssrv_cmd.c
> @@ -579,10 +579,9 @@ static int nss_cmd_getpw_send_reply(struct
> nss_dom_ctx *dctx, bool filter)
> return EOK;
> }
>
> -/* Currently only refreshing expired netgroups is supported. */
> static bool
> is_refreshed_on_bg(enum sss_dp_acct_type req_type,
> - enum sss_dp_acct_type refresh_expired_interval)
> + uint32_t refresh_expired_interval)
> {
> if (refresh_expired_interval == 0) {
> return false;
> @@ -590,6 +589,8 @@ is_refreshed_on_bg(enum sss_dp_acct_type req_type,
>
> switch (req_type) {
> case SSS_DP_NETGR:
> + case SSS_DP_USER:
> + case SSS_DP_GROUP:
> return true;
> default:
> return false;
> @@ -753,31 +754,30 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
> get_dp_name_and_id(dctx->cmdctx, dctx->domain, req_type,
> opt_name, opt_id,
> &name, &id);
>
> - /* if we have any reply let's check cache validity, but ignore
> netgroups
> - * if refresh_expired_interval is set (which implies that another
> method
> - * is used to refresh netgroups)
> - */
> + /* if we have any reply let's check cache validity */
> if (res->count > 0) {
> - if (is_refreshed_on_bg(req_type,
> -
> dctx->domain->refresh_expired_interval)) {
> - ret = EOK;
> - } else {
> - if (req_type == SSS_DP_INITGROUPS) {
> - cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
> -
> SYSDB_INITGR_EXPIRE,
> - 0);
> - } else {
> - cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
> -
> SYSDB_CACHE_EXPIRE,
> - 0);
> - }
> + bool refreshed_on_bg;
> + uint32_t bg_refresh_interval =
> dctx->domain->refresh_expired_interval;
If you want to declare variables inside the block you can also move it
so it is next to its assignment to reduce the scope even further since
we use C99 anyway... but I'll leave it up to you.
I'll keep it the way it is.
Variables should be declared at the beginning of the block.
And this is the inner most block, so I can not make it's
scope smaller.
>
> - /* if we have any reply let's check cache validity */
> - ret = sss_cmd_check_cache(res->msgs[0],
> - nctx->cache_refresh_percent,
> - cacheExpire);
> + if (req_type == SSS_DP_INITGROUPS) {
> + cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
> +
> SYSDB_INITGR_EXPIRE,
> + 0);
> + } else {
> + cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
> +
> SYSDB_CACHE_EXPIRE,
> + 0);
> }
> - if (ret == EOK) {
> +
> + /* Check if background refresh is enabled for this entry */
You can put this on one line.
> + refreshed_on_bg = is_refreshed_on_bg(req_type,
> + bg_refresh_interval);
> +
> + /* if we have any reply let's check cache validity */
> + ret = sss_cmd_check_cache(res->msgs[0],
> + nctx->cache_refresh_percent,
> + cacheExpire);
> + if (ret == EOK || (ret == EAGAIN && refreshed_on_bg)) {
> DEBUG(SSSDBG_TRACE_FUNC, "Cached entry is valid,
> returning..\n");
> return EOK;
> } else if (ret != EAGAIN && ret != ENOENT) {
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org