[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