Re: [SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

[Pavel Reichl]

On 12/11/2014 06:35 PM, Michal Židek wrote:
> On 12/10/2014 03:24 PM, Lukas Slebodnik wrote:
> > On (10/12/14 14:48), Pavel Reichl wrote:
> > > On 12/10/2014 02:30 PM, Lukas Slebodnik wrote:
> > > > On (10/12/14 11:31), Pavel Reichl wrote:
> > > > > On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
> > > > > > On (10/12/14 10:09), Pavel Reichl wrote:
> > > > > > > On 12/10/2014 09:28 AM, Lukas Slebodnik wrote:
> > > > > > >> On (09/12/14 12:13), Pavel Reichl wrote:
> > > > > > >>> On 12/02/2014 12:50 PM, Lukas Slebodnik wrote:
> > > > > > >>>> On (29/10/14 17:17), Pavel Reichl wrote:
> > > > > > >>>>> Hello,
> > > > > > >>>>>
> > > > > > >>>>> please see attached patch.
> > > > > > >>>>>
> > > > > > >>>>> This patch is part of solution for
> > > > > > >>>>> https://fedorahosted.org/sssd/ticket/1991
> > > > > > >>>>> which aims to unify return values of sysdb calls in case no 
> > > > > > >>>>> results are
> > > > > > >>>>> found.
> > > > > > >>>>>
> > > > > > >>>> a) this patch cannot be applied on current master.
> > > > > > >>> rebased
> > > > > > >>>> b) I wrote similar patch becuse sssd_be crashed in function
> > > > > > >>>>      get_object_from_cache. sysdb_search_object_by_sid didn' 
> > > > > > >>>> returned ENOENT and
> > > > > > >>>>      res->msgs was NULL (result: dereference of NULL pointer)
> > > > > > >>>>      -- it is yet another result of broken IPA <-> AD trust
> > > > > > >>>> c) Could you compare your version and mine?
> > > > > > >>>>      Feel free to include my patch to yours if it is not good 
> > > > > > >>>> enogh.
> > > > > > >>> I don't say it was not good enough but I updated my patch.
> > > > > > >>>> d) test test_sysdb_delete_by_sid seems to be unrelated to 
> > > > > > >>>> this patch.
> > > > > > >>>>     I would prefer to have it in separete patch.
> > > > > > >>> I have moved it to separate patch. This patch is just to 
> > > > > > >>> verify that
> > > > > > >>> sysdb_delete_by_sid() was not changed by the second patch.
> > > > > > >>>> LS
> > > > > > >>>>
> > > > > > >>> Thanks, please see updated patches.
> > > > > > >> >From 53c3be375eff41dc70e17bb9e079ec98b8c2c5e7 Mon Sep 17 
> > > > > > >> 00:00:00 2001
> > > > > > >>> From: Pavel Reichl 
> > > > > > >>> Date: Tue, 9 Dec 2014 09:47:11 +0000
> > > > > > >>> Subject: [PATCH 1/2] TESTS: sysdb_delete_by_sid() test return 
> > > > > > >>> value
> > > > > > >>>
> > > > > > >>> Check that return value of sysdb_delete_by_sid() is not 
> > > > > > >>> changed as
> > > > > > >>> called SYSDB functions have changed the return value.
> > > > > > >>>
> > > > > > >>> Part of patches for:
> > > > > > >>> https://fedorahosted.org/sssd/ticket/1991
> > > > > > >>> ---
> > > > > > >> LGTM
> > > > > > >>
> > > > > > >> >From 1cb395244d3ab3906af5b6c8b294e80055ad0a07 Mon Sep 17 
> > > > > > >> 00:00:00 2001
> > > > > > >>> From: Pavel Reichl 
> > > > > > >>> 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
> > > > > > >>> ---
> > > > > > >>> src/db/sysdb.h                 |  2 +-
> > > > > > >>> src/db/sysdb_ops.c             | 70 
> > > > > > >>> ++++++------------------------------------
> > > > > > >>> src/responder/nss/nsssrv_cmd.c | 27 ++++++++--------
> > > > > > >>> src/responder/pac/pacsrv_cmd.c | 26 ++++++++++------
> > > > > > >>> src/tests/sysdb-tests.c        |  5 +--
> > > > > > >>> 5 files changed, 42 insertions(+), 88 deletions(-)
> > > > > > >>>
> > > > > > >>> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> > > > > > >>> index 
> > > > > > >>> 5bd7f90acb685bbaff5c98f433c7dce8175c33ca..9b88b4a63619456f8f3cc1961e29bbf3946ca5b8 
> > > > > > >>> 100644
> > > > > > >>> --- a/src/db/sysdb.h
> > > > > > >>> +++ b/src/db/sysdb.h
> > > > > > >>> @@ -1033,7 +1033,7 @@ errno_t 
> > > > > > >>> sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx,
> > > > > > >>>                                     struct sss_domain_info 
> > > > > > >>> *domain,
> > > > > > >>>                                     const char *sid_str,
> > > > > > >>>                                     const char **attrs,
> > > > > > >>> -                                   struct ldb_result **msg);
> > > > > > >>> +                                   struct ldb_result **res);
> > > > > > >>>
> > > > > > >>> errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
> > > > > > >>>                                      struct sss_domain_info 
> > > > > > >>> *domain,
> > > > > > >>> diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> > > > > > >>> index 
> > > > > > >>> 998046a2ca1c746b2032f430e5f9c4a7151e1dbc..8d6b11c248fd7f895ff6a95c25d4372fc7f8445d 
> > > > > > >>> 100644
> > > > > > >>> --- a/src/db/sysdb_ops.c
> > > > > > >>> +++ b/src/db/sysdb_ops.c
> > > > > > >>> @@ -2995,8 +2995,15 @@ int sysdb_delete_by_sid(struct 
> > > > > > >>> sysdb_ctx *sysdb,
> > > > > > >>>
> > > > > > >>>      ret = sysdb_search_object_by_sid(tmp_ctx, domain, 
> > > > > > >>> sid_str, NULL, &res);
> > > > > > >>>      if (ret != EOK) {
> > > > > > >>> -        DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d 
> > > > > > >>> (%s)\n",
> > > > > > >>> -              ret, strerror(ret));
> > > > > > >>> +        if (ret == ENOENT) {
> > > > > > >>> +            /* No existing entry. Just quit. */
> > > > > > >>> +            DEBUG(SSSDBG_TRACE_FUNC,
> > > > > > >>> +                  "search by sid did not return any 
> > > > > > >>> results.\n");
> > > > > > >>> +            ret = EOK;
> > > > > > >>> +        } else {
> > > > > > >>> +            DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: 
> > > > > > >>> %d (%s)\n",
> > > > > > >>> +                  ret, strerror(ret));
> > > > > > >>> +        }
> > > > > > >>>          goto done;
> > > > > > >> I'm sorry but this isn't very readable.
> > > > > > >> I prefer flat version.
> > > > > > >> If/else if/else is easier to read and if you want to do micro 
> > > > > > >> optimisation
> > > > > > >> then you can use switch case.
> > > > > > > I find it better readable this way - errors are handled in 
> > > > > > > single block. It's
> > > > > > > not against any code style rules I'm aware of. But I suppose if 
> > > > > > > this is the
> > > > > > > only way to get the patches pushed I'll do it your preferred way...
> > > > > > The problem is that I need to spend a lot of time with "parsing 
> > > > > > this code"
> > > > > > Someone can overlook goto done in this case and introduce a bug.
> > > > > > Your solution isn't bad but it isn't straightforward.
> > > > > >
> > > > > > This is especially problem in pac responder.
> > > > > > There are 3 nested levels of if. WHY?
> > > > > > I'm sorry but I can lost easily there.
> > > > > >   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;
> > > > > >          }
> > > > > >          DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to 
> > > > > > our cache!\n");
> > > > > >          return EIO;
> > > > > >      }
> > > > > >
> > > > > > //snip
> > > > > >
> > > > > > >>> @@ -912,9 +913,14 @@ 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");
> > > > > > >>> +            ret = EINVAL;
> > > > > >                  ^^^^^^^^^^^^^
> > > > > >              I found another interesting part.
> > > > > > Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) 
> > > > > > was not
> > > > > > special cased before your change.
> > > > >
> > > > > Please again see context:
> > > > > Before changing sysdb_search_object_by_sid() to return ENOENT on 
> > > > > no result
> > > > > EOK was returned and condition '(group->count != 1)' was true in 
> > > > > that case.
> > > > > So I just kept this behaviour. Is that clear now?
> > > >
> > > > Ahh I see. I overlook != 1 I was looking for  == 0.
> > > >
> > > > but still it would be better to return ENOENT. So we can 
> > > > distinguish between
> > > > group->count == 0 and group->count > 1. It should not happen very.
> > > >
> > > > LS
> > >
> > > hmm sounds right, I just check the calling side...and then update 
> > > the patch
> > > as you proposed.
> > >
> > > Michal would you be so kind to arbitrate mine and Lukas' little 
> > > dispute about
> > > the nesting ifs? Just say what would you prefer.
> >
> > +1 for different opinions.
> >
> > LS
>
> 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
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

OK, thank you both for opinions and for comments.

Please see updated patch.

Thanks!