[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Pavel Reichl preichl at redhat.com
Fri Dec 12 14:25:28 UTC 2014


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.

>
>> +    } else if (ret != EOK) {
>>          DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n");
>>          return EIO;
>>      }
>> @@ -4502,19 +4514,6 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
>>          return ENOENT;
>>      }
>>
>> -    if (dctx->res->count == 0 && !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;
>> -    }
>> -
> ^^^^
> @see removed code.
>
>>      /* if this is a caching provider (or if we haven't checked the cache
>>       * yet) then verify that the cache is uptodate */
>>      if (dctx->check_provider) {
>> diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
>> index cc92592893899b1fa269188facc1a8154f80991d..07d2f0cf79b70429dc7cf2784a8e31d651e5095f 100644
>> --- a/src/responder/pac/pacsrv_cmd.c
>> +++ b/src/responder/pac/pacsrv_cmd.c
>> @@ -911,10 +911,13 @@ 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");
>                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                                      This message can be little bit confusing.
>            We exactly know what's wrong. There isn't such object for SID.
>            It's better to be clear.
>
> 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