[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Lukas Slebodnik lslebodn at redhat.com
Wed Dec 17 14:47:05 UTC 2014


On (17/12/14 15:27), Pavel Reichl wrote:
>
>On 12/15/2014 12:15 PM, Pavel Reichl wrote:
>>
>>On 12/15/2014 10:12 AM, Lukas Slebodnik wrote:
>>>On (12/12/14 17:10), Pavel Reichl wrote:
>>>>On 12/12/2014 04:10 PM, Lukas Slebodnik wrote:
>>>>>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?
>>>>I don't see how it could happen with old code, but if it did - I'll
>>>>send the
>>>>separate patch as you wish. Otherwise I would prefer to stick with my
>>>>current
>>>>patch.
>>>>How it could segfault?
>>>>
>>>Firstly, I would like to apologize that I didn't spend lot of time with
>>>a review. I just read the patch
>>>
>>>Let's summarise some facts.
>>>* your match is named: sysdb_search_object_by_sid returns ENOENT
>>>   sysdb_search_object_by_sid returns ENOENT if no results are found
>>>* It isn't possible to crash with current master.
>>>* You conceal a reason why there could be crash.
>>>   The function nss_cmd_getbysid_send_reply checks the result count for
>>>0,
>>>   which we wanted to fix .
>>>* You had to change behaviour of the function nss_cmd_getbysid_search
>>>   because there could be crash.
>>>It should be sign for you that something stinks in the code.
>>>
>>>Let's imagine situation. We have function1 which is called be function2
>>>which
>>>ic called by function3 which id called by function4. All these functions
>>>rely
>>>on fact that res->count is 0 they stop execution. You decided to convert
>>>function1 to return ENOENT instead of checking res->count to zero. All
>>>other
>>>functions should be converted as well because res needn't be initialized
>>>and
>>>they could crash. Such change should not be done in single patch. It
>>>would
>>>easily become mess a nightmare for reviewer. There is high probability of
>>>overlooking some places which was not converted properly. The right
>>>approach is
>>>to convert function from top to down (and not from down to top).
>>>Each patch will contain conversion of just one function:
>>>     patch1: function4 returns ENOENT
>>>     patch2: function3 returns ENOENT
>>>     ..
>>>Each patch will be simple and will not change the behaviour of other
>>>function.
>>>
>>>I hope it is clear to you now.
>>>Let me know whether you will sent patch ASAP or I should send temporary
>>>solution for ticket #2520
>>I think it will be best if you send temporally solution.  Acking process
>>for this patch is really cumbersome. :-(
>>>
>>>Summary: your solution is partially done therefore NACK
>>>
>>>LS
>>
>Updated patches attached.

ACK

LS



More information about the sssd-devel mailing list