[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Lukas Slebodnik lslebodn at redhat.com
Fri Dec 12 14:02:20 UTC 2014


On (12/12/14 14:25), Pavel Reichl wrote:
>On 12/11/2014 06:35 PM, Michal Židek wrote:
>>I think that smaller nesting levels are better.
>>
>>In general I would do
>>
>>if (ret == SPECIFIC_CODE) {
>>   specific_error_handle_block
>>} else (ret != EOK) {
>>   general_error_handle_block
>>}
>>
>>If there are many specific error codes that need to be addressed,
>>use switch, but 1-2 else-ifs are ok to me.
>>
>>This pattern can be used if common_to_all_erros_action exists (and is
>>something more than just goto done):
>>
>>if (ret != EOK) {
>>   if (ret == SPECIFIC_CODE) {
>>       specific_error_handle_block
>>   }
>>   common_to_all_errors_action;
>>}
>>
>>So I would slightly prefer Lukas's solution over Pavel's.
>>
>>In this particullar case I would do:
>>if (ret == ENOENT) {
>>    ....
>>} else if (ret != EOK) {
>>    ....
>>}
>>
>>Michal
>>
>OK, thank you both for opinions and for comments.
>
>Please see updated patch.
>
>Thanks!

Two questions/comments are inline.

>From 94eade7b3c0c0729f5d4a322061c7c9688bd8130 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <preichl at redhat.com>
>Date: Tue, 9 Dec 2014 11:01:13 +0000
>Subject: [PATCH 2/2] SYSDB: sysdb_search_object_by_sid returns ENOENT
>
>sysdb_search_object_by_sid returns ENOENT if no results are found.
>
>Part od solution for:
>https://fedorahosted.org/sssd/ticket/1991
>
>Fixes:
>https://fedorahosted.org/sssd/ticket/2520
>---
> src/db/sysdb.h                 |  2 +-
> src/db/sysdb_ops.c             | 68 ++++++------------------------------------
> src/responder/nss/nsssrv_cmd.c | 27 ++++++++---------
> src/responder/pac/pacsrv_cmd.c | 29 ++++++++++--------
> src/tests/sysdb-tests.c        |  5 +---
> 5 files changed, 40 insertions(+), 91 deletions(-)
> errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
>diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
>index ea58920bc3611c84958e8d0aca0e122d90c68e5c..3c5d450714fb3f7655cd32aeef900b4f5e9782c7 100644
>--- a/src/responder/nss/nsssrv_cmd.c
>+++ b/src/responder/nss/nsssrv_cmd.c
>@@ -4491,7 +4491,19 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
> 
>     ret = sysdb_search_object_by_sid(cmdctx, dom, cmdctx->secid, NULL,
>                                      &dctx->res);
>-    if (ret != EOK) {
>+    if (ret == ENOENT) {
>+        if (!dctx->check_provider) {
>+            DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid call.\n");
>+
>+            /* set negative cache only if not result of cache check */
>+            ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid);
>+            if (ret != EOK) {
>+                DEBUG(SSSDBG_MINOR_FAILURE,
>+                      "Cannot set negative cache for %s\n", cmdctx->secid);
>+            }
>+        }
>+        return ENOENT;
You changed behaviour with this nesting if statements.
ENOENT will be returned every time. Previously it was returned just in case of
true condition (dctx->res->count == 0 && !dctx->check_provider)
Could you explain such change?

>+    } else if (ret != EOK) {
>         DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n");
>         return EIO;
>     }
>@@ -4502,19 +4514,6 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
>         return ENOENT;
>     }
> 
>-    if (dctx->res->count == 0 && !dctx->check_provider) {
>-        DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid call.\n");
>-
>-        /* set negative cache only if not result of cache check */
>-        ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid);
>-        if (ret != EOK) {
>-            DEBUG(SSSDBG_MINOR_FAILURE,
>-                  "Cannot set negative cache for %s\n", cmdctx->secid);
>-        }
>-
>-        return ENOENT;
>-    }
>-
^^^^
@see removed code.

>     /* if this is a caching provider (or if we haven't checked the cache
>      * yet) then verify that the cache is uptodate */
>     if (dctx->check_provider) {
>diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
>index cc92592893899b1fa269188facc1a8154f80991d..07d2f0cf79b70429dc7cf2784a8e31d651e5095f 100644
>--- a/src/responder/pac/pacsrv_cmd.c
>+++ b/src/responder/pac/pacsrv_cmd.c
>@@ -911,10 +911,13 @@ pac_store_membership(struct pac_req_ctx *pr_ctx,
> 
>     ret = sysdb_search_object_by_sid(tmp_ctx, grp_dom, grp_sid_str,
>                                      group_attrs, &group);
>-    if (ret != EOK) {
>-        DEBUG(SSSDBG_TRACE_INTERNAL, "sysdb_search_object_by_sid " \
>-                                      "for SID [%s] failed [%d][%s].\n",
>-                                      grp_sid_str, ret, strerror(ret));
>+    if (ret == ENOENT) {
>+        DEBUG(SSSDBG_OP_FAILURE, "Unexpected number of groups returned.\n");
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                    This message can be little bit confusing.
          We exactly know what's wrong. There isn't such object for SID.
          It's better to be clear.

LS



More information about the sssd-devel mailing list