On (20/09/13 14:28), Jakub Hrozek wrote:
On Fri, Sep 20, 2013 at 11:00:18AM +0200, Lukas Slebodnik wrote:
On (20/09/13 10:54), Pavel Březina wrote:
On 09/20/2013 02:30 AM, Simo Sorce wrote:
On Thu, 2013-09-19 at 18:45 +0200, Jakub Hrozek wrote:
On Wed, Sep 18, 2013 at 10:40:13PM +0200, Jakub Hrozek wrote:
On Wed, Sep 18, 2013 at 01:41:36PM -0400, Simo Sorce wrote: >On Wed, 2013-09-18 at 17:58 +0200, Jakub Hrozek wrote: >>Hi, >> >>the first patch fixes the bug that Jean-Baptiste Denis found earlier >>today. In case the lookup was not done using a FQDN, the code would skip >>setting the entries to the ncache. >> >>The second patch is an incremental improvement. I don't think we should >>abort the whole lookup if setting an entry in negcache would fail. The >>negative cache is a performance optimization after all. > >It seem to me that with the first patch you are changing behavior as you >leave 'ret' unchanged to whatever error is returned instead of setting >it to ENOENT before going to 'done'. > >Simo.
Ugh, that is a bug.
I was going back and forth on changing the particular return to goto and when I made my mind, I forgot to set the errno.
Thanks Simo for catching it, I'll prepare a new version.
I think we were both confused by the poor label naming. The label actually, despite being named "done", made the function return ENOENT so the code was correct. But even I was confused couple of hours after sending the patch so the code had to be improved :)
So in the attached patch I renamed the label to notfound. I hope that's OK if we don't use the commonly used "done" in this case. I think the most important thing is that there is only a single label we jump to.
Alternatively, if you prefer strictly one exit point, I could convert the function to only use goto done and set negcache based on errno value (if ENOENT->ncache) but I didn't see that as necessary.
I prefer the idiom: ret = ENOENT; goto done;
to: goto notfound;
Simo.
I agree. We should not introduce new labels.
Well, I think this particular usage was OK as it was strictly a "exception", but I won't argue too hard. I also didn't want to change too much code in a bugfix patch..
I agree too. Compilers are smart enough to do this kind of optimization.
LS
Yeah, my point was minimal code change during a bugfix that might make its way even to a stable branch (as this bug affects even sssd-1-9) rather than optimization..
Anyway, new patches are attached.
From d98862aa128aee9aeec9af14ff00fcd57a254091 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 18 Sep 2013 15:42:41 +0200 Subject: [PATCH 1/2] NSS: Set UID and GID to negative cache after searching all domains
https://fedorahosted.org/sssd/ticket/2090
Previously, when searching by UID or GID, the negative cache will only work in case the UID was searched for using fully qualified names.
src/responder/nss/nsssrv_cmd.c | 171 +++++++++++++++++++++++++---------------- 1 file changed, 105 insertions(+), 66 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 229548976744220b6f10b184a28a8ac8c11559a7..ad78b47a664c986b2040f53e8fbf8be0c96c6c6e 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -1236,7 +1236,8 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) dom = get_next_domain(dom, true); continue; }
return ENOENT;
ret = ENOENT;goto done; } if (dom != dctx->domain) {@@ -1253,18 +1254,21 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) sysdb = dom->sysdb; if (sysdb == NULL) { DEBUG(0, ("Fatal: Sysdb CTX not found for this domain!\n"));
return EIO;
ret = EIO;goto done; } ret = sysdb_getpwuid(cmdctx, sysdb, dom, cmdctx->id, &dctx->res); if (ret != EOK) { DEBUG(1, ("Failed to make request to our cache!\n"));
return EIO;
ret = EIO;goto done; } if (dctx->res->count > 1) { DEBUG(0, ("getpwuid call returned more than one result !?!\n"));
return ENOENT;
ret = ENOENT;goto done; } if (dctx->res->count == 0 && !dctx->check_provider) {@@ -1274,15 +1278,10 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) continue; }
DEBUG(2, ("No results for getpwuid call\n"));/* set negative cache only if not result of cache check */ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}return ENOENT;
DEBUG(SSSDBG_MINOR_FAILURE, ("No results for getpwuid call\n"));ret = ENOENT;goto done; } /* if this is a caching provider (or if we haven't checked the cache@@ -1296,18 +1295,30 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) /* Anything but EOK means we should reenter the mainloop * because we may be refreshing the cache */
return ret;
goto done; } } /* One result found */ DEBUG(6, ("Returning info for uid [%d@%s]\n", cmdctx->id, dom->name));
return EOK;
ret = EOK;goto done;
- DEBUG(2, ("No matching domain found for [%d], fail!\n", cmdctx->id));
- return ENOENT;
- /* All domains were tried and none had the entry. */
- ret = ENOENT;
+done:
- if (ret == ENOENT) {
/* The entry was not found, need to set result in negative cache */ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}- }
- DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id));
- return ret;
}
static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx); @@ -2668,7 +2679,8 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) dom = get_next_domain(dom, true); continue; }
return ENOENT;
ret = ENOENT;goto done; } if (dom != dctx->domain) {@@ -2685,18 +2697,21 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) sysdb = dom->sysdb; if (sysdb == NULL) { DEBUG(0, ("Fatal: Sysdb CTX not found for this domain!\n"));
return EIO;
ret = EIO;goto done; } ret = sysdb_getgrgid(cmdctx, sysdb, dom, cmdctx->id, &dctx->res); if (ret != EOK) { DEBUG(1, ("Failed to make request to our cache!\n"));
return EIO;
ret = EIO;goto done; } if (dctx->res->count > 1) { DEBUG(0, ("getgrgid call returned more than one result !?!\n"));
return ENOENT;
ret = ENOENT;goto done; } if (dctx->res->count == 0 && !dctx->check_provider) {@@ -2706,15 +2721,10 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) continue; }
DEBUG(2, ("No results for getgrgid call\n"));/* set negative cache only if not result of cache check */ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}return ENOENT;
DEBUG(SSSDBG_MINOR_FAILURE, ("No results for getgrgid call\n"));ret = ENOENT;goto done; } /* if this is a caching provider (or if we haven't checked the cache@@ -2728,18 +2738,31 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) /* Anything but EOK means we should reenter the mainloop * because we may be refreshing the cache */
return ret;
goto done; } } /* One result found */ DEBUG(6, ("Returning info for gid [%d@%s]\n", cmdctx->id, dom->name));
return EOK;
/* Success. Break from the loop and return EOK */ret = EOK;goto done;
- DEBUG(2, ("No matching domain found for [%d], fail!\n", cmdctx->id));
- return ENOENT;
- /* All domains were tried and none had the entry. */
- ret = ENOENT;
+done:
- if (ret == ENOENT) {
/* The entry was not found, need to set result in negative cache */ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}- }
- DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id));
- return ret;
}
static int nss_cmd_getgrgid(struct cli_ctx *cctx) @@ -3639,7 +3662,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) dom = get_next_domain(dom, true); continue; }
return ENOENT;
ret = ENOMEM;
^^^^^^ Is it aim or mistake? s/ENOMEM/ENOENT/
goto done; } } else { /* if it is a domainless search, skip domains that require fully@@ -3670,7 +3694,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) name = sss_get_cased_name(cmdctx, cmdctx->name, dom->case_sensitive); if (name == NULL) { DEBUG(SSSDBG_OP_FAILURE, ("sss_get_cased_name failed.\n"));
return ENOMEM;
ret = ENOMEM;goto done; } /* For subdomains a fully qualified name is needed for@@ -3679,7 +3704,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) sysdb_name = sss_tc_fqname(cmdctx, dom->names, dom, name); if (sysdb_name == NULL) { DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n"));
return ENOMEM;
ret = ENOMEM;goto done; } }@@ -3702,7 +3728,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) /* There are no further domains or this was a * fully-qualified user request. */
return ENOENT;
ret = ENOENT;goto done; } DEBUG(SSSDBG_TRACE_FUNC, ("Requesting info for [%s@%s]\n",@@ -3714,7 +3741,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) if (sysdb == NULL) { DEBUG(SSSDBG_FATAL_FAILURE, ("Fatal: Sysdb CTX not found for this domain!\n"));
return EIO;
ret = EIO;goto done; } if (cmdctx->cmd == SSS_NSS_GETSIDBYID) {@@ -3723,7 +3751,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) if (ret != EOK && ret != ENOENT) { DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to make request to our cache!\n"));
return EIO;
ret = EIO;goto done; } if (ret == EOK) {@@ -3735,7 +3764,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) if (ret != EOK && ret != ENOENT) { DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to make request to our cache!\n"));
return EIO;
ret = EIO;goto done; } if (ret == EOK) {@@ -3749,7 +3779,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) if (ret != EOK && ret != ENOENT) { DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to make request to our cache!\n"));
return EIO;
ret = EIO;goto done; } if (ret == EOK) {@@ -3762,7 +3793,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) if (ret != EOK && ret != ENOENT) { DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to make request to our cache!\n"));
return EIO;
ret = EIO;goto done; } if (ret == EOK) {@@ -3774,7 +3806,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) dctx->res = talloc_zero(cmdctx, struct ldb_result); if (dctx->res == NULL) { DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));
return ENOMEM;
ret = ENOMEM;goto done; } if (user_found || group_found) {@@ -3782,7 +3815,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) dctx->res->msgs = talloc_array(dctx->res, struct ldb_message *, 1); if (dctx->res->msgs == NULL) { DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
return ENOMEM;
ret = ENOMEM;goto done; } dctx->res->msgs[0] = talloc_steal(dctx->res, msg); }@@ -3806,20 +3840,8 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) }
DEBUG(SSSDBG_OP_FAILURE, ("No matching user or group found.\n"));
if (cmdctx->cmd == SSS_NSS_GETSIDBYID) {ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}}return ENOENT;
ret = ENOENT;goto done; } /* if this is a caching provider (or if we haven't checked the cache@@ -3848,7 +3870,7 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) /* Anything but EOK means we should reenter the mainloop * because we may be refreshing the cache */
return ret;
goto done; } }@@ -3861,17 +3883,34 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) name, dom->name)); }
return EOK;
/* Success. Break from the loop and return EOK */ret = EOK;goto done;
- if (cmdctx->cmd == SSS_NSS_GETSIDBYID) {
DEBUG(SSSDBG_MINOR_FAILURE,("No matching domain found for [%d], fail!\n", cmdctx->id));- } else {
DEBUG(SSSDBG_MINOR_FAILURE,("No matching domain found for [%s], fail!\n", cmdctx->name));
- /* All domains were tried and none had the entry. */
- ret = ENOENT;
+done:
- if (ret == ENOENT) {
/* The entry was not found, need to set result in negative cache */if (cmdctx->cmd == SSS_NSS_GETSIDBYID) {DEBUG(SSSDBG_MINOR_FAILURE,("No matching domain found for [%d], fail!\n", cmdctx->id));ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);if (ret != EOK) {return ret;}} else {DEBUG(SSSDBG_MINOR_FAILURE,("No matching domain found for [%s], fail!\n", cmdctx->name));}
- return ENOENT;
- return ret;
}
static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
1.8.3.1
From 65e1d921c6109614550ddfc09279a89f0801b27b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 18 Sep 2013 15:49:46 +0200 Subject: [PATCH 2/2] NSS: Failure to store entry negative cache should not be fatal
2nd patch ACK
LS