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