On Tue, Apr 22, 2014 at 12:20:22PM +0200, Lukas Slebodnik wrote:
On (22/04/14 12:02), Pavel Reichl wrote:
>On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
>> On (17/04/14 18:44), Pavel Reichl wrote:
>> >Hello,
>> >
>> >attached patch is the first of many to solve
>> >https://fedorahosted.org/sssd/ticket/1991
>> >
>> >"The return codes of various sysdb operations differ. Some search
>> >operations would return ENOENT if they don't find a matching object
some
>> >would return EOK but an empty result list."
>> >
>> >I think it would be best if in case that no results were found both
>> >ENOENT value and 'properly' empty list were returned.
>> >
>> >Thank for opinions or/and review.
>> >
>> >Pavel Reichl
>>
>> yet another comment.
>>
>> >From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001
>> >From: Pavel Reichl <preichl(a)redhat.com>
>> >Date: Wed, 26 Feb 2014 16:52:26 +0000
>> >Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
>> >
>> >sysdb_getnetgr did not return ENOENT althought it was expected.
>> >
>> >Resolves:
>> >https://fedorahosted.org/sssd/ticket/1991
>> >---
>> > src/db/sysdb.h | 1 +
>> > src/db/sysdb_search.c | 7 ++++++-
>> > 2 files changed, 7 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>> >index
5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644
>> >--- a/src/db/sysdb.h
>> >+++ b/src/db/sysdb.h
>> >@@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx {
>> > } value;
>> > };
>> >
>> >+/* If no group is found ENOENT is returned and result count is set to 0 */
>> > errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
>> > struct sss_domain_info *domain,
>> > const char *netgroup,
>> >diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
>> >index
60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644
>> >--- a/src/db/sysdb_search.c
>> >+++ b/src/db/sysdb_search.c
>> >@@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
>> > }
>> >
>> > *res = talloc_steal(mem_ctx, result);
>> >- ret = EOK;
>> >+
>> >+ if((*res)->count == 0) {
>> ^^^^^^
>> I do not see a reason of using output argument here. You can use local
>> variable result.
>OK, fixed.
>
>> >+ ret = ENOENT;
>> >+ } else {
>> >+ ret = EOK;
>> >+ }
>> >
>> > done:
>> > talloc_zfree(tmp_ctx);
>> >--
>> >1.8.4.2
>> >
>>
>> BTW
>> I would expect more reusable solution for ticket #1991 or do you want to add
>> the same snippet to other function?
>
>Well, actually I don't know yet. I think that there are too many
>different functions, some of them call directly ldap_search others call
^^^^^^^^^^^
Did you mean ldb_search?
>it indirectly. At first I would prefer to concentrate to fix
>implementation to expected behavior and mainly to don't break anything.
>
>But of course if/when I will see patterns for reusable solution I will
>propose it.
>
reusable solution can to use wrapper function for ldb_search
sss_ldb_search ???
ret = ldb_search( ...
ret = sysdb_error_to_errno(lret);
if(ret== EOK && result->count == 0) {
ret = ENOENT;
}
any objections from other developers?
This seems like a good way to go if we have at least two places that
would use this pattern.