[SSSD] [PATCHES] LDAP: fix ldap_setup_enumeration() handling ENOENT
Pavel Reichl
preichl at redhat.com
Fri Nov 28 15:09:12 UTC 2014
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.
If we stick with my version I should update unit test - to test for both
possible ENOENT events.
>
> >From 325c84d5f9d685ab9793b357dc4070d3b926ced2 Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Tue, 4 Nov 2014 08:55:55 +0000
>> Subject: [PATCH 2/2] LDAP: fix ldap_setup_enumeration() handling ENOENT
>>
>> This patch fixes ldap_setup_enumeration() to handle ENOENT returned by
>> sysdb_has_enumerated().
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/1991
>> ---
>> src/providers/ldap/ldap_id_enum.c | 6 +++++-
>> src/tests/sysdb-tests.c | 7 +++----
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> 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 7f1ca8755d7066788cb4706b923c5282417b59aa..c8be7f968160372c4ca23fa68671455f018d2a3f 100644
>> --- a/src/tests/sysdb-tests.c
>> +++ b/src/tests/sysdb-tests.c
>> @@ -4943,10 +4943,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",
> This test should be probably part of the 1st patch
> becuase it fix failed test.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list