We don't know the group name at that point yet, so better not print "null" in the debug message..
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
LS
On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
No, because we need to know the object domain first to format the name properly and in order to find the correct domain, we need to know the SID. See 970c5afb Maybe we should add a comment to that function, too, so that someone doesn't try to 'optimize' it in future..
On 02/25/2016 11:06 AM, Jakub Hrozek wrote:
On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
No, because we need to know the object domain first to format the name properly and in order to find the correct domain, we need to know the SID. See 970c5afb Maybe we should add a comment to that function, too, so that someone doesn't try to 'optimize' it in future..
We could probably use SYSDB_NAME here, couldn't we? But given it's TRACE_ALL and we have lived without the information so long, it's good enough to let it be as is in my opinion.
Ack.
On (25/02/16 11:06), Jakub Hrozek wrote:
On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
No, because we need to know the object domain first to format the name properly and in order to find the correct domain, we need to know the SID. See 970c5afb Maybe we should add a comment to that function, too, so that someone doesn't try to 'optimize' it in future..
Ahh, I missed that we might need different domain for sdap_get_group_primary_name.
But there is a question. Do we really need this trace debug message? IMHO it's confusing debug message with ldap provider.
LS
On Tue, Mar 08, 2016 at 09:34:29AM +0100, Lukas Slebodnik wrote:
On (25/02/16 11:06), Jakub Hrozek wrote:
On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
No, because we need to know the object domain first to format the name properly and in order to find the correct domain, we need to know the SID. See 970c5afb Maybe we should add a comment to that function, too, so that someone doesn't try to 'optimize' it in future..
Ahh, I missed that we might need different domain for sdap_get_group_primary_name.
But there is a question. Do we really need this trace debug message? IMHO it's confusing debug message with ldap provider.
Dunno, we can remove it as well. I'm fine either way, but printing NULL is potentionally dangerous.
On (08/03/16 18:14), Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 09:34:29AM +0100, Lukas Slebodnik wrote:
On (25/02/16 11:06), Jakub Hrozek wrote:
On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
No, because we need to know the object domain first to format the name properly and in order to find the correct domain, we need to know the SID. See 970c5afb Maybe we should add a comment to that function, too, so that someone doesn't try to 'optimize' it in future..
Ahh, I missed that we might need different domain for sdap_get_group_primary_name.
But there is a question. Do we really need this trace debug message? IMHO it's confusing debug message with ldap provider.
Dunno, we can remove it as well. I'm fine either way, but printing NULL is potentionally dangerous.
Then I vote for removing it. I cannot see a benfit of just logging message "objectSID: not available for group". It does not say which group and therefore it's confusing.
LS
On 03/08/2016 10:57 PM, Lukas Slebodnik wrote:
On (08/03/16 18:14), Jakub Hrozek wrote:
On Tue, Mar 08, 2016 at 09:34:29AM +0100, Lukas Slebodnik wrote:
On (25/02/16 11:06), Jakub Hrozek wrote:
On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
On (24/02/16 16:43), Jakub Hrozek wrote:
We don't know the group name at that point yet, so better not print "null" in the debug message..
From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 26 Jan 2016 16:29:08 +0100 Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
src/providers/ldap/sdap_async_groups.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, goto done; } } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n"); sid_str = NULL;
Could we move " sdap_get_group_primary_name(..., &group_name)" before "sdap_attrs_get_sid_str"?
No, because we need to know the object domain first to format the name properly and in order to find the correct domain, we need to know the SID. See 970c5afb Maybe we should add a comment to that function, too, so that someone doesn't try to 'optimize' it in future..
Ahh, I missed that we might need different domain for sdap_get_group_primary_name.
But there is a question. Do we really need this trace debug message? IMHO it's confusing debug message with ldap provider.
Dunno, we can remove it as well. I'm fine either way, but printing NULL is potentionally dangerous.
Then I vote for removing it. I cannot see a benfit of just logging message "objectSID: not available for group". It does not say which group and therefore it's confusing.
I'm fine either way.
On (09/03/16 18:47), Jakub Hrozek wrote:
On Wed, Mar 09, 2016 at 10:06:38AM +0100, Pavel Březina wrote:
I'm fine either way.
OK, see the attached patch.
From ea49df6b0816845ec667392a170f4176e759731f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 9 Mar 2016 18:44:43 +0100 Subject: [PATCH] LDAP: Remove misleading debug message
The ENOENT message is already handled in sdap_attrs_get_sid_str() and the debug message in the LDAP provider relied on name which was not known at the time.
src/providers/ldap/sdap_async_groups.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 3ccf7deb53d365a106cd18af1835f527424d5bf3..68acd32aec3681eae7ef703050efff0c874112e8 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -537,10 +537,6 @@ static int sdap_save_group(TALLOC_CTX *memctx, sss_strerror(ret)); goto done; }
- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n", sss_strerror(ret));sid_str = NULL;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ After this patch we will see SSSDBG_MINOR_FAILURE. Which is worse than "objectSID: not available for group" without any name.
LS
On Thu, Mar 10, 2016 at 10:15:01AM +0100, Lukas Slebodnik wrote:
On (09/03/16 18:47), Jakub Hrozek wrote:
On Wed, Mar 09, 2016 at 10:06:38AM +0100, Pavel Březina wrote:
I'm fine either way.
OK, see the attached patch.
From ea49df6b0816845ec667392a170f4176e759731f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 9 Mar 2016 18:44:43 +0100 Subject: [PATCH] LDAP: Remove misleading debug message
The ENOENT message is already handled in sdap_attrs_get_sid_str() and the debug message in the LDAP provider relied on name which was not known at the time.
src/providers/ldap/sdap_async_groups.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 3ccf7deb53d365a106cd18af1835f527424d5bf3..68acd32aec3681eae7ef703050efff0c874112e8 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -537,10 +537,6 @@ static int sdap_save_group(TALLOC_CTX *memctx, sss_strerror(ret)); goto done; }
- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n", sss_strerror(ret));sid_str = NULL;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
After this patch we will see SSSDBG_MINOR_FAILURE. Which is worse than "objectSID: not available for group" without any name.
I think I need to start testing my patches. Let's try it again..
On (10/03/16 11:41), Jakub Hrozek wrote:
On Thu, Mar 10, 2016 at 10:15:01AM +0100, Lukas Slebodnik wrote:
On (09/03/16 18:47), Jakub Hrozek wrote:
On Wed, Mar 09, 2016 at 10:06:38AM +0100, Pavel Březina wrote:
I'm fine either way.
OK, see the attached patch.
From ea49df6b0816845ec667392a170f4176e759731f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 9 Mar 2016 18:44:43 +0100 Subject: [PATCH] LDAP: Remove misleading debug message
The ENOENT message is already handled in sdap_attrs_get_sid_str() and the debug message in the LDAP provider relied on name which was not known at the time.
src/providers/ldap/sdap_async_groups.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 3ccf7deb53d365a106cd18af1835f527424d5bf3..68acd32aec3681eae7ef703050efff0c874112e8 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -537,10 +537,6 @@ static int sdap_save_group(TALLOC_CTX *memctx, sss_strerror(ret)); goto done; }
- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
} else { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n", sss_strerror(ret));sid_str = NULL;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
After this patch we will see SSSDBG_MINOR_FAILURE. Which is worse than "objectSID: not available for group" without any name.
I think I need to start testing my patches. Let's try it again..
Have you tested the latest patch it?
From e3abad3d7eef06f115cf89760ba0e2146fd6c218 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Wed, 9 Mar 2016 18:44:43 +0100 Subject: [PATCH] LDAP: Remove misleading debug message
The ENOENT message is already handled in sdap_attrs_get_sid_str() and the debug message in the LDAP provider relied on name which was not known at the time.
src/providers/ldap/sdap_async_groups.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 3ccf7deb53d365a106cd18af1835f527424d5bf3..82a15d144de67232212d83c7b5882b63ba710f69 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -537,11 +537,7 @@ static int sdap_save_group(TALLOC_CTX *memctx, sss_strerror(ret)); goto done; }
- } else if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group [%s].\n",
group_name);
sid_str = NULL;
- } else {
- } else if (ret != ENOENT) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n", sss_strerror(ret)); sid_str = NULL;
"sid_str" might be used uninitialized on line 559
557 /* If this object has a SID available, we will determine the correct 558 * domain by its SID. */ 559 if (sid_str != NULL) { 560 subdomain = sss_get_domain_by_sid_ldap_fallback(get_domains_head(dom), 561 sid_str); 562 if (subdomain) { 563 dom = subdomain; 564 } else { 565 DEBUG(SSSDBG_TRACE_FUNC, "SID %s does not belong to any known " 566 "domain\n", sid_str); 567 } 568 }
LS
sssd-devel@lists.fedorahosted.org