This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch: - the function did not fail, but non-zero value remains in errno from previous program run - the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not sure whether it is possible)
Other than that the patch looks fine.
Jan
On Wed, Aug 10, 2011 at 10:14:14AM +0200, Jan Zelený wrote:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch:
- the function did not fail, but non-zero value remains in errno from previous
program run
- the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not sure
whether it is possible)
Other than that the patch looks fine.
Jan
Thanks, a new patch is attached.
On Wed, Aug 10, 2011 at 10:14:14AM +0200, Jan Zelený wrote:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch:
- the function did not fail, but non-zero value remains in errno from
previous program run
- the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not
sure whether it is possible)
Other than that the patch looks fine.
Jan
Thanks, a new patch is attached.
Looks ok now, Ack
Jan
On Wed, 2011-08-10 at 15:29 +0200, Jan Zelený wrote:
On Wed, Aug 10, 2011 at 10:14:14AM +0200, Jan Zelený wrote:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch:
- the function did not fail, but non-zero value remains in errno from
previous program run
- the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not
sure whether it is possible)
Other than that the patch looks fine.
Jan
Thanks, a new patch is attached.
Looks ok now, Ack
It looks like this patch, although acked was never pushed upstream ? Has it been dropped offline ?
Simo.
On Tue, Nov 27, 2012 at 11:27:03PM -0500, Simo Sorce wrote:
On Wed, 2011-08-10 at 15:29 +0200, Jan Zelený wrote:
On Wed, Aug 10, 2011 at 10:14:14AM +0200, Jan Zelený wrote:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch:
- the function did not fail, but non-zero value remains in errno from
previous program run
- the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not
sure whether it is possible)
Other than that the patch looks fine.
Jan
Thanks, a new patch is attached.
Looks ok now, Ack
It looks like this patch, although acked was never pushed upstream ? Has it been dropped offline ?
Simo.
-- Simo Sorce * Red Hat, Inc * New York
See this sentence in my original mail:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
So no, it was never pushed to master. It provides a *lot* of debugging info and may easily flood the logs. We may want to reconsider the submission, but I would do so only if we dedicate one log level to low level tracing such as ldb transactions or libldb error output.
On Wed 28 Nov 2012 02:46:43 AM EST, Jakub Hrozek wrote:
On Tue, Nov 27, 2012 at 11:27:03PM -0500, Simo Sorce wrote:
On Wed, 2011-08-10 at 15:29 +0200, Jan Zelený wrote:
On Wed, Aug 10, 2011 at 10:14:14AM +0200, Jan Zelený wrote:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
For example: export SSSD_DEBUG_LDAP_SEARCH="0xffff" would set LDAP_DEBUG_ANY
The attached patch applies cleanly on the RHEL6.1 branch. I also have a version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch:
- the function did not fail, but non-zero value remains in errno from
previous program run
- the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not
sure whether it is possible)
Other than that the patch looks fine.
Jan
Thanks, a new patch is attached.
Looks ok now, Ack
It looks like this patch, although acked was never pushed upstream ? Has it been dropped offline ?
Simo.
-- Simo Sorce * Red Hat, Inc * New York
See this sentence in my original mail:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
So no, it was never pushed to master. It provides a *lot* of debugging info and may easily flood the logs. We may want to reconsider the submission, but I would do so only if we dedicate one log level to low level tracing such as ldb transactions or libldb error output.
That is supposed to be the point of SSSDBG_TRACE_ALL. I realize we're not always consistent on that, though.
On Wed, Nov 28, 2012 at 07:40:32AM -0500, Stephen Gallagher wrote:
On Wed 28 Nov 2012 02:46:43 AM EST, Jakub Hrozek wrote:
On Tue, Nov 27, 2012 at 11:27:03PM -0500, Simo Sorce wrote:
On Wed, 2011-08-10 at 15:29 +0200, Jan Zelený wrote:
On Wed, Aug 10, 2011 at 10:14:14AM +0200, Jan Zelený wrote:
>This patch should not be pushed to master, but I would like to get it >reviewed anyway. > >It should be used to provide a custom build for users experiencing >cases where ldap_search_ext would block (c.f. >https://bugzilla.redhat.com/show_bug.cgi?id=728343) > >For example: >export SSSD_DEBUG_LDAP_SEARCH="0xffff" >would set LDAP_DEBUG_ANY > >The attached patch applies cleanly on the RHEL6.1 branch. I also have a >version that applies on master/1.5 if needed.
I have three things: On line 98 you close the original stderr - this is not a problem, but it is redundant
On line 162 you are using strtol and testing that it did not fail. Please consider two possibilities that you haven't considered in your patch:
- the function did not fail, but non-zero value remains in errno from
previous program run
- the env. variable SSSD_DEBUG_LDAP_SEARCH contains empty string (I'm not
sure whether it is possible)
Other than that the patch looks fine.
Jan
Thanks, a new patch is attached.
Looks ok now, Ack
It looks like this patch, although acked was never pushed upstream ? Has it been dropped offline ?
Simo.
-- Simo Sorce * Red Hat, Inc * New York
See this sentence in my original mail:
This patch should not be pushed to master, but I would like to get it reviewed anyway.
It should be used to provide a custom build for users experiencing cases where ldap_search_ext would block (c.f. https://bugzilla.redhat.com/show_bug.cgi?id=728343)
So no, it was never pushed to master. It provides a *lot* of debugging info and may easily flood the logs. We may want to reconsider the submission, but I would do so only if we dedicate one log level to low level tracing such as ldb transactions or libldb error output.
That is supposed to be the point of SSSDBG_TRACE_ALL. I realize we're not always consistent on that, though.
OK, attached is the original acked patch, just rebased on master.
Right now, the debug is controlled by a special environment variable. If we were to use the patch upstream, I would propose just turning the libldap debug based on an the debug_level option value.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Simo Sorce -
Stephen Gallagher