[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