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.
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel