Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
On 08/11/2015 06:36 PM, Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
ACK.
CI passed except for known problematic integration test. http://sssd-ci.duckdns.org/logs/job/21/32/rhel6/ci.html
Michal
On (11/08/15 18:36), Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb_ops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, } else if (type == SYSDB_MEMBER_GROUP) { member_dn = sysdb_group_dn(tmp_ctx, domain, member); } else {
DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); ret = EINVAL; goto done;}
if (!member_dn) {
DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); ret = ENOMEM;
Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
LS
On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
On (11/08/15 18:36), Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb_ops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, } else if (type == SYSDB_MEMBER_GROUP) { member_dn = sysdb_group_dn(tmp_ctx, domain, member); } else {
DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); ret = EINVAL; goto done;}
if (!member_dn) {
DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); ret = ENOMEM;Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
I added the freeing of tmp context, but I think that in case we are really out of memory this might not be enough. The more probable scenario to end up in this branch is IMO that sanitizing DN fails or some other failure.
I added debug message to check sysdb_dn_sanitize result to distinguish its failure.javascript:r(5)
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (12/08/15 09:42), Pavel Reichl wrote:
On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
On (11/08/15 18:36), Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb_ops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, } else if (type == SYSDB_MEMBER_GROUP) { member_dn = sysdb_group_dn(tmp_ctx, domain, member); } else {
DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); ret = EINVAL; goto done;}
if (!member_dn) {
DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); ret = ENOMEM;Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
I added the freeing of tmp context, but I think that in case we are really out of memory this might not be enough. The more probable scenario to end up in this branch is IMO that sanitizing DN fails or some other failure.
So it would be better to add debug message directly to sysdb_dn_sanitize. instead of after each invocation of sysdb_dn_sanitize. (sysdb_netgroup_dn) So you can distinguish there whether it's a memory allocation failure or ldb function failed.
LS
On 08/12/2015 10:11 AM, Lukas Slebodnik wrote:
On (12/08/15 09:42), Pavel Reichl wrote:
On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
On (11/08/15 18:36), Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb_ops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, } else if (type == SYSDB_MEMBER_GROUP) { member_dn = sysdb_group_dn(tmp_ctx, domain, member); } else {
DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); ret = EINVAL; goto done;}
if (!member_dn) {
DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); ret = ENOMEM;Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
I added the freeing of tmp context, but I think that in case we are really out of memory this might not be enough. The more probable scenario to end up in this branch is IMO that sanitizing DN fails or some other failure.
So it would be better to add debug message directly to sysdb_dn_sanitize. instead of after each invocation of sysdb_dn_sanitize. (sysdb_netgroup_dn)
OK, in logs you can lost one layer of calling hierarchy, but that's still better than nothing at all.
So you can distinguish there whether it's a memory allocation failure or ldb function failed.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (12/08/15 10:28), Pavel Reichl wrote:
On 08/12/2015 10:11 AM, Lukas Slebodnik wrote:
On (12/08/15 09:42), Pavel Reichl wrote:
On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
On (11/08/15 18:36), Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn2]. Skipping. [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member [user@dom] to group [somedn3]. Skipping.
It's hard to see what went wrong. Attached patch should add more information.
From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb_ops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, } else if (type == SYSDB_MEMBER_GROUP) { member_dn = sysdb_group_dn(tmp_ctx, domain, member); } else {
DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); ret = EINVAL; goto done;}
if (!member_dn) {
DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); ret = ENOMEM;Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
I added the freeing of tmp context, but I think that in case we are really out of memory this might not be enough. The more probable scenario to end up in this branch is IMO that sanitizing DN fails or some other failure.
So it would be better to add debug message directly to sysdb_dn_sanitize. instead of after each invocation of sysdb_dn_sanitize. (sysdb_netgroup_dn)
OK, in logs you can lost one layer of calling hierarchy, but that's still better than nothing at all.
So you can distinguish there whether it's a memory allocation failure or ldb function failed.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb.c | 9 +++++++++ src/db/sysdb_ops.c | 16 ++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 9da655759c0c35d52854b668693195b3360c5f8b..729d03593cc59e2c9f08e9f1fbe07234cc4a38ab 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -84,6 +84,9 @@ errno_t sysdb_dn_sanitize(TALLOC_CTX *mem_ctx, const char *input,
*sanitized = ldb_dn_escape_value(mem_ctx, val); if (!*sanitized) {
talloc_zfree(val.data);DEBUG(SSSDBG_MINOR_FAILURE, "ldb_dn_escape_value failed for: %s.\n",input); ret = ENOMEM;@@ -162,6 +165,9 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
ret = sysdb_dn_sanitize(NULL, name, &clean_name); if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"sysdb_dn_sanitize failed for: '%s' - [%d]: %s.\n",name, ret, sss_strerror(ret)); return NULL;@@ -188,6 +194,9 @@ struct ldb_dn *sysdb_group_dn(TALLOC_CTX *mem_ctx,
ret = sysdb_dn_sanitize(NULL, name, &clean_name); if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"sysdb_dn_sanitize failed for: %s - [%d]: %s.\n",name, ret, sss_strerror(ret)); return NULL;
I'm sorry but my previous was not probably clear. So my prefference was to add debug message to sysdb_dn_sanitize, which you did. The another option was to add debug messages after sysdb_dn_sanitize. So you did it just patrially and it was not necessary.
BTW I checked function ldb_dn_escape_value and it can fail only in case of insufficient memory. Your assumption needn't be correct. So debug messages might not help. They will just complicate situation with additional allocations in debug messages after real ENOMEM situation.
IMHO the better way would be to check argument "input" for NULL in functions sysdb_dn_sanitize (and also on other places. We had "Out of memory" false positive reports in past. They were caused by fact that we wanted do allocate copy of NULL (inssuficient validation of inputs).
LS
On 08/12/2015 01:06 PM, Lukas Slebodnik wrote:
On (12/08/15 10:28), Pavel Reichl wrote:
On 08/12/2015 10:11 AM, Lukas Slebodnik wrote:
On (12/08/15 09:42), Pavel Reichl wrote:
On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
On (11/08/15 18:36), Pavel Reichl wrote:
Hello,
I'm investigating log file with debug_level 9 that contains following lines
> [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member > [user@dom] to group [somedn]. Skipping. > [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member > [user@dom] to group [somedn2]. Skipping. > [sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member > [user@dom] to group [somedn3]. Skipping. It's hard to see what went wrong. Attached patch should add more information.
From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb_ops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, } else if (type == SYSDB_MEMBER_GROUP) { member_dn = sysdb_group_dn(tmp_ctx, domain, member); } else {
DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); ret = EINVAL; goto done;}
if (!member_dn) {
DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); ret = ENOMEM;Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
I added the freeing of tmp context, but I think that in case we are really out of memory this might not be enough. The more probable scenario to end up in this branch is IMO that sanitizing DN fails or some other failure.
So it would be better to add debug message directly to sysdb_dn_sanitize. instead of after each invocation of sysdb_dn_sanitize. (sysdb_netgroup_dn)
OK, in logs you can lost one layer of calling hierarchy, but that's still better than nothing at all.
So you can distinguish there whether it's a memory allocation failure or ldb function failed.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb.c | 9 +++++++++ src/db/sysdb_ops.c | 16 ++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 9da655759c0c35d52854b668693195b3360c5f8b..729d03593cc59e2c9f08e9f1fbe07234cc4a38ab 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -84,6 +84,9 @@ errno_t sysdb_dn_sanitize(TALLOC_CTX *mem_ctx, const char *input,
*sanitized = ldb_dn_escape_value(mem_ctx, val); if (!*sanitized) {
talloc_zfree(val.data);DEBUG(SSSDBG_MINOR_FAILURE, "ldb_dn_escape_value failed for: %s.\n",input); ret = ENOMEM;@@ -162,6 +165,9 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
ret = sysdb_dn_sanitize(NULL, name, &clean_name); if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"sysdb_dn_sanitize failed for: '%s' - [%d]: %s.\n",name, ret, sss_strerror(ret)); return NULL;@@ -188,6 +194,9 @@ struct ldb_dn *sysdb_group_dn(TALLOC_CTX *mem_ctx,
ret = sysdb_dn_sanitize(NULL, name, &clean_name); if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"sysdb_dn_sanitize failed for: %s - [%d]: %s.\n",name, ret, sss_strerror(ret)); return NULL;I'm sorry but my previous was not probably clear. So my prefference was to add debug message to sysdb_dn_sanitize, which you did. The another option was to add debug messages after sysdb_dn_sanitize. So you did it just patrially and it was not necessary.
BTW I checked function ldb_dn_escape_value and it can fail only in case of insufficient memory. Your assumption needn't be correct. So debug messages might not help. They will just complicate situation with additional allocations in debug messages after real ENOMEM situation.
IMHO the better way would be to check argument "input" for NULL in functions sysdb_dn_sanitize (and also on other places. We had "Out of memory" false positive reports in past. They were caused by fact that we wanted do allocate copy of NULL (inssuficient validation of inputs).
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Lukas, I need this messages to debug user case, messages are not perfect, and won't be. Please provide better patch or pick one from the changed versions you requested. I don't think we need to spent any more time nitpicking. Thanks!
On (12/08/15 13:49), Pavel Reichl wrote:
On 08/12/2015 01:06 PM, Lukas Slebodnik wrote:
On (12/08/15 10:28), Pavel Reichl wrote:
On 08/12/2015 10:11 AM, Lukas Slebodnik wrote:
On (12/08/15 09:42), Pavel Reichl wrote:
On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
On (11/08/15 18:36), Pavel Reichl wrote: >Hello, > >I'm investigating log file with debug_level 9 that contains following lines > >>[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member >>[user@dom] to group [somedn]. Skipping. >>[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member >>[user@dom] to group [somedn2]. Skipping. >>[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member >>[user@dom] to group [somedn3]. Skipping. >It's hard to see what went wrong. Attached patch should add more information. > >From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001 >From: Pavel Reichl preichl@redhat.com >Date: Tue, 11 Aug 2015 12:25:22 -0400 >Subject: [PATCH] SYSDB: add more debug msgs. to group membership code > >--- >src/db/sysdb_ops.c | 14 ++++++++++++-- >1 file changed, 12 insertions(+), 2 deletions(-) > >diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c >index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644 >--- a/src/db/sysdb_ops.c >+++ b/src/db/sysdb_ops.c >@@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain, > } else if (type == SYSDB_MEMBER_GROUP) { > member_dn = sysdb_group_dn(tmp_ctx, domain, member); > } else { >+ DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type); > ret = EINVAL; > goto done; > } > > if (!member_dn) { >+ DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n"); > ret = ENOMEM; Here is a small problem.
Allocation failed but debug_fn might allocate some memory especially with enabled journald. So debug messge needn't be printed. It would be good to release all unused resources (tmp_ctx if available) or do not print such messages at all.
I added the freeing of tmp context, but I think that in case we are really out of memory this might not be enough. The more probable scenario to end up in this branch is IMO that sanitizing DN fails or some other failure.
So it would be better to add debug message directly to sysdb_dn_sanitize. instead of after each invocation of sysdb_dn_sanitize. (sysdb_netgroup_dn)
OK, in logs you can lost one layer of calling hierarchy, but that's still better than nothing at all.
So you can distinguish there whether it's a memory allocation failure or ldb function failed.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Tue, 11 Aug 2015 12:25:22 -0400 Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
src/db/sysdb.c | 9 +++++++++ src/db/sysdb_ops.c | 16 ++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 9da655759c0c35d52854b668693195b3360c5f8b..729d03593cc59e2c9f08e9f1fbe07234cc4a38ab 100644 --- a/src/db/sysdb.c +++ b/src/db/sysdb.c @@ -84,6 +84,9 @@ errno_t sysdb_dn_sanitize(TALLOC_CTX *mem_ctx, const char *input,
*sanitized = ldb_dn_escape_value(mem_ctx, val); if (!*sanitized) {
talloc_zfree(val.data);DEBUG(SSSDBG_MINOR_FAILURE, "ldb_dn_escape_value failed for: %s.\n",input); ret = ENOMEM;@@ -162,6 +165,9 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
ret = sysdb_dn_sanitize(NULL, name, &clean_name); if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"sysdb_dn_sanitize failed for: '%s' - [%d]: %s.\n",name, ret, sss_strerror(ret)); return NULL;@@ -188,6 +194,9 @@ struct ldb_dn *sysdb_group_dn(TALLOC_CTX *mem_ctx,
ret = sysdb_dn_sanitize(NULL, name, &clean_name); if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,"sysdb_dn_sanitize failed for: %s - [%d]: %s.\n",name, ret, sss_strerror(ret)); return NULL;I'm sorry but my previous was not probably clear. So my prefference was to add debug message to sysdb_dn_sanitize, which you did. The another option was to add debug messages after sysdb_dn_sanitize. So you did it just patrially and it was not necessary.
BTW I checked function ldb_dn_escape_value and it can fail only in case of insufficient memory. Your assumption needn't be correct. So debug messages might not help. They will just complicate situation with additional allocations in debug messages after real ENOMEM situation.
IMHO the better way would be to check argument "input" for NULL in functions sysdb_dn_sanitize (and also on other places. We had "Out of memory" false positive reports in past. They were caused by fact that we wanted do allocate copy of NULL (inssuficient validation of inputs).
LS
Lukas, I need this messages to debug user case,
So it means that they are not proved it will help to find issue. Does it make sense to push them to master? I often provide scratch build for users with many temporary debug messages. After I find out which debug message help then I will send reduced patch with useful debug messages to upstream.
messages are not perfect, and won't be.
I have never claimed writig debug messages are simple task. I have few patches on the list with debbug messages which shoudl be changed (request from reviewers)
Please provide better patch or pick one from the changed versions you requested.
I already mentioned in 1st review that logging ENOMEM messages does not make a sense without releasing some memory. Releasing temporary context might help because talloc header is 96 bytes long (IIRC)
I thought that ldb_dn_escape_value can fail in some cases. But nether you nor me checked documetation or source code on first time. I'm sorry for a late notice. But it means that the latest version would print error message after ENOMEM without releasing memory. 1. after ldb_dn_escape_value 2. after sysdb_user_dn and sysdb_group_dn
It seems that you do not want to change debug messages and neither of patches is suitable for merging to upstream. (I don't want to say it might not help reveal issue for user) So I will mark patch as rejected in patchwork.
I don't think we need to spent any more time nitpicking.
It's not about nitpicking. I also provided and idea which could be reason of issues. The ENOMEM is not very likely situation but possible. It's super simple to limit resources with systemd drop-in files. @see man systemd-exec
LS
sssd-devel@lists.fedorahosted.org
-
Lukas Slebodnik -
Michal Židek -
Pavel Reichl