[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