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(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?
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.
Thanks!