On Thu, Apr 17, 2014 at 07:25:09PM +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
Thanks for doing this work, there's been many bugs over the years as a
result of inconsitent return codes from different sysdb functions.
I have one more request, though -- would you mind sending a short mail
(without the [PATCH] tag) to this list explaining why you chose the
convention you did? I know we talked about the reasons in person but not
all developers are physically located in Brno :-)
>
>"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
>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) {
>+ ret = ENOENT;
Style police: Seems like there are two spaces between "=" and
"ENOENT".
>+ } else {
>+ ret = EOK;
>+ }
>
> done:
> talloc_zfree(tmp_ctx);
>--
>1.8.4.2
http://jhrozek.fedorapeople.org/sssd-coverage/html/db/sysdb_search.c.gcov...
80.0% of functions are covered.
Please do not decrease code coverage in sysdb. It is one of the most important
parts of sssd.
I agree. Not increasing the coverage is OK-ish, but we should not
decrease the coverage unless the module in question is extremely hard to
test (like the AD provider might be sometimes).