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(a)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(a)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?
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;
+ } else {
+ DEBUG(SSSDBG_TRACE_INTERNAL, "sysdb_search_object_by_sid " \
+ "for SID [%s] failed [%d][%s].\n",
+ grp_sid_str, ret, strerror(ret));
+ }
goto done;
}
if (group->count != 1) {
DEBUG(SSSDBG_OP_FAILURE, "Unexpected number of groups
returned.\n");
ret = EINVAL;
goto done;
}
BTW please add ticket
https://fedorahosted.org/sssd/ticket/2520 to commit
message because this patch fixes the crash.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel