[PATCH] LDAP: Do not print verbose DEBUG messages from providers that don't set UUID - sssd-devel - Fedora Mailing-Lists

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH] LDAP: Do not print verbose DEBUG messages from providers that don't set UUID

Re: [SSSD] [INI] Patches for the...

Re: [SSSD] [PATCH] dyndns: don't...

Jakub Hrozek

Wednesday, 3 June 2015 Wed, 3 Jun '15

10:58 a.m.

I think ENOENT in this case is better because the caller might (and does) special-case the value to handle "no UUID" case separately.

Attachments:

0001-LDAP-Do-not-print-verbose-DEBUG-messages-from-provid.patch (text/plain — 1.0 KB)

Reply

Show replies by date

Lukas Slebodnik

Wednesday, 3 June Wed, 3 Jun

11:13 a.m.

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(a)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) LS

Reply

Jakub Hrozek

11:21 a.m.

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(a)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.

Reply

Lukas Slebodnik

11:29 a.m.

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(a)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(a)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. LS

Reply

Sumit Bose

Thursday, 4 June Thu, 4 Jun

2:07 a.m.

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

Reply

Lukas Slebodnik

2:30 a.m.

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(a)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(a)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(-)

The patch works, we can discuss about other improvements, but this patch can be pushed. ACK http://sssd-ci.duckdns.org/logs/job/16/71/summary.html LS

Reply

Jakub Hrozek

7:07 a.m.

On Thu, Jun 04, 2015 at 09:30:34AM +0200, Lukas Slebodnik wrote:

ACK http://sssd-ci.duckdns.org/logs/job/16/71/summary.html LS

* master: e7e61c777e13e0bb07d29b5d1b53e21ca199bf0f

Reply

3260

days inactive

3261

days old

sssd-devel@lists.fedorahosted.org

Manage subscription

6 comments

3 participants

tags (0)

participants (3)

Jakub Hrozek
Lukas Slebodnik
Sumit Bose