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 = 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.
> break;
>
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
