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