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

Lukas Slebodnik lslebodn at redhat.com
Fri Nov 28 15:35:15 UTC 2014


On (28/11/14 16:09), Pavel Reichl wrote:
>
>On 11/28/2014 10:45 AM, Lukas Slebodnik wrote:
>>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.
>OK, I squeeze the patches into one.
>>
>>>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.
>Hmm, I considered your opinion, but I think mine approach might be better
>because there are 2 valid scenarios when this function can return ENOENT:
>
>1) ldb_search return res->count equal to 0, because there is nothing on such
>DN
>2) there exist such DN but there's not such attribute.
>
>I believe that caller should be informed by returning  ENOENT in both cases
>and not hiding second failure by returning default value - which is IMO not
>clear.
>
As you want.

>If we stick with my version I should update unit test - to test for both
>possible ENOENT events.
>
+1

I have two another comments.

>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
>---
> 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;
>         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);
>+    }
The else branch isn't necessare here.
because there is "goto done" in true branch
In my opinion, it easier to read code without many if/else statements.

>     ret = EOK;
> 
>diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
>index e01ddf4782c0a5a557f39d1adc2efd74b6234461..7f1ca8755d7066788cb4706b923c5282417b59aa 100644
>--- a/src/tests/sysdb-tests.c
>+++ b/src/tests/sysdb-tests.c
>@@ -3474,6 +3474,48 @@ START_TEST (test_sysdb_memberof_user_cleanup)
> }
> END_TEST
> 
>+START_TEST (test_sysdb_set_get_bool)
>+{
>+    struct sysdb_test_ctx *test_ctx;
>+    struct ldb_dn *dn;
>+    bool value;
>+    int ret;
>+    const char *attr_val = "BOOL_VALUE";
>+
>+    /* Setup */
>+    ret = setup_sysdb_tests(&test_ctx);
>+    if (ret != EOK) {
>+        fail("Could not set up the test");
>+        return;
>+    }
>+
>+    dn = ldb_dn_new_fmt(test_ctx, test_ctx->sysdb->ldb, SYSDB_DOM_BASE,
>+                        test_ctx->domain->name);
We have function for this purpose sysdb_domain_dn

LS



More information about the sssd-devel mailing list