[SSSD] [PATCHES] LDAP: fix ldap_setup_enumeration() handling ENOENT

Pavel Reichl preichl at redhat.com
Mon Dec 1 12:49:51 UTC 2014


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SYSDB-sysdb_get_bool-return-ENOENT-unit-tests.patch
Type: text/x-patch
Size: 5500 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141201/b3b4d608/attachment.bin>


More information about the sssd-devel mailing list