[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Sumit Bose sbose at redhat.com
Wed Dec 17 14:59:50 UTC 2014


On Wed, Dec 17, 2014 at 03:47:05PM +0100, Lukas Slebodnik wrote:
> 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

Pushed to master:
    13c0cf829eca7891ad9d0087e91c72650f990149
    d7b90921c1a404f0d9fb8384a8fd55fd15b86916
    4bbcc2d6d3f16b015796818746a45134861c93a4

bye,
Sumit
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list