I think ENOENT in this case is better because the caller might (and does) special-case the value to handle "no UUID" case separately.
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@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
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@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 LDAPdon'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.
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@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 LDAPdon'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@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
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@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 LDAPdon'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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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 LDAPdon'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@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
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
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Sumit Bose