[PATCH 0/1] MR7335: wldap32: Build libldap with debugging facilities turned on, enable tracing with +libldap channel.
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/wldap32/bind.c | 5 ++++- dlls/wldap32/init.c | 6 ++++++ dlls/wldap32/main.c | 7 +++++++ libs/ldap/Makefile.in | 2 +- 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/dlls/wldap32/bind.c b/dlls/wldap32/bind.c index c297c234ad6..419dbb979ff 100644 --- a/dlls/wldap32/bind.c +++ b/dlls/wldap32/bind.c @@ -29,6 +29,7 @@ #include "winldap_private.h" WINE_DEFAULT_DEBUG_CHANNEL(wldap32); +WINE_DECLARE_DEBUG_CHANNEL(libldap); /*********************************************************************** * ldap_bindA (WLDAP32.@) @@ -200,6 +201,7 @@ ULONG CDECL ldap_bind_sW( LDAP *ld, WCHAR *dn, WCHAR *cred, ULONG method ) else if (method == WLDAP32_LDAP_AUTH_NEGOTIATE) { SEC_WINNT_AUTH_IDENTITY_W *id = (SEC_WINNT_AUTH_IDENTITY_W *)cred, idW; + unsigned int flags; if (id && (id->Flags & SEC_WINNT_AUTH_IDENTITY_ANSI)) { @@ -209,7 +211,8 @@ ULONG CDECL ldap_bind_sW( LDAP *ld, WCHAR *dn, WCHAR *cred, ULONG method ) id = &idW; } - ret = map_error( ldap_sasl_interactive_bind_s( CTX(ld), NULL, NULL, NULL, NULL, LDAP_SASL_QUIET, + flags = TRACE_ON( libldap ) ? 0 : LDAP_SASL_QUIET; + ret = map_error( ldap_sasl_interactive_bind_s( CTX(ld), NULL, NULL, NULL, NULL, flags, interact_callback, id ) ); if (id && (id->Flags & SEC_WINNT_AUTH_IDENTITY_ANSI)) { diff --git a/dlls/wldap32/init.c b/dlls/wldap32/init.c index 472bf2ffdbf..0e41e610458 100644 --- a/dlls/wldap32/init.c +++ b/dlls/wldap32/init.c @@ -34,6 +34,7 @@ #include "winldap_private.h" WINE_DEFAULT_DEBUG_CHANNEL(wldap32); +WINE_DECLARE_DEBUG_CHANNEL(libldap); /* Split a space separated string of hostnames into a string array */ static char **split_hostnames( const char *hostnames ) @@ -203,6 +204,11 @@ static LDAP *create_context( const char *url ) if (map_error( ldap_initialize( &CTX(ld), url ) ) == WLDAP32_LDAP_SUCCESS) { ldap_set_option( CTX(ld), WLDAP32_LDAP_OPT_PROTOCOL_VERSION, &version ); + if (TRACE_ON( libldap )) + { + int ld_debug = -1; + ldap_set_option( CTX(ld), LDAP_OPT_DEBUG_LEVEL, &ld_debug ); + } return ld; } free( ld ); diff --git a/dlls/wldap32/main.c b/dlls/wldap32/main.c index ac983372da7..0b79635b63d 100644 --- a/dlls/wldap32/main.c +++ b/dlls/wldap32/main.c @@ -24,10 +24,12 @@ #include "winbase.h" #include "wine/debug.h" +#include "winldap_private.h" HINSTANCE hwldap32; WINE_DEFAULT_DEBUG_CHANNEL(wldap32); +WINE_DECLARE_DEBUG_CHANNEL(libldap); BOOL WINAPI DllMain( HINSTANCE hinst, DWORD reason, LPVOID reserved ) { @@ -38,6 +40,11 @@ BOOL WINAPI DllMain( HINSTANCE hinst, DWORD reason, LPVOID reserved ) case DLL_PROCESS_ATTACH: hwldap32 = hinst; DisableThreadLibraryCalls( hinst ); + if (TRACE_ON( libldap )) + { + int ld_debug = -1; + ldap_set_option( NULL, LDAP_OPT_DEBUG_LEVEL, &ld_debug ); + } break; } return TRUE; diff --git a/libs/ldap/Makefile.in b/libs/ldap/Makefile.in index 5e5882584d0..8cfad386285 100644 --- a/libs/ldap/Makefile.in +++ b/libs/ldap/Makefile.in @@ -1,6 +1,6 @@ EXTLIB = libldap.a EXTRAINCL = -I$(srcdir)/include -EXTRADEFS = -D_TIMEVAL_DEFINED +EXTRADEFS = -D_TIMEVAL_DEFINED -DLDAP_DEBUG -DOLD_DEBUG SOURCES = \ liblber/bprint.c \ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7335
If we want to do debug builds of libldap I think it should be off by default. It has side effects like enabling asserts. Why do we need a separate debug channel? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94573
On Fri Feb 14 11:44:52 2025 +0000, Hans Leidekker wrote:
If we want to do debug builds of libldap I think it should be off by default. It has side effects like enabling asserts. Why do we need a separate debug channel? Adding libldap output to wldap32 channel makes +wldap32 very massive and hard to read. I didn't see any asserts during GSS-SPNEGO debugging, and having this patch in a local build for an year++ didn't reveal anything like that. Contrary, it helped a lot with debugging wldap32 + kerberos pair, so I find it very helpful feature.
How would you propose making debug build off by default, if that's a desirable way to proceed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94575
On Fri Feb 14 11:44:52 2025 +0000, Dmitry Timoshkov wrote:
Adding libldap output to wldap32 channel makes +wldap32 very massive and hard to read. I didn't see any asserts during GSS-SPNEGO debugging, and having this patch in a local build for an year++ didn't reveal anything like that. Contrary, it helped a lot with debugging wldap32 + kerberos pair, so I find it very helpful feature. How would you propose making debug build off by default, if that's a desirable way to proceed? Not sure how that should be done but I have used debugging builds of libdap and I don't think it's too much to ask the developer to enable it. It's also not common that we need to provide a user with a debug build so I'm not sure this is worth it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94587
On Fri Feb 14 12:19:23 2025 +0000, Hans Leidekker wrote:
Not sure how that should be done but I have used debugging builds of libdap and I don't think it's too much to ask the developer to enable it. It's also not common that we need to provide a user with a debug build so I'm not sure this is worth it. How about dropping the libldap part of the patch and just leave the wldap32 one?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94588
On Fri Feb 14 12:24:34 2025 +0000, Dmitry Timoshkov wrote:
How about dropping the libldap part of the patch and just leave the wldap32 one? I think that's fine but why do you set the option both globally and on the context? Is it necessary to change the flags to ldap_sasl_interactive_bind_s()? I still think it should use the existing debug channel.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94589
On Fri Feb 14 12:38:51 2025 +0000, Hans Leidekker wrote:
I think that's fine but why do you set the option both globally and on the context? Is it necessary to change the flags to ldap_sasl_interactive_bind_s()? I still think it should use the existing debug channel. I can leave setting the LDAP_OPT_DEBUG_LEVEL in a single place. Which one would you prefer better?
Iit's very helpful to make ldap_sasl_interactive_bind_s() not quite. Not a problem, I'll use existing debug channel. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94590
On Fri Feb 14 12:47:02 2025 +0000, Dmitry Timoshkov wrote:
I can leave setting the LDAP_OPT_DEBUG_LEVEL in a single place. Which one would you prefer better? Iit's very helpful to make ldap_sasl_interactive_bind_s() not quite. Not a problem, I'll use existing debug channel. I looked at libs/ldap source and turning on LDAP_DEBUG doesn't affect asserts at all, assert() statements verify the state in internal helpers.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94591
On Fri Feb 14 12:51:41 2025 +0000, Dmitry Timoshkov wrote:
I looked at libs/ldap source and turning on LDAP_DEBUG doesn't affect asserts at all, assert() statements verify the state in internal helpers. I'd say globally so you can get debug output even before a context is created. 0 == LDAP_SASL_AUTOMATIC which means it can prompt. So it changes behavior which the user may not expect.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94592
On Fri Feb 14 12:59:02 2025 +0000, Hans Leidekker wrote:
I'd say globally so you can get debug output even before a context is created. 0 == LDAP_SASL_AUTOMATIC which means it can prompt. So it changes behavior which the user may not expect. Wht do you mean by prompt? As far as I can see without LDAP_SASL_QUITE libldap calls sasl_getprop() which is part of our backend (sasl_w.c), and it never interactively propmps the user.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94593
On Fri Feb 14 13:14:09 2025 +0000, Dmitry Timoshkov wrote:
Wht do you mean by prompt? As far as I can see without LDAP_SASL_QUITE libldap calls sasl_getprop() which is part of our backend (sasl_w.c), and it never interactively propmps the user. From ldap.h:
/* Interaction flags (should be passed about in a control)
* Automatic (default): use defaults, prompt otherwise
* Interactive: prompt always
* Quiet: never prompt
*/
#define LDAP_SASL_AUTOMATIC»»···0U
#define LDAP_SASL_INTERACTIVE»··1U
#define LDAP_SASL_QUIET»»···»···2U
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94597
On Fri Feb 14 13:26:44 2025 +0000, Hans Leidekker wrote:
From ldap.h: ``` /* Interaction flags (should be passed about in a control) * Automatic (default): use defaults, prompt otherwise * Interactive: prompt always * Quiet: never prompt */ #define LDAP_SASL_AUTOMATIC»»···0U #define LDAP_SASL_INTERACTIVE»··1U #define LDAP_SASL_QUIET»»···»···2U ``` Grepping for LDAP_SASL_AUTOMATIC and LDAP_SASL_INTERACTIVE over the libs/ldap sources yields nothing. So the only meaningful flag is LDAP_SASL_QUITE which turns off calling the backend, and our backend never propmpts the user.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94598
Our backend is not complete. We shouldn't depend on the flag not being implemented. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7335#note_94599
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Hans Leidekker (＠hans)