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 <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.
>
+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