On (12/12/14 16:04), Pavel Reichl wrote:
On 12/12/2014 03:52 PM, Lukas Slebodnik wrote:
>On (12/12/14 15:25), Pavel Reichl wrote:
>>On 12/12/2014 03:02 PM, Lukas Slebodnik wrote:
>>>On (12/12/14 14:25), Pavel Reichl wrote:
>>>>On 12/11/2014 06:35 PM, Michal Židek wrote:
>>>>>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
>>>>>
>>>>OK, thank you both for opinions and for comments.
>>>>
>>>>Please see updated patch.
>>>>
>>>>Thanks!
>>>Two questions/comments are inline.
>>>
>>>>From 94eade7b3c0c0729f5d4a322061c7c9688bd8130 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
>>>>
>>>>Fixes:
>>>>https://fedorahosted.org/sssd/ticket/2520
>>>>---
>>>>src/db/sysdb.h | 2 +-
>>>>src/db/sysdb_ops.c | 68
++++++------------------------------------
>>>>src/responder/nss/nsssrv_cmd.c | 27 ++++++++---------
>>>>src/responder/pac/pacsrv_cmd.c | 29 ++++++++++--------
>>>>src/tests/sysdb-tests.c | 5 +---
>>>>5 files changed, 40 insertions(+), 91 deletions(-)
>>>>errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
>>>>diff --git a/src/responder/nss/nsssrv_cmd.c
b/src/responder/nss/nsssrv_cmd.c
>>>>index
ea58920bc3611c84958e8d0aca0e122d90c68e5c..3c5d450714fb3f7655cd32aeef900b4f5e9782c7 100644
>>>>--- a/src/responder/nss/nsssrv_cmd.c
>>>>+++ b/src/responder/nss/nsssrv_cmd.c
>>>>@@ -4491,7 +4491,19 @@ static errno_t nss_cmd_getbysid_search(struct
nss_dom_ctx *dctx)
>>>>
>>>> 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;
>>>You changed behaviour with this nesting if statements.
>>>ENOENT will be returned every time. Previously it was returned just in case
of
>>>true condition (dctx->res->count == 0 &&
!dctx->check_provider)
>>>Could you explain such change?
>>Because otherwise 'res' is accessed in case of ENOENT which implies
segfault
>>in nss_cmd_getbysid_send_reply() or even earlier in check_cache(), if I read
>>the code correctly.
>>
>Thank you for explanation.
yw
>
>But it does not change the fact that you change
>the behaviour with this refactoring patch.
>Please do such change in separate patch.
I don't think this a good idea. The change in sysdb is changing behaviour -
no doubt about it and I really think I have to amend the code that uses it.
It is IMO a very bad idea send separate patch that introduces probable
segfault and fix it in another.
I disagree. The segfault could be even with old code or did I miss something?
Please split it into separate patch. This change is not related to your ticket
and it's better to have ticket per issue.
LS