On 11/28/2014 04:35 PM, Lukas Slebodnik wrote:
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 <prei...@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 <prei...@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.
OK, I know Jakub prefers such style too so I'll try to remember next time.
Done.
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
OK, but please note that same code is a must (at least i think so) for
testing non-existing DN in the same test.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>From badbf490741154e404ee3bd10a3dfb1c2b045aba Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 4 Nov 2014 08:52:54 +0000
Subject: [PATCH] SYSDB: sysdb_get_bool() return ENOENT & unit tests
sysdb_get_bool() return ENOENT if no result is found.
Unit test for sysdb_get_bool() & sysdb_set_bool() was added.
This patch also fixes ldap_setup_enumeration() to handle ENOENT returned by
sysdb_has_enumerated().
Resolves:
https://fedorahosted.org/sssd/ticket/1991
---
src/db/sysdb.c | 9 +++++-
src/providers/ldap/ldap_id_enum.c | 6 +++-
src/tests/sysdb-tests.c | 62 ++++++++++++++++++++++++++++++++++++---
3 files changed, 71 insertions(+), 6 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 1f02585e747dda6aadde772f76f30d3d69c4cfc0..61a2240016b5cb77e6fbbc3286fd1a194c5a0b48 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,6 +1540,12 @@ errno_t sysdb_get_bool(struct sysdb_ctx *sysdb,
goto done;
}
+ el = ldb_msg_find_element(res->msgs[0], attr_name);
+ if (el == NULL || el->num_values == 0) {
+ ret = ENOENT;
+ goto done;
+ }
+
*value = ldb_msg_find_attr_as_bool(res->msgs[0], attr_name, false);
ret = EOK;
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 26835d9eb3647c99a8cf50d98143088dc0655f10..bbfc88b3e351c96131bd58e06c3ba6d7803c733b 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -3474,6 +3474,58 @@ 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, *ne_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 = sysdb_domain_dn(test_ctx, test_ctx->domain);
+ fail_unless(dn != NULL);
+
+ /* attribute is not created yet */
+ ret = sysdb_get_bool(test_ctx->sysdb, dn, attr_val,
+ &value);
+ fail_unless(ret == ENOENT,
+ "sysdb_get_bool returned %d:[%s], but ENOENT is expected",
+ ret, sss_strerror(ret));
+
+ /* add attribute */
+ ret = sysdb_set_bool(test_ctx->sysdb, dn, test_ctx->domain->name,
+ attr_val, true);
+ fail_unless(ret == EOK);
+
+ /* successfully obtain attribute */
+ ret = sysdb_get_bool(test_ctx->sysdb, dn, attr_val,
+ &value);
+ fail_unless(ret == EOK, "sysdb_get_bool failed %d:[%s]",
+ ret, sss_strerror(ret));
+ fail_unless(value == true);
+
+ /* use non-existing DN */
+ ne_dn = ldb_dn_new_fmt(test_ctx, test_ctx->sysdb->ldb, SYSDB_DOM_BASE,
+ "non-existing domain");
+ fail_unless(ne_dn != NULL);
+ ret = sysdb_get_bool(test_ctx->sysdb, ne_dn, attr_val,
+ &value);
+ fail_unless(ret == ENOENT,
+ "sysdb_get_bool returned %d:[%s], but ENOENT is expected",
+ ret, sss_strerror(ret));
+
+ /* free ctx */
+ talloc_free(test_ctx);
+}
+END_TEST
+
START_TEST (test_sysdb_attrs_to_list)
{
struct sysdb_attrs *attrs_list[3];
@@ -4907,10 +4959,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",
@@ -6231,6 +6282,9 @@ Suite *create_sysdb_suite(void)
tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+ /* Misc */
+ tcase_add_test(tc_sysdb, test_sysdb_set_get_bool);
+
/* Ghost users tests */
tcase_add_loop_test(tc_memberof, test_sysdb_memberof_store_group_with_ghosts,
MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
