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.
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.
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.
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.
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.
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.
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.
I agree too. Compilers are smart enough to do this kind of optimization.
LS
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.
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
On Tue, Sep 24, 2013 at 04:06:22PM +0200, Lukas Slebodnik wrote:
@@ -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/
It's a typo and thanks for the catch. New patches are attached.
On (24/09/13 16:10), Jakub Hrozek wrote:
On Tue, Sep 24, 2013 at 04:06:22PM +0200, Lukas Slebodnik wrote:
@@ -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/It's a typo and thanks for the catch. New patches are attached.
I check ldap log (on server). Next query for non existing user apeared after 15 seconds (default value of entry_negative_timeout) and reporter had already verified patch.
ACK
LS
On Tue, Sep 24, 2013 at 05:15:47PM +0200, Lukas Slebodnik wrote:
On (24/09/13 16:10), Jakub Hrozek wrote:
On Tue, Sep 24, 2013 at 04:06:22PM +0200, Lukas Slebodnik wrote:
@@ -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/It's a typo and thanks for the catch. New patches are attached.
I check ldap log (on server). Next query for non existing user apeared after 15 seconds (default value of entry_negative_timeout) and reporter had already verified patch.
ACK
LS
Thank you for the careful review, pushed the patches to master and sssd-1-11.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Simo Sorce