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.
>From 1ec4e44e9ccdabaf5b84a26416cba11092efa412 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> 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 | 69 ++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 36 deletions(-) diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 229548976744220b6f10b184a28a8ac8c11559a7..648950acebcbbb441cc3e1882a989ad69c7088ef 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -1236,7 +1236,7 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) dom = get_next_domain(dom, true); continue; } - return ENOENT; + goto notfound; } if (dom != dctx->domain) { @@ -1264,7 +1264,7 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) if (dctx->res->count > 1) { DEBUG(0, ("getpwuid call returned more than one result !?!\n")); - return ENOENT; + goto notfound; } if (dctx->res->count == 0 && !dctx->check_provider) { @@ -1274,15 +1274,9 @@ 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")); + goto notfound; } /* if this is a caching provider (or if we haven't checked the cache @@ -1306,7 +1300,13 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) return EOK; } - DEBUG(2, ("No matching domain found for [%d], fail!\n", cmdctx->id)); +notfound: + 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 ENOENT; } @@ -2668,7 +2668,7 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) dom = get_next_domain(dom, true); continue; } - return ENOENT; + goto notfound; } if (dom != dctx->domain) { @@ -2696,7 +2696,7 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) if (dctx->res->count > 1) { DEBUG(0, ("getgrgid call returned more than one result !?!\n")); - return ENOENT; + goto notfound; } if (dctx->res->count == 0 && !dctx->check_provider) { @@ -2706,15 +2706,9 @@ 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")); + goto notfound; } /* if this is a caching provider (or if we haven't checked the cache @@ -2738,7 +2732,13 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) return EOK; } - DEBUG(2, ("No matching domain found for [%d], fail!\n", cmdctx->id)); +notfound: + 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 ENOENT; } @@ -3806,20 +3806,7 @@ 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; + goto notfound; } /* if this is a caching provider (or if we haven't checked the cache @@ -3864,9 +3851,19 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) return EOK; } +notfound: 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)); -- 1.8.3.1
>From bf06d844ff432cf3b7a26ddf541a67978e82b022 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Wed, 18 Sep 2013 15:49:46 +0200 Subject: [PATCH 2/2] NSS: Failure to store entry negative cache should not be fatal The only effect the failure to store a result to negative cache might have would be a slower lookup next time. --- src/responder/nss/nsssrv_cmd.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 648950acebcbbb441cc3e1882a989ad69c7088ef..11616a8d8f1636d34aabf52bf439d2f73377dade 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -773,7 +773,8 @@ static int nss_cmd_getpwnam_search(struct nss_dom_ctx *dctx) /* set negative cache only if not result of cache check */ ret = sss_ncache_set_user(nctx->ncache, false, dom, name); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot set negcache for %s@%s\n", + name, dom->name)); } /* if a multidomain search, try with next */ @@ -1303,7 +1304,9 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx) notfound: ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id); if (ret != EOK) { - return ret; + /* This should not be fatal, we'll just hit the DP next time */ + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negative cache for UID %d\n", cmdctx->id)); } DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id)); @@ -2583,7 +2586,8 @@ static int nss_cmd_getgrnam_search(struct nss_dom_ctx *dctx) /* set negative cache only if not result of cache check */ ret = sss_ncache_set_group(nctx->ncache, false, dom, name); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot set negcache for %s@%s\n", + name, dom->name)); } /* if a multidomain search, try with next */ @@ -2735,7 +2739,8 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx) notfound: ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negative cache for GID %d\n", cmdctx->id)); } DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id)); @@ -3561,7 +3566,8 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx) /* set negative cache only if not result of cache check */ ret = sss_ncache_set_user(nctx->ncache, false, dom, name); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot set negcache for %s@%s\n", + name, dom->name)); } /* if a multidomain search, try with next */ @@ -3791,12 +3797,14 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx) if (cmdctx->cmd == SSS_NSS_GETSIDBYNAME) { ret = sss_ncache_set_user(nctx->ncache, false, dom, name); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negcache for %s@%s\n", name, dom->name)); } ret = sss_ncache_set_group(nctx->ncache, false, dom, name); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negcache for %s@%s\n", name, dom->name)); } } /* if a multidomain search, try with next */ @@ -3857,12 +3865,14 @@ notfound: ("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; + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negative cache for UID %d\n", cmdctx->id)); } ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negative cache for GID %d\n", cmdctx->id)); } } else { DEBUG(SSSDBG_MINOR_FAILURE, @@ -3911,7 +3921,8 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx) /* set negative cache only if not result of cache check */ ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid); if (ret != EOK) { - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, + ("Cannot set negative cache for %s\n", cmdctx->secid)); } return ENOENT; -- 1.8.3.1
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
