On 12/10/2014 03:24 PM, Lukas Slebodnik wrote:
> 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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel OK, thank you both for
opinions and for comments.
Please see updated patch.
Thanks!