[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Lukas Slebodnik lslebodn at redhat.com
Fri Dec 12 14:52:48 UTC 2014


On (12/12/14 15:25), Pavel Reichl wrote:
>
>On 12/12/2014 03:02 PM, Lukas Slebodnik wrote:
>>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?
>Because otherwise 'res' is accessed in case of ENOENT which implies segfault
>in nss_cmd_getbysid_send_reply() or even earlier in check_cache(), if I read
>the code correctly.
>
Thank you for explanation.

But it does not change the fact that you change
the behaviour with this refactoring patch.
Please do such change in separate patch.

LS



More information about the sssd-devel mailing list