[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Lukas Slebodnik lslebodn at redhat.com
Fri Dec 12 15:10:18 UTC 2014


On (12/12/14 16:04), Pavel Reichl wrote:
>
>On 12/12/2014 03:52 PM, Lukas Slebodnik wrote:
>>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.
>yw
>>
>>But it does not change the fact that you change
>>the behaviour with this refactoring patch.
>>Please do such change in separate patch.
>I don't think this a good idea. The change in sysdb is changing behaviour -
>no doubt about it and I really think I have to amend the code that uses it.
>It is IMO a very bad idea send separate patch that introduces probable
>segfault and fix it in another.
>
I disagree. The segfault could be even with old code or did I miss something?
Please split it into separate patch. This change is not related to your ticket
and it's better to have ticket per issue.

LS



More information about the sssd-devel mailing list