[SSSD] [PATCHES] LDAP: fix ldap_setup_enumeration() handling ENOENT

Lukas Slebodnik lslebodn at redhat.com
Fri Nov 28 09:45:59 UTC 2014


On (04/11/14 10:10), Pavel Reichl wrote:
>Hello,
>
>please simple attached patches.
>
>Thanks.

>From f4eedb6ba21bf0c483ba8b037695ef02f5e61ed8 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <preichl at redhat.com>
>Date: Tue, 4 Nov 2014 08:52:54 +0000
>Subject: [PATCH 1/2] SYSDB: sysdb_get_bool() return ENOENT & unit tests
>
>sysdb_get_bool() return ENOENT if no result is found.
>Also unit test for sysdb_get_bool() & sysdb_set_bool() was added.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/1991
>---
Our CI would not like this patch.

Running suite(s): sysdb
99%: Checks: 827, Failures: 1, Errors: 0
../sssd/src/tests/sysdb-tests.c:4953:F:SYSDB Tests:test_sysdb_has_enumerated:0: Error [2][No such file or directory] checking enumeration

There should not be patch which breaks unit tests.

> src/db/sysdb.c          | 11 +++++++++--
> src/tests/sysdb-tests.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
>diff --git a/src/db/sysdb.c b/src/db/sysdb.c
>index 1f02585e747dda6aadde772f76f30d3d69c4cfc0..53230dc4a3ac67b7faf0c68f792a828502b0ffc1 100644
>--- a/src/db/sysdb.c
>+++ b/src/db/sysdb.c
>@@ -1508,6 +1508,7 @@ errno_t sysdb_get_bool(struct sysdb_ctx *sysdb,
>     errno_t ret;
>     int lret;
>     const char *attrs[2] = {attr_name, NULL};
>+    struct ldb_message_element *el;
> 
>     tmp_ctx = talloc_new(NULL);
>     if (tmp_ctx == NULL) {
>@@ -1530,7 +1531,7 @@ errno_t sysdb_get_bool(struct sysdb_ctx *sysdb,
>          * to contain this attribute.
>          */
>         *value = false;
>-        ret = EOK;
>+        ret = ENOENT;
I agree with this change.

>         goto done;
>     } else if (res->count != 1) {
>         DEBUG(SSSDBG_CRIT_FAILURE,
>@@ -1539,7 +1540,13 @@ errno_t sysdb_get_bool(struct sysdb_ctx *sysdb,
>         goto done;
>     }
> 
>-    *value = ldb_msg_find_attr_as_bool(res->msgs[0], attr_name, false);
>+    el = ldb_msg_find_element(res->msgs[0], attr_name);
>+    if (el == NULL || el->num_values == 0) {
>+        ret = ENOENT;
>+        goto done;
>+    } else {
>+        *value = ldb_msg_find_attr_as_bool(res->msgs[0], attr_name, false);
>+    }
But I'm not sure whether we need this one as well.
It makes sense to return ENOENT for users, groups or string.
In my opinion, default value "false" for boolean is good enough.


>From 325c84d5f9d685ab9793b357dc4070d3b926ced2 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <preichl at redhat.com>
>Date: Tue, 4 Nov 2014 08:55:55 +0000
>Subject: [PATCH 2/2] LDAP: fix ldap_setup_enumeration() handling ENOENT
>
>This patch fixes ldap_setup_enumeration() to handle ENOENT returned by
>sysdb_has_enumerated().
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/1991
>---
> src/providers/ldap/ldap_id_enum.c | 6 +++++-
> src/tests/sysdb-tests.c           | 7 +++----
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
>diff --git a/src/providers/ldap/ldap_id_enum.c b/src/providers/ldap/ldap_id_enum.c
>index 9ffa3e5d9737048e2f4508cea4edc28d6f8cda48..13d2a62544b3956165ef9eb480fb5b813c890fd4 100644
>--- a/src/providers/ldap/ldap_id_enum.c
>+++ b/src/providers/ldap/ldap_id_enum.c
>@@ -41,7 +41,11 @@ errno_t ldap_setup_enumeration(struct be_ctx *be_ctx,
>     struct ldap_enum_ctx *ectx;
> 
>     ret = sysdb_has_enumerated(sdom->dom, &has_enumerated);
>-    if (ret != EOK) {
>+    if (ret == ENOENT) {
>+        /* default value */
>+        has_enumerated = false;
>+        ret = EOK;
>+    } else if (ret != EOK) {
>         return ret;
>     }
> 
>diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
>index 7f1ca8755d7066788cb4706b923c5282417b59aa..c8be7f968160372c4ca23fa68671455f018d2a3f 100644
>--- a/src/tests/sysdb-tests.c
>+++ b/src/tests/sysdb-tests.c
>@@ -4943,10 +4943,9 @@ START_TEST(test_sysdb_has_enumerated)
>     fail_if(ret != EOK, "Could not set up the test");
> 
>     ret = sysdb_has_enumerated(test_ctx->domain, &enumerated);
>-    fail_if(ret != EOK, "Error [%d][%s] checking enumeration",
>-                        ret, strerror(ret));
>-
>-    fail_if(enumerated, "Enumeration should default to false");
>+    fail_if(ret != ENOENT,
>+            "Error [%d][%s] checking enumeration ENOENT is expected",
>+            ret, strerror(ret));
> 
>     ret = sysdb_set_enumerated(test_ctx->domain, true);
>     fail_if(ret != EOK, "Error [%d][%s] setting enumeration",
This test should be probably part of the 1st patch
becuase it fix failed test.

LS



More information about the sssd-devel mailing list