[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Pavel Reichl preichl at redhat.com
Wed Dec 10 13:48:56 UTC 2014


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 <preichl at redhat.com>
>>>>>> 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 <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
>>>>>> ---
>>>>>> 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.
> _______________________________________________
> sssd-devel mailing list

Michal would you be so kind to arbitrate mine and Lukas' little dispute 
about the nesting ifs? Just say what would you prefer.

Thanks!
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list