Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-09 Thread Stephen Gallagher
On 09/09/2009 07:50 AM, Sumit Bose wrote:
 On Tue, Sep 08, 2009 at 08:32:55PM -0400, Stephen Gallagher wrote:
 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.

 
 Works for me, but can you add a man page entry for
 EnumCacheNoWaitRefreshTimeout ?
 
 bye,
 Sumit
 ___
 sssd-devel mailing list
 sssd-devel@lists.fedorahosted.org
 https://fedorahosted.org/mailman/listinfo/sssd-devel

Whoops, forgot to add those to the commit. New patch 0002 attached
(patch 0001 unaffected)



-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From f9eece15b906ad857ef0ba0f668a06a7552ac379 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/examples/sssd.conf |9 +
 server/man/sssd.conf.5.xml|   13 +
 server/responder/nss/nsssrv.c |   16 
 server/responder/nss/nsssrv.h |4 +++-
 server/responder/nss/nsssrv_cmd.c |   35 ++-
 5 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/server/examples/sssd.conf b/server/examples/sssd.conf
index 90e0c8d..b47ab9d 100644
--- a/server/examples/sssd.conf
+++ b/server/examples/sssd.conf
@@ -13,6 +13,15 @@ description = NSS Responder Configuration
 filterGroups = root
 filterUsers = root
 
+# The EntryCacheTimeout indicates the number of seconds to retain before
+# an entry in cache is considered stale and must block to refresh.
+# The EntryCacheNoWaitRefreshTimeout indicates the number of seconds to
+# wait before updating the cache out-of-band. (NSS requests will still
+# be returned from cache until the full EntryCacheTimeout). Setting this
+# value to 0 turns this feature off (default)
+; EntryCacheTimeout = 600
+; EntryCacheNoWaitRefreshTimeout = 300
+
 [services/dp]
 description = Data Provider Configuration
 
diff --git a/server/man/sssd.conf.5.xml b/server/man/sssd.conf.5.xml
index f4cb87a..fe2262d 100644
--- a/server/man/sssd.conf.5.xml
+++ b/server/man/sssd.conf.5.xml
@@ -331,6 +331,19 @@
 /listitem
 /varlistentry
 varlistentry
+termEntryCacheNoWaitRefreshTimeout (integer)/term
+listitem
+para
+How long should nss_sss return cached entries 
before
+initiating an out-of-band cache refresh (0 disables
+this feature)
+/para
+para
+Default: 0
+/para
+/listitem
+/varlistentry
+varlistentry
 termEntryNegativeTimeout (integer)/term
 listitem
 para
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));
+

Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-09 Thread Sumit Bose
On Wed, Sep 09, 2009 at 08:25:19AM -0400, Stephen Gallagher wrote:
 On 09/09/2009 07:50 AM, Sumit Bose wrote:
  On Tue, Sep 08, 2009 at 08:32:55PM -0400, Stephen Gallagher wrote:
  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.
 
  
  Works for me, but can you add a man page entry for
  EnumCacheNoWaitRefreshTimeout ?
  
  bye,
  Sumit
  ___
  sssd-devel mailing list
  sssd-devel@lists.fedorahosted.org
  https://fedorahosted.org/mailman/listinfo/sssd-devel
 
 Whoops, forgot to add those to the commit. New patch 0002 attached
 (patch 0001 unaffected)
 
Thanks.

ACK

bye,
Sumit
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-09 Thread Sumit Bose
On Tue, Sep 08, 2009 at 08:32:55PM -0400, Stephen Gallagher wrote:
 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.
 

hmm, I must force myself not to use debugging option while testing
patches ...

I have seen this one:

responder/nss/nsssrv_cmd.c: In function 'nss_cmd_getgrnam_callback':
responder/nss/nsssrv_cmd.c:1728: warning: unused variable 'cache_ctx'


bye,
Sumit
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-09 Thread Stephen Gallagher
On 09/09/2009 08:46 AM, Sumit Bose wrote:
 On Tue, Sep 08, 2009 at 08:32:55PM -0400, Stephen Gallagher wrote:
 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.

 
 hmm, I must force myself not to use debugging option while testing
 patches ...
 
 I have seen this one:
 
 responder/nss/nsssrv_cmd.c: In function 'nss_cmd_getgrnam_callback':
 responder/nss/nsssrv_cmd.c:1728: warning: unused variable 'cache_ctx'
 
 
 bye,
 Sumit
 ___
 sssd-devel mailing list
 sssd-devel@lists.fedorahosted.org
 https://fedorahosted.org/mailman/listinfo/sssd-devel

Thought I had removed all of those. That was in there for when I
originally thought I would need to return something from check_cache to
the caller.

New patches attached.

-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From 894a0a3880cb729b07eafe7b25a942656b976ffc 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 |  270 +
 1 files changed, 93 insertions(+), 177 deletions(-)

diff --git a/server/responder/nss/nsssrv_cmd.c 
b/server/responder/nss/nsssrv_cmd.c
index 405dae8..3b0f867 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 = 

Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-09 Thread Sumit Bose
On Wed, Sep 09, 2009 at 08:58:54AM -0400, Stephen Gallagher wrote:
 On 09/09/2009 08:46 AM, Sumit Bose wrote:
  On Tue, Sep 08, 2009 at 08:32:55PM -0400, Stephen Gallagher wrote:
  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.
 
  
  hmm, I must force myself not to use debugging option while testing
  patches ...
  
  I have seen this one:
  
  responder/nss/nsssrv_cmd.c: In function 'nss_cmd_getgrnam_callback':
  responder/nss/nsssrv_cmd.c:1728: warning: unused variable 'cache_ctx'
  
  
  bye,
  Sumit
  ___
  sssd-devel mailing list
  sssd-devel@lists.fedorahosted.org
  https://fedorahosted.org/mailman/listinfo/sssd-devel
 
 Thought I had removed all of those. That was in there for when I
 originally thought I would need to return something from check_cache to
 the caller.
 
 New patches attached.
 

ACK

bye,
Sumit
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-09 Thread Stephen Gallagher
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/09/2009 09:27 AM, Sumit Bose wrote:
 On Wed, Sep 09, 2009 at 08:58:54AM -0400, Stephen Gallagher wrote:
 On 09/09/2009 08:46 AM, Sumit Bose wrote:
 On Tue, Sep 08, 2009 at 08:32:55PM -0400, Stephen Gallagher wrote:
 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.


 hmm, I must force myself not to use debugging option while testing
 patches ...

 I have seen this one:
 
 responder/nss/nsssrv_cmd.c: In function 'nss_cmd_getgrnam_callback':
 responder/nss/nsssrv_cmd.c:1728: warning: unused variable 'cache_ctx'
 

 bye,
 Sumit
 ___
 sssd-devel mailing list
 sssd-devel@lists.fedorahosted.org
 https://fedorahosted.org/mailman/listinfo/sssd-devel

 Thought I had removed all of those. That was in there for when I
 originally thought I would need to return something from check_cache to
 the caller.

 New patches attached.

 
 ACK
 
 bye,
 Sumit
 ___
 sssd-devel mailing list
 sssd-devel@lists.fedorahosted.org
 https://fedorahosted.org/mailman/listinfo/sssd-devel

Pushed to master.


- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqnvYQACgkQeiVVYja6o6PIhQCgsVE5q69/ksiqtoLsVAj/O61u
84wAmwZTizHYKVJF4Zw6IB/UpevnnDwT
=opE9
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel


Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-09-08 Thread Stephen Gallagher
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 

Re: [SSSD] [PATCH] Add support for the EntryCacheNoWaitRefreshTimeout

2009-08-17 Thread Sumit Bose
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.

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.

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

 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 .

 +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 =