[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT
Pavel Reichl
preichl at redhat.com
Fri Dec 12 13:25:21 UTC 2014
On 12/11/2014 06:35 PM, Michal Židek wrote:
> 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 at 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 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
>>>>>>>>> ---
>>>>>>>>> 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 at 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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TESTS-sysdb_delete_by_sid-test-return-value.patch
Type: text/x-patch
Size: 2071 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141212/835e96a8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SYSDB-sysdb_search_object_by_sid-returns-ENOENT.patch
Type: text/x-patch
Size: 9666 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141212/835e96a8/attachment-0001.bin>
More information about the sssd-devel
mailing list