11:36 a.m.
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.
1:32 p.m.
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
11:18 p.m.
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(a)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
2:42 a.m.
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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
3:11 a.m.
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(a)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
3:28 a.m.
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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
6:06 a.m.
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(a)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(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)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
6:49 a.m.
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(a)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(a)lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl(a)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(a)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!
1:08 a.m.
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(a)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(a)lists.fedorahosted.org
>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00 2001
>>From: Pavel Reichl <preichl(a)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
3178
days inactive
3180
days old
sssd-devel@lists.fedorahosted.org
8 comments
3 participants
participants (3)
-
Lukas Slebodnik -
Michal Židek -
Pavel Reichl