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(a)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(a)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