[SSSD] [PATCH] LDAP: Do not print verbose DEBUG messages from providers that don't set UUID
Sumit Bose
sbose at redhat.com
Thu Jun 4 07:07:28 UTC 2015
On Wed, Jun 03, 2015 at 06:29:01PM +0200, Lukas Slebodnik wrote:
> On (03/06/15 18:21), Jakub Hrozek wrote:
> >On Wed, Jun 03, 2015 at 06:13:04PM +0200, Lukas Slebodnik wrote:
> >> On (03/06/15 17:58), Jakub Hrozek wrote:
> >> >I think ENOENT in this case is better because the caller might (and
> >> >does) special-case the value to handle "no UUID" case separately.
> >>
> >> >From 762d47583e799bb956564ead9418a64a8dd494e1 Mon Sep 17 00:00:00 2001
> >> >From: Jakub Hrozek <jhrozek at redhat.com>
> >> >Date: Wed, 3 Jun 2015 17:57:29 +0200
> >> >Subject: [PATCH] LDAP: Do not print verbose DEBUG messages from providers that
> >> ^^^^
> >> changes are in sydb and not in LDAP
> >> > don't set UUID
> >> >
> >> >https://fedorahosted.org/sssd/ticket/2666
> >> >---
> >> > src/db/sysdb_ops.c | 7 ++++++-
> >> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> >> >index 9afb0d7d74cdbc91570f4fe6485790d2a4eeb4be..d34583787f1fce9bd07e272ae7549b8bd4f0b7dd 100644
> >> >--- a/src/db/sysdb_ops.c
> >> >+++ b/src/db/sysdb_ops.c
> >> >@@ -3795,7 +3795,12 @@ errno_t sysdb_handle_original_uuid(const char *orig_name,
> >> > struct ldb_message_element *el;
> >> > char guid_str_buf[GUID_STR_BUF_SIZE];
> >> >
> >> >- if (orig_name == NULL || src_attrs == NULL || src_name == NULL
> >> >+ if (orig_name == NULL) {
> >> >+ /* This provider doesn't handle UUIDs */
> >> >+ return ENOENT;
> >> >+ }
> >> >+
> >> >+ if (src_attrs == NULL || src_name == NULL
> >> > || dest_attrs == NULL || dest_name == NULL) {
> >> > return EINVAL;
> >> > }
> >>
> >> sh$ ./test_sysdb_utils
> >> [==========] Running 1 test(s).
> >> [ RUN ] test_sysdb_handle_original_uuid
> >> 0x2 != 0x16
> >> ../sssd/src/tests/cmocka/test_sysdb_utils.c:48: error: Failure!
> >>
> >> [ FAILED ] test_sysdb_handle_original_uuid
> >> [==========] 1 test(s) run.
> >> [ PASSED ] 0 test(s).
> >> [ FAILED ] 1 test(s), listed below:
> >> [ FAILED ] test_sysdb_handle_original_uuid
> >>
> >> 1 FAILED TEST(S)
> >>
> >
> >Let's try it again.
>
> >From 9c69b6f3069ab5728de8f0956a4f7d1cc0afba3b Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek at redhat.com>
> >Date: Wed, 3 Jun 2015 17:57:29 +0200
> >Subject: [PATCH] LDAP: Do not print verbose DEBUG messages from providers that
> > don't set UUID
> >
> >https://fedorahosted.org/sssd/ticket/2666
> >---
> > src/db/sysdb_ops.c | 7 ++++++-
> > src/tests/cmocka/test_sysdb_utils.c | 10 +++++++++-
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> >
> >diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> >index 9afb0d7d74cdbc91570f4fe6485790d2a4eeb4be..d34583787f1fce9bd07e272ae7549b8bd4f0b7dd 100644
> >--- a/src/db/sysdb_ops.c
> >+++ b/src/db/sysdb_ops.c
> >@@ -3795,7 +3795,12 @@ errno_t sysdb_handle_original_uuid(const char *orig_name,
> > struct ldb_message_element *el;
> > char guid_str_buf[GUID_STR_BUF_SIZE];
> >
> >- if (orig_name == NULL || src_attrs == NULL || src_name == NULL
> >+ if (orig_name == NULL) {
> >+ /* This provider doesn't handle UUIDs */
> >+ return ENOENT;
> >+ }
> >+
> >+ if (src_attrs == NULL || src_name == NULL
> > || dest_attrs == NULL || dest_name == NULL) {
> > return EINVAL;
> > }
> >diff --git a/src/tests/cmocka/test_sysdb_utils.c b/src/tests/cmocka/test_sysdb_utils.c
> >index 1e9baa88cd82e7631a1de3db7f567f1f050ca67b..b791f14b7fdfdd4e55343fc1c03a7fd55d8ed101 100644
> >--- a/src/tests/cmocka/test_sysdb_utils.c
> >+++ b/src/tests/cmocka/test_sysdb_utils.c
> >@@ -45,7 +45,7 @@ static void test_sysdb_handle_original_uuid(void **state)
> > struct ldb_val guid_val = {bin_guid, 16};
> >
> > ret = sysdb_handle_original_uuid(NULL, NULL, NULL, NULL, NULL);
> >- assert_int_equal(ret, EINVAL);
> >+ assert_int_equal(ret, ENOENT);
> >
> > src_attrs = sysdb_new_attrs(NULL);
> > assert_non_null(src_attrs);
> >@@ -63,6 +63,14 @@ static void test_sysdb_handle_original_uuid(void **state)
> > ret = sysdb_attrs_add_string(src_attrs, "UUID", IPA_UUID);
> > assert_int_equal(ret, EOK);
> >
> >+ ret = sysdb_handle_original_uuid(NULL, src_attrs, "GUID",
> >+ dest_attrs, "def");
> >+ assert_int_equal(ret, ENOENT);
> >+
> >+ ret = sysdb_handle_original_uuid("objectGUID", NULL, "GUID",
> >+ dest_attrs, "def");
> >+ assert_int_equal(ret, EINVAL);
> >+
> > ret = sysdb_handle_original_uuid("objectGUID", src_attrs, "GUID",
> > dest_attrs, "def");
> > assert_int_equal(ret, EOK);
>
> Patch works as expected.
>
> I have a slightly related question (maybe for Sumit, because he wrote patch)
> Why do we print message if ENOENT is returned?
>
> ret = sysdb_handle_original_uuid(
> opts->group_map[SDAP_AT_GROUP_UUID].def_name,
> attrs,
> opts->group_map[SDAP_AT_GROUP_UUID].sys_name,
> group_attrs, SYSDB_UUID);
> if (ret != EOK) {
> DEBUG((ret == ENOENT) ? SSSDBG_TRACE_ALL : SSSDBG_MINOR_FAILURE,
> "Failed to retrieve UUID [%d][%s].\n", ret, sss_strerror(ret));
> }
>
>
> Would it be better to have next version?
> if (ret != EOK && ret != ENOENT) {
> DEBUG(SSSDBG_MINOR_FAILURE,
> "Failed to retrieve UUID [%d][%s].\n", ret, sss_strerror(ret));
> }
>
> because if the provider does not handle UUIDs
> then we needn't print message at all.
The intention was mainly to help debugging issues where e.g. the LDAP
provider was used against AD (for whatever reasons) and a wrong
attribute name for the UUID was used. But since afaik the UUID is not
used in the context at all, i.e. it is only saved to the cache, I don't
mind if you prefer to change it.
bye,
Sumit
>
> 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