I have refactored nsssrv_cmd.c and created a new patch for the
EntryCacheNoWaitRefreshTimeout.

I have created a new function, check_cache() which is a common entry
point for getpwnam, getpwuid, getgrnam and getgrgid to examine whether
the cache is still valid.

Addressing other points from the review inline below.


On 08/17/2009 11:19 AM, Sumit Bose wrote:
> On Fri, Aug 14, 2009 at 03:46:54PM -0400, Stephen Gallagher wrote:
>> This timeout specifies the lifetime of a cache entry before it is
>> updated out-of-band. When this timeout is hit, the request will
>> still complete from cache, but the SSSD will also go and update
>> the cached entry in the background to extend the life of the
>> cache entry and reduce the wait time of a future request.
>>
>> Support for the EnumCacheNoWaitRefreshTimeout is still forthcoming, but
>> I wanted to get a formal review on this portion.
> 
> 
> NACK. I think this patch indicates that nsssrv_cmd.c needs some
> refactoring, please do this before adding more code.
> 

Done.

> More comments follow below.
> 
> bye,
> Sumit
> 
> 
>> From c8d774ee2741c76c4c2a07bcae112924b0061e86 Mon Sep 17 00:00:00 2001
>> From: Stephen Gallagher <sgall...@redhat.com>
>> Date: Fri, 14 Aug 2009 08:59:53 -0400
>> Subject: [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout
>>
>> This timeout specifies the lifetime of a cache entry before it is
>> updated out-of-band. When this timeout is hit, the request will
>> still complete from cache, but the SSSD will also go and update
>> the cached entry in the background to extend the life of the
>> cache entry and reduce the wait time of a future request.
>> ---
>>  server/examples/sssd.conf         |   13 ++
>>  server/man/sssd.conf.5.xml        |   13 ++
>>  server/responder/nss/nsssrv.c     |   20 ++++
>>  server/responder/nss/nsssrv.h     |    7 +-
>>  server/responder/nss/nsssrv_cmd.c |  227 
>> ++++++++++++++++++++++++++++++-------
>>  5 files changed, 236 insertions(+), 44 deletions(-)
>>
>> diff --git a/server/responder/nss/nsssrv.c b/server/responder/nss/nsssrv.c
>> index 456c629..6d7bf74 100644
>> --- a/server/responder/nss/nsssrv.c
>> +++ b/server/responder/nss/nsssrv.c
>> @@ -103,6 +103,26 @@ static int nss_get_config(struct nss_ctx *nctx,
>>                           &nctx->neg_timeout);
>>      if (ret != EOK) goto done;
>>  
>> +    ret = confdb_get_int(cdb, nctx, NSS_SRV_CONFIG,
>> +                         "EnumCacheNoWaitRefreshTimeout", 0,
>> +                         &nctx->enum_cache_refresh_timeout);
>> +    if (ret != EOK) goto done;
>> +    if (nctx->enum_cache_refresh_timeout >= nctx->enum_cache_timeout) {
>> +        DEBUG(0,("Configuration error: EnumCacheNoWaitRefreshTimeout 
>> exceeds"
>> +                 "EnumCacheTimeout. Disabling feature.\n"));
>> +        nctx->enum_cache_refresh_timeout = 0;
>> +    }
>> +
>> +    ret = confdb_get_int(cdb, nctx, NSS_SRV_CONFIG,
>> +                         "EntryCacheNoWaitRefreshTimeout", 0,
>> +                         &nctx->cache_refresh_timeout);
>> +    if (ret != EOK) goto done;
>> +    if (nctx->cache_refresh_timeout >= nctx->cache_timeout) {
>> +        DEBUG(0,("Configuration error: EntryCacheNoWaitRefreshTimeout 
>> exceeds"
>> +                 "EntryCacheTimeout. Disabling feature.\n"));
>> +        nctx->cache_refresh_timeout = 0;
>> +    }
>> +
>>      ret = confdb_get_string_as_list(cdb, tmpctx, NSS_SRV_CONFIG,
>>                                      "filterUsers", &filter_list);
>>      if (ret == ENOENT) filter_list = NULL;
> 
> I haven't check other timeouts so far, but I think it makes sense to
> check if *_timeout < 0.

Added a check for the cache_refresh_timeout < 0

> 
>> diff --git a/server/responder/nss/nsssrv.h b/server/responder/nss/nsssrv.h
>> index 0d3124c..e756384 100644
>> --- a/server/responder/nss/nsssrv.h
>> +++ b/server/responder/nss/nsssrv.h
>> @@ -50,11 +50,14 @@ struct getent_ctx;
>>  struct nss_ctx {
>>      struct resp_ctx *rctx;
>>  
>> -    int cache_timeout;
>> -    int neg_timeout;
>>      struct nss_nc_ctx *ncache;
>>  
>> +    int neg_timeout;
>> +    int cache_timeout;
>>      int enum_cache_timeout;
>> +    int cache_refresh_timeout;
>> +    int enum_cache_refresh_timeout;
>> +
>>      time_t last_user_enum;
>>      time_t last_group_enum;
>>  
> 
> The *_timeout variables are use to compare against unsigned values. I
> would prefer the *_timeout having a type of time_t or uint*.
> 

Unfortunately, our confdb utilities only allow me to read out an int.
There's no benefit to casting them to unsigned in any case, since no
sensible admin is ever going to have a cache timeout greater than 2^16
seconds, and we're checking to make sure it's greater than zero.

>> diff --git a/server/responder/nss/nsssrv_cmd.c 
>> b/server/responder/nss/nsssrv_cmd.c
>> index e8f178a..f00a423 100644
>> --- a/server/responder/nss/nsssrv_cmd.c
>> +++ b/server/responder/nss/nsssrv_cmd.c
>> @@ -273,12 +273,15 @@ static void nss_cmd_getpwnam_callback(void *ptr, int 
>> status,
>>      struct cli_ctx *cctx = cmdctx->cctx;
>>      struct sss_domain_info *dom;
>>      struct nss_ctx *nctx;
>> -    int timeout;
>> +    int timeout, refresh_timeout;
> 
> see above and http://freeipa.org/page/Coding_Style#Declaring .
> 

Fixed.

>> +    time_t now;
>>      uint64_t lastUpdate;
>>      uint8_t *body;
>>      size_t blen;
>>      bool call_provider = false;
>>      bool neghit = false;
>> +    bool need_callback = true;
>> +    sss_dp_callback_t cb = NULL;
>>      int ncret;
>>      int ret;
>>  
>> @@ -296,16 +299,33 @@ static void nss_cmd_getpwnam_callback(void *ptr, int 
>> status,
>>      if (dctx->check_provider) {
>>          switch (res->count) {
>>          case 0:
>> +            /* This is a cache miss. We need to get the updated user 
>> information
>> +             * before returning it.
>> +             */
>>              call_provider = true;
>> +            need_callback = true;
>>              break;
>>  
>>          case 1:
>>              timeout = nctx->cache_timeout;
>> -
>> +            refresh_timeout = nctx->cache_refresh_timeout;
>>              lastUpdate = ldb_msg_find_attr_as_uint64(res->msgs[0],
>>                                                       SYSDB_LAST_UPDATE, 0);
>> -            if (lastUpdate + timeout < time(NULL)) {
>> +            now = time(NULL);
>> +            if (lastUpdate + timeout < now) {
>> +                /* This is a cache miss. We need to get the updated user
>> +                 * information before returning it.
>> +                 */
>> +                call_provider = true;
>> +                need_callback = true;
>> +            }
>> +            else if (refresh_timeout && (lastUpdate + refresh_timeout < 
>> now)) {
>> +                /* We're past the the cache refresh timeout
>> +                 * We'll return the value from the cache, but we'll also
>> +                 * queue the cache entry for update out-of-band.
>> +                 */
>>                  call_provider = true;
>> +                need_callback = false;
>>              }
> 
> I would prefer an else-block here. Although call_provider and
> need_callback are initialised to do the right thing an else-block would
> make it easier to follow the logic here.
> 

Added.

> 
>>              break;
>>  
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel


-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From 315007d9113638692b404ef7107408d32810f6f3 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Tue, 8 Sep 2009 17:22:25 -0400
Subject: [PATCH 1/2] Consolidate cache lookups in the NSS

getpwnam, getpwuid, getgrnam and getgrgid will now use a common
function, check_cache, for determining whether to return a cached
value or to go to the provider.
---
 server/responder/nss/nsssrv_cmd.c |  271 +++++++++++++------------------------
 1 files changed, 94 insertions(+), 177 deletions(-)

diff --git a/server/responder/nss/nsssrv_cmd.c 
b/server/responder/nss/nsssrv_cmd.c
index 405dae8..da53e09 100644
--- a/server/responder/nss/nsssrv_cmd.c
+++ b/server/responder/nss/nsssrv_cmd.c
@@ -80,6 +80,12 @@ static int nss_cmd_send_error(struct nss_cmd_ctx *cmdctx, 
int err)
     return; \
 } while(0)
 
+#define NSS_CMD_FATAL_ERROR_CODE(cctx, ret) do { \
+    DEBUG(1,("Fatal error, killing connection!")); \
+    talloc_free(cctx); \
+    return ret; \
+} while(0)
+
 static struct sss_domain_info *nss_get_dom(struct sss_domain_info *doms,
                                            const char *domain)
 {
@@ -262,37 +268,21 @@ done:
     return EOK;
 }
 
-static void nss_cmd_getpwnam_dp_callback(uint16_t err_maj, uint32_t err_min,
-                                         const char *err_msg, void *ptr);
-
-static void nss_cmd_getpwnam_callback(void *ptr, int status,
-                                   struct ldb_result *res)
+static errno_t check_cache(struct nss_dom_ctx *dctx,
+                           struct nss_ctx *nctx,
+                           struct ldb_result *res,
+                           int req_type,
+                           const char *opt_name,
+                           uint32_t opt_id,
+                           sss_dp_callback_t callback)
 {
-    struct nss_dom_ctx *dctx = talloc_get_type(ptr, struct nss_dom_ctx);
-    struct nss_cmd_ctx *cmdctx = dctx->cmdctx;
-    struct cli_ctx *cctx = cmdctx->cctx;
-    struct sysdb_ctx *sysdb;
-    struct sss_domain_info *dom;
-    struct nss_ctx *nctx;
+    errno_t ret;
     int timeout;
+    time_t now;
     uint64_t lastUpdate;
-    uint8_t *body;
-    size_t blen;
+    struct nss_cmd_ctx *cmdctx = dctx->cmdctx;
+    struct cli_ctx *cctx = cmdctx->cctx;
     bool call_provider = false;
-    bool neghit = false;
-    int ncret;
-    int ret;
-
-    nctx = talloc_get_type(cctx->rctx->pvt_ctx, struct nss_ctx);
-
-    if (status != LDB_SUCCESS) {
-        ret = nss_cmd_send_error(cmdctx, status);
-        if (ret != EOK) {
-            NSS_CMD_FATAL_ERROR(cctx);
-        }
-        sss_cmd_done(cctx, cmdctx);
-        return;
-    }
 
     if (dctx->check_provider) {
         switch (res->count) {
@@ -302,10 +292,11 @@ static void nss_cmd_getpwnam_callback(void *ptr, int 
status,
 
         case 1:
             timeout = nctx->cache_timeout;
+            now = time(NULL);
 
             lastUpdate = ldb_msg_find_attr_as_uint64(res->msgs[0],
                                                      SYSDB_LAST_UPDATE, 0);
-            if (lastUpdate + timeout < time(NULL)) {
+            if (lastUpdate + timeout < now) {
                 call_provider = true;
             }
             break;
@@ -314,15 +305,14 @@ static void nss_cmd_getpwnam_callback(void *ptr, int 
status,
             DEBUG(1, ("getpwnam call returned more than one result !?!\n"));
             ret = nss_cmd_send_error(cmdctx, ENOENT);
             if (ret != EOK) {
-                NSS_CMD_FATAL_ERROR(cctx);
+                NSS_CMD_FATAL_ERROR_CODE(cctx, ENOENT);
             }
             sss_cmd_done(cctx, cmdctx);
-            return;
+            return ENOENT;
         }
     }
 
     if (call_provider) {
-
         /* dont loop forever :-) */
         dctx->check_provider = false;
         timeout = SSS_CLI_SOCKET_TIMEOUT/2;
@@ -333,18 +323,65 @@ static void nss_cmd_getpwnam_callback(void *ptr, int 
status,
         }
 
         ret = sss_dp_send_acct_req(cctx->rctx, cmdctx,
-                                   nss_cmd_getpwnam_dp_callback, dctx,
-                                   timeout, dctx->domain->name, SSS_DP_USER,
-                                   cmdctx->name, 0);
+                                   callback, dctx, timeout,
+                                   dctx->domain->name, req_type,
+                                   opt_name, opt_id);
         if (ret != EOK) {
             DEBUG(3, ("Failed to dispatch request: %d(%s)\n",
                       ret, strerror(ret)));
             ret = nss_cmd_send_error(cmdctx, ret);
             if (ret != EOK) {
-                NSS_CMD_FATAL_ERROR(cctx);
+                NSS_CMD_FATAL_ERROR_CODE(cctx, EIO);
             }
             sss_cmd_done(cctx, cmdctx);
+            return EIO;
         }
+
+        /* Tell the calling function to return so the dp callback
+         * can resolve
+         */
+        return EAGAIN;
+    }
+
+    return EOK;
+}
+
+static void nss_cmd_getpwnam_dp_callback(uint16_t err_maj, uint32_t err_min,
+                                         const char *err_msg, void *ptr);
+
+static void nss_cmd_getpwnam_callback(void *ptr, int status,
+                                   struct ldb_result *res)
+{
+    struct nss_dom_ctx *dctx = talloc_get_type(ptr, struct nss_dom_ctx);
+    struct nss_cmd_ctx *cmdctx = dctx->cmdctx;
+    struct cli_ctx *cctx = cmdctx->cctx;
+    struct sysdb_ctx *sysdb;
+    struct sss_domain_info *dom;
+    struct nss_ctx *nctx;
+    uint8_t *body;
+    size_t blen;
+    bool neghit = false;
+    int ncret;
+    int ret;
+
+    nctx = talloc_get_type(cctx->rctx->pvt_ctx, struct nss_ctx);
+
+    if (status != LDB_SUCCESS) {
+        ret = nss_cmd_send_error(cmdctx, status);
+        if (ret != EOK) {
+            NSS_CMD_FATAL_ERROR(cctx);
+        }
+        sss_cmd_done(cctx, cmdctx);
+        return;
+    }
+
+    ret = check_cache(dctx, nctx, res,
+                      SSS_DP_USER, cmdctx->name, 0,
+                      nss_cmd_getpwnam_dp_callback);
+    if (ret != EOK) {
+        /* Anything but EOK means we should reenter the mainloop
+         * because we may be refreshing the cache
+         */
         return;
     }
 
@@ -668,11 +705,8 @@ static void nss_cmd_getpwuid_callback(void *ptr, int 
status,
     struct sss_domain_info *dom;
     struct sysdb_ctx *sysdb;
     struct nss_ctx *nctx;
-    int timeout;
-    uint64_t lastUpdate;
     uint8_t *body;
     size_t blen;
-    bool call_provider = false;
     bool neghit = false;
     int ret;
     int ncret;
@@ -688,57 +722,13 @@ static void nss_cmd_getpwuid_callback(void *ptr, int 
status,
         return;
     }
 
-    if (dctx->check_provider) {
-        switch (res->count) {
-        case 0:
-            call_provider = true;
-            break;
-
-        case 1:
-            timeout = nctx->cache_timeout;
-
-            lastUpdate = ldb_msg_find_attr_as_uint64(res->msgs[0],
-                                                     SYSDB_LAST_UPDATE, 0);
-            if (lastUpdate + timeout < time(NULL)) {
-                call_provider = true;
-            }
-            break;
-
-        default:
-            DEBUG(1, ("getpwuid call returned more than one result !?!\n"));
-            ret = nss_cmd_send_error(cmdctx, ENOENT);
-            if (ret != EOK) {
-                NSS_CMD_FATAL_ERROR(cctx);
-            }
-            sss_cmd_done(cctx, cmdctx);
-            return;
-        }
-    }
-
-    if (call_provider) {
-
-        /* dont loop forever :-) */
-        dctx->check_provider = false;
-        timeout = SSS_CLI_SOCKET_TIMEOUT/2;
-
-        /* keep around current data in case backend is offline */
-        if (res->count) {
-            dctx->res = talloc_steal(dctx, res);
-        }
-
-        ret = sss_dp_send_acct_req(cctx->rctx, cmdctx,
-                                   nss_cmd_getpwuid_dp_callback, dctx,
-                                   timeout, dctx->domain->name, SSS_DP_USER,
-                                   NULL, cmdctx->id);
-        if (ret != EOK) {
-            DEBUG(3, ("Failed to dispatch request: %d(%s)\n",
-                      ret, strerror(ret)));
-            ret = nss_cmd_send_error(cmdctx, ret);
-            if (ret != EOK) {
-                NSS_CMD_FATAL_ERROR(cctx);
-            }
-            sss_cmd_done(cctx, cmdctx);
-        }
+    ret = check_cache(dctx, nctx, res,
+                      SSS_DP_USER, NULL, cmdctx->id,
+                      nss_cmd_getpwuid_dp_callback);
+    if (ret != EOK) {
+        /* Anything but EOK means we should reenter the mainloop
+         * because we may be refreshing the cache
+         */
         return;
     }
 
@@ -1702,11 +1692,9 @@ static void nss_cmd_getgrnam_callback(void *ptr, int 
status,
     struct sss_domain_info *dom;
     struct sysdb_ctx *sysdb;
     struct nss_ctx *nctx;
-    int timeout;
-    uint64_t lastUpdate;
+    struct sss_cache_ctx *cache_ctx = NULL;
     uint8_t *body;
     size_t blen;
-    bool call_provider = false;
     bool neghit = false;
     int ncret;
     int i, ret;
@@ -1722,47 +1710,13 @@ static void nss_cmd_getgrnam_callback(void *ptr, int 
status,
         return;
     }
 
-    if (dctx->check_provider) {
-        switch (res->count) {
-        case 0:
-            call_provider = true;
-            break;
-
-        default:
-            timeout = nctx->cache_timeout;
-
-            lastUpdate = ldb_msg_find_attr_as_uint64(res->msgs[0],
-                                                     SYSDB_LAST_UPDATE, 0);
-            if (lastUpdate + timeout < time(NULL)) {
-                call_provider = true;
-            }
-        }
-    }
-
-    if (call_provider) {
-
-        /* dont loop forever :-) */
-        dctx->check_provider = false;
-        timeout = SSS_CLI_SOCKET_TIMEOUT/2;
-
-        /* keep around current data in case backend is offline */
-        if (res->count) {
-            dctx->res = talloc_steal(dctx, res);
-        }
-
-        ret = sss_dp_send_acct_req(cctx->rctx, cmdctx,
-                                   nss_cmd_getgrnam_dp_callback, dctx,
-                                   timeout, dctx->domain->name, SSS_DP_GROUP,
-                                   cmdctx->name, 0);
-        if (ret != EOK) {
-            DEBUG(3, ("Failed to dispatch request: %d(%s)\n",
-                      ret, strerror(ret)));
-            ret = nss_cmd_send_error(cmdctx, ret);
-            if (ret != EOK) {
-                NSS_CMD_FATAL_ERROR(cctx);
-            }
-            sss_cmd_done(cctx, cmdctx);
-        }
+    ret = check_cache(dctx, nctx, res,
+                      SSS_DP_GROUP, cmdctx->name, 0,
+                      nss_cmd_getgrnam_dp_callback);
+    if (ret != EOK) {
+        /* Anything but EOK means we should reenter the mainloop
+         * because we may be refreshing the cache
+         */
         return;
     }
 
@@ -2082,11 +2036,8 @@ static void nss_cmd_getgrgid_callback(void *ptr, int 
status,
     struct sss_domain_info *dom;
     struct sysdb_ctx *sysdb;
     struct nss_ctx *nctx;
-    int timeout;
-    uint64_t lastUpdate;
     uint8_t *body;
     size_t blen;
-    bool call_provider = false;
     bool neghit = false;
     int i, ret;
     int ncret;
@@ -2102,47 +2053,13 @@ static void nss_cmd_getgrgid_callback(void *ptr, int 
status,
         return;
     }
 
-    if (dctx->check_provider) {
-        switch (res->count) {
-        case 0:
-            call_provider = true;
-            break;
-
-        default:
-            timeout = nctx->cache_timeout;
-
-            lastUpdate = ldb_msg_find_attr_as_uint64(res->msgs[0],
-                                                     SYSDB_LAST_UPDATE, 0);
-            if (lastUpdate + timeout < time(NULL)) {
-                call_provider = true;
-            }
-        }
-    }
-
-    if (call_provider) {
-
-        /* dont loop forever :-) */
-        dctx->check_provider = false;
-        timeout = SSS_CLI_SOCKET_TIMEOUT/2;
-
-        /* keep around current data in case backend is offline */
-        if (res->count) {
-            dctx->res = talloc_steal(dctx, res);
-        }
-
-        ret = sss_dp_send_acct_req(cctx->rctx, cmdctx,
-                                   nss_cmd_getgrgid_dp_callback, dctx,
-                                   timeout, dctx->domain->name, SSS_DP_GROUP,
-                                   NULL, cmdctx->id);
-        if (ret != EOK) {
-            DEBUG(3, ("Failed to dispatch request: %d(%s)\n",
-                      ret, strerror(ret)));
-            ret = nss_cmd_send_error(cmdctx, ret);
-            if (ret != EOK) {
-                NSS_CMD_FATAL_ERROR(cctx);
-            }
-            sss_cmd_done(cctx, cmdctx);
-        }
+    ret = check_cache(dctx, nctx, res,
+                      SSS_DP_GROUP, NULL, cmdctx->id,
+                      nss_cmd_getgrgid_dp_callback);
+    if (ret != EOK) {
+        /* Anything but EOK means we should reenter the mainloop
+         * because we may be refreshing the cache
+         */
         return;
     }
 
-- 
1.6.2.5

From a303327f301313a6fecddc97492342a75eac88ee Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Tue, 8 Sep 2009 20:18:12 -0400
Subject: [PATCH 2/2] Add support for the EntryCacheNoWaitRefreshTimeout

This timeout specifies the lifetime of a cache entry before it is
updated out-of-band. When this timeout is hit, the request will
still complete from cache, but the SSSD will also go and update
the cached entry in the background to extend the life of the
cache entry and reduce the wait time of a future request.
---
 server/responder/nss/nsssrv.c     |   16 ++++++++++++++++
 server/responder/nss/nsssrv.h     |    4 +++-
 server/responder/nss/nsssrv_cmd.c |   35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/server/responder/nss/nsssrv.c b/server/responder/nss/nsssrv.c
index ad80438..3920189 100644
--- a/server/responder/nss/nsssrv.c
+++ b/server/responder/nss/nsssrv.c
@@ -107,6 +107,22 @@ static int nss_get_config(struct nss_ctx *nctx,
                          &nctx->filter_users_in_groups);
     if (ret != EOK) goto done;
 
+
+    ret = confdb_get_int(cdb, nctx, NSS_SRV_CONFIG,
+                         "EntryCacheNoWaitRefreshTimeout", 0,
+                         &nctx->cache_refresh_timeout);
+    if (ret != EOK) goto done;
+    if (nctx->cache_refresh_timeout >= nctx->cache_timeout) {
+        DEBUG(0,("Configuration error: EntryCacheNoWaitRefreshTimeout exceeds"
+                 "EntryCacheTimeout. Disabling feature.\n"));
+        nctx->cache_refresh_timeout = 0;
+    }
+    if (nctx->cache_refresh_timeout < 0) {
+        DEBUG(0,("Configuration error: EntryCacheNoWaitRefreshTimeout is"
+                 "invalid. Disabling feature.\n"));
+        nctx->cache_refresh_timeout = 0;
+    }
+
     ret = confdb_get_string_as_list(cdb, tmpctx, NSS_SRV_CONFIG,
                                     "filterUsers", &filter_list);
     if (ret == ENOENT) filter_list = NULL;
diff --git a/server/responder/nss/nsssrv.h b/server/responder/nss/nsssrv.h
index c5a7bb3..a5adbaf 100644
--- a/server/responder/nss/nsssrv.h
+++ b/server/responder/nss/nsssrv.h
@@ -46,10 +46,12 @@ struct getent_ctx;
 struct nss_ctx {
     struct resp_ctx *rctx;
 
-    int cache_timeout;
     int neg_timeout;
     struct nss_nc_ctx *ncache;
 
+    int cache_timeout;
+    int cache_refresh_timeout;
+
     int enum_cache_timeout;
     time_t last_user_enum;
     time_t last_group_enum;
diff --git a/server/responder/nss/nsssrv_cmd.c 
b/server/responder/nss/nsssrv_cmd.c
index da53e09..9407f40 100644
--- a/server/responder/nss/nsssrv_cmd.c
+++ b/server/responder/nss/nsssrv_cmd.c
@@ -278,26 +278,50 @@ static errno_t check_cache(struct nss_dom_ctx *dctx,
 {
     errno_t ret;
     int timeout;
+    int refresh_timeout;
     time_t now;
     uint64_t lastUpdate;
     struct nss_cmd_ctx *cmdctx = dctx->cmdctx;
     struct cli_ctx *cctx = cmdctx->cctx;
     bool call_provider = false;
+    sss_dp_callback_t cb = NULL;
 
     if (dctx->check_provider) {
         switch (res->count) {
         case 0:
+            /* This is a cache miss. We need to get the updated user
+             * information before returning it.
+             */
             call_provider = true;
+            cb = callback;
             break;
 
         case 1:
             timeout = nctx->cache_timeout;
+            refresh_timeout = nctx->cache_refresh_timeout;
             now = time(NULL);
 
             lastUpdate = ldb_msg_find_attr_as_uint64(res->msgs[0],
                                                      SYSDB_LAST_UPDATE, 0);
             if (lastUpdate + timeout < now) {
+                /* This is a cache miss. We need to get the updated user
+                 * information before returning it.
+                 */
                 call_provider = true;
+                cb = callback;
+            }
+            else if (refresh_timeout && (lastUpdate + refresh_timeout < now)) {
+                /* We're past the the cache refresh timeout
+                 * We'll return the value from the cache, but we'll also
+                 * queue the cache entry for update out-of-band.
+                 */
+                call_provider = true;
+                cb = NULL;
+            }
+            else {
+                /* Cache is still valid. Just return it. */
+                call_provider = false;
+                cb = NULL;
             }
             break;
 
@@ -340,7 +364,16 @@ static errno_t check_cache(struct nss_dom_ctx *dctx,
         /* Tell the calling function to return so the dp callback
          * can resolve
          */
-        return EAGAIN;
+        if (cb) {
+            return EAGAIN;
+        }
+
+        /* No callback required
+         * This was an out-of-band update. We'll return EOK
+         * so the calling function can return the cached entry
+         * immediately.
+         */
+        DEBUG(3, ("Updating cache out-of-band\n"));
     }
 
     return EOK;
-- 
1.6.2.5

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

Reply via email to