[PATCH] Two patches with more debugging for the LDAP provider

[Jakub Hrozek]

Hi,

I was trying to help a user who had problems with the AD provider and
realized I had hard time following what exact server are we searching at
the time and, because we hit a referral, where the referral pointed to.

I wrote the two attached patches. I'm OK with keeping them in my local
tree, but if other developers agree, maybe we can include them upstream
as well.

[Lukas Slebodnik]

On (18/07/14 15:52), Jakub Hrozek wrote:
> Hi,
>
> I was trying to help a user who had problems with the AD provider and
> realized I had hard time following what exact server are we searching at
> the time and, because we hit a referral, where the referral pointed to.
>
> I wrote the two attached patches. I'm OK with keeping them in my local
> tree, but if other developers agree, maybe we can include them upstream
> as well.
> From 10edfd72516e7c1ac24b3ebe148825d12325e04e Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Fri, 18 Jul 2014 15:04:28 +0200
> Subject: [PATCH 1/2] LDAP: Print referrals for debugging purposes
>
> src/providers/ldap/sdap_async.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 590cbe100bdf5f3d6df8d9d7b356fd1d560cd729..50e60ff846d9f94bb9f7343246429f1345286719 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -1335,6 +1335,8 @@ static void sdap_get_generic_ext_done(struct sdap_op *op,
>     struct sdap_get_generic_ext_state *state = tevent_req_data(req,
>                                             struct sdap_get_generic_ext_state);
>     char *errmsg = NULL;
> int i;
> char **refs = NULL;
> int result;
> int ret;
> int lret;
>
> @@ -1370,7 +1372,7 @@ static void sdap_get_generic_ext_done(struct sdap_op *op,
> case LDAP_RES_SEARCH_RESULT:
>     ret = ldap_parse_result(state->sh->ldap, reply->msg,
>                            &result, NULL, &errmsg, NULL,
>                            &result, NULL, &errmsg, &refs,
>                           &returned_controls, 0);
>   if (ret != LDAP_SUCCESS) {
>       DEBUG(SSSDBG_OP_FAILURE,
>
> @@ -1383,6 +1385,13 @@ static void sdap_get_generic_ext_done(struct sdap_op *op,
>                   sss_ldap_err2string(result), result,
>                   errmsg ? errmsg : "no errmsg set");
>    if (refs) {

I thought you prefer refs != NULL
http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
>        for (i = 0; refs[i]; i++) {
>            DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", refs[i]);
>        }
>        ldap_memvfree((void **) refs);
>    }
>   if (result == LDAP_SIZELIMIT_EXCEEDED) {
>       /* Try to return what we've got */
>       DEBUG(SSSDBG_MINOR_FAILURE,
>
> -- 
> 1.9.3
> From 8b1ad7a4797b941d8d35700c7a5558f3362ba758 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Fri, 18 Jul 2014 15:37:34 +0200
> Subject: [PATCH 2/2] LDAP: Dump LDAP server IP address with a high DEBUG level
>
> src/providers/ldap/sdap_async.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 50e60ff846d9f94bb9f7343246429f1345286719..4e91c02b7ba82ad9d1d8500f84906f0e40f74399 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -1083,6 +1083,41 @@ static errno_t add_to_deref_reply(TALLOC_CTX *mem_ctx,
>     return EOK;
> }
>
> +void sdap_print_server(struct sdap_handle *sh)

It would be better to have this function static.
> +{
> int ret;
> int fd;
> struct sockaddr_storage ss;
> socklen_t ss_len = sizeof(ss);
> char ip[NI_MAXHOST];
> if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {

and one more time
http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
I couldn't resist :-) :-) :-)

Otherwise, LGTM.
You asked for opinion of other developers, therefore I let final ACK for
someone else.  I am fine with pushing current version.

LS

[Jakub Hrozek]

On Fri, Jul 18, 2014 at 04:14:58PM +0200, Lukas Slebodnik wrote:
> On (18/07/14 15:52), Jakub Hrozek wrote:
> > Hi,
> >
> > I was trying to help a user who had problems with the AD provider and
> > realized I had hard time following what exact server are we searching at
> > the time and, because we hit a referral, where the referral pointed to.
> >
> > I wrote the two attached patches. I'm OK with keeping them in my local
> > tree, but if other developers agree, maybe we can include them upstream
> > as well.
> > From 10edfd72516e7c1ac24b3ebe148825d12325e04e Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Fri, 18 Jul 2014 15:04:28 +0200
> > Subject: [PATCH 1/2] LDAP: Print referrals for debugging purposes

[...]
> > @@ -1383,6 +1385,13 @@ static void sdap_get_generic_ext_done(struct sdap_op *op,
> >                   sss_ldap_err2string(result), result,
> >                   errmsg ? errmsg : "no errmsg set");
> >    if (refs) {
>
> I thought you prefer refs != NULL
> http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/

Fixed, I admit I copied this code from ldapsearch.c
> >        for (i = 0; refs[i]; i++) {
> >            DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", refs[i]);
> >        }
> >        ldap_memvfree((void **) refs);
> >    }
> > From 8b1ad7a4797b941d8d35700c7a5558f3362ba758 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Fri, 18 Jul 2014 15:37:34 +0200
> > Subject: [PATCH 2/2] LDAP: Dump LDAP server IP address with a high DEBUG level
> >
> > src/providers/ldap/sdap_async.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> > index 50e60ff846d9f94bb9f7343246429f1345286719..4e91c02b7ba82ad9d1d8500f84906f0e40f74399 100644
> > --- a/src/providers/ldap/sdap_async.c
> > +++ b/src/providers/ldap/sdap_async.c
> > @@ -1083,6 +1083,41 @@ static errno_t add_to_deref_reply(TALLOC_CTX *mem_ctx,
> >     return EOK;
> > }
> >
> > +void sdap_print_server(struct sdap_handle *sh)
>
> It would be better to have this function static.

Done.
> > +{
> > int ret;
> > int fd;
> > struct sockaddr_storage ss;
> > socklen_t ss_len = sizeof(ss);
> > char ip[NI_MAXHOST];
> > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
>
> and one more time
> http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
> I couldn't resist :-) :-) :-)

I don't agree with that blog post completely, I think using "!" if you
really mean boolean false is OK. I dislike "if (!ptr)" as a way of
saying "if (ptr == NULL)".

[Pavel Reichl]

On 07/18/2014 04:23 PM, Jakub Hrozek wrote:

Hi,

I think it's always good to add more logs.  Code LGTM so ACK++
> ret = getnameinfo((struct sockaddr *) &ss, ss_len,
>                  ip, sizeof(ip), NULL, 0, NI_NUMERICHOST);
> ip[NI_MAXHOST-1] = '\0';

I was just wondering why to set explicitly '\0' at end of  the string, 
man page says:

"On  success  0  is returned, and node and service names, if requested, 
are filled with *null-terminated strings*, possibly truncated to fit the 
specified buffer lengths."

I guess you are just being careful, which is OK with me.
> if (ret != 0) {
>    DEBUG(SSSDBG_MINOR_FAILURE, "getnameinfo failed\n");
>    return;
> }

[Lukas Slebodnik]

On (18/07/14 16:23), Jakub Hrozek wrote:
> On Fri, Jul 18, 2014 at 04:14:58PM +0200, Lukas Slebodnik wrote:
> > On (18/07/14 15:52), Jakub Hrozek wrote:
> > > Hi,
> > >
> > > I was trying to help a user who had problems with the AD provider and
> > > realized I had hard time following what exact server are we searching at
> > > the time and, because we hit a referral, where the referral pointed to.
> > >
> > > I wrote the two attached patches. I'm OK with keeping them in my local
> > > tree, but if other developers agree, maybe we can include them upstream
> > > as well.
> > > From 10edfd72516e7c1ac24b3ebe148825d12325e04e Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Fri, 18 Jul 2014 15:04:28 +0200
> > > Subject: [PATCH 1/2] LDAP: Print referrals for debugging purposes
>
> [...]
> > > @@ -1383,6 +1385,13 @@ static void sdap_get_generic_ext_done(struct sdap_op *op,
> > >                   sss_ldap_err2string(result), result,
> > >                   errmsg ? errmsg : "no errmsg set");
> > >    if (refs) {
> >
> > I thought you prefer refs != NULL
> > http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
>
> Fixed, I admit I copied this code from ldapsearch.c
> > >        for (i = 0; refs[i]; i++) {
> > >            DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", refs[i]);
> > >        }
> > >        ldap_memvfree((void **) refs);
> > >    }
> > > From 8b1ad7a4797b941d8d35700c7a5558f3362ba758 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Fri, 18 Jul 2014 15:37:34 +0200
> > > Subject: [PATCH 2/2] LDAP: Dump LDAP server IP address with a high DEBUG level
> > >
> > > src/providers/ldap/sdap_async.c | 37 +++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 37 insertions(+)
> > >
> > > diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> > > index 50e60ff846d9f94bb9f7343246429f1345286719..4e91c02b7ba82ad9d1d8500f84906f0e40f74399 100644
> > > --- a/src/providers/ldap/sdap_async.c
> > > +++ b/src/providers/ldap/sdap_async.c
> > > @@ -1083,6 +1083,41 @@ static errno_t add_to_deref_reply(TALLOC_CTX *mem_ctx,
> > >     return EOK;
> > > }
> > >
> > > +void sdap_print_server(struct sdap_handle *sh)
> >
> > It would be better to have this function static.
>
> Done.
> > > +{
> > > int ret;
> > > int fd;
> > > struct sockaddr_storage ss;
> > > socklen_t ss_len = sizeof(ss);
> > > char ip[NI_MAXHOST];
> > > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
> >
> > and one more time
> > http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
> > I couldn't resist :-) :-) :-)
>
> I don't agree with that blog post completely, I think using "!" if you
> really mean boolean false is OK. I dislike "if (!ptr)" as a way of
> saying "if (ptr == NULL)".
> if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       type of this expression is integer and not boolean :-)

LS

[Pavel Březina]

On 07/18/2014 04:52 PM, Lukas Slebodnik wrote:
> On (18/07/14 16:23), Jakub Hrozek wrote:
> > On Fri, Jul 18, 2014 at 04:14:58PM +0200, Lukas Slebodnik wrote:
> > > On (18/07/14 15:52), Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > I was trying to help a user who had problems with the AD provider and
> > > > realized I had hard time following what exact server are we searching at
> > > > the time and, because we hit a referral, where the referral pointed to.
> > > >
> > > > I wrote the two attached patches. I'm OK with keeping them in my local
> > > > tree, but if other developers agree, maybe we can include them upstream
> > > > as well.
> > > > From 10edfd72516e7c1ac24b3ebe148825d12325e04e Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Fri, 18 Jul 2014 15:04:28 +0200
> > > > Subject: [PATCH 1/2] LDAP: Print referrals for debugging purposes
> >
> > [...]
> > > > @@ -1383,6 +1385,13 @@ static void sdap_get_generic_ext_done(struct sdap_op *op,
> > > >                    sss_ldap_err2string(result), result,
> > > >                    errmsg ? errmsg : "no errmsg set");
> > > >    if (refs) {
> > >
> > > I thought you prefer refs != NULL
> > > http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
> >
> > Fixed, I admit I copied this code from ldapsearch.c
> > > >        for (i = 0; refs[i]; i++) {
> > > >            DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", refs[i]);
> > > >        }
> > > >        ldap_memvfree((void **) refs);
> > > >    }
> > > > From 8b1ad7a4797b941d8d35700c7a5558f3362ba758 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Fri, 18 Jul 2014 15:37:34 +0200
> > > > Subject: [PATCH 2/2] LDAP: Dump LDAP server IP address with a high DEBUG level
> > > >
> > > > src/providers/ldap/sdap_async.c | 37 +++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 37 insertions(+)
> > > >
> > > > diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> > > > index 50e60ff846d9f94bb9f7343246429f1345286719..4e91c02b7ba82ad9d1d8500f84906f0e40f74399 100644
> > > > --- a/src/providers/ldap/sdap_async.c
> > > > +++ b/src/providers/ldap/sdap_async.c
> > > > @@ -1083,6 +1083,41 @@ static errno_t add_to_deref_reply(TALLOC_CTX *mem_ctx,
> > > >      return EOK;
> > > > }
> > > >
> > > > +void sdap_print_server(struct sdap_handle *sh)
> > >
> > > It would be better to have this function static.
> >
> > Done.
> > > > +{
> > > > int ret;
> > > > int fd;
> > > > struct sockaddr_storage ss;
> > > > socklen_t ss_len = sizeof(ss);
> > > > char ip[NI_MAXHOST];
> > > > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
> > >
> > > and one more time
> > > http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
> > > I couldn't resist :-) :-) :-)
> >
> > I don't agree with that blog post completely, I think using "!" if you
> > really mean boolean false is OK. I dislike "if (!ptr)" as a way of
> > saying "if (ptr == NULL)".
> > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     type of this expression is integer and not boolean :-)

We have DEBUG_IS_SET macro for this type of tests.
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Jul 21, 2014 at 10:42:05AM +0200, Pavel Březina wrote:
> > > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >    type of this expression is integer and not boolean :-)
>
> We have DEBUG_IS_SET macro for this type of tests.

OK, I removed the explicit '\0' and used DEBUG_IS_SET().

[Jakub Hrozek]

On Mon, Jul 21, 2014 at 12:33:16PM +0200, Jakub Hrozek wrote:
> On Mon, Jul 21, 2014 at 10:42:05AM +0200, Pavel Březina wrote:
> > > > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
> > >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >    type of this expression is integer and not boolean :-)
> >
> > We have DEBUG_IS_SET macro for this type of tests.
>
> OK, I removed the explicit '\0' and used DEBUG_IS_SET().

Pavel mentioned on IRC I managed to reverse the condition of
DEBUG_IS_SET.

[Pavel Březina]

On 08/05/2014 12:43 PM, Jakub Hrozek wrote:
> On Mon, Jul 21, 2014 at 12:33:16PM +0200, Jakub Hrozek wrote:
> > On Mon, Jul 21, 2014 at 10:42:05AM +0200, Pavel Březina wrote:
> > > > > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
> > > >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >     type of this expression is integer and not boolean :-)
> > >
> > > We have DEBUG_IS_SET macro for this type of tests.
> >
> > OK, I removed the explicit '\0' and used DEBUG_IS_SET().
>
> Pavel mentioned on IRC I managed to reverse the condition of
> DEBUG_IS_SET.

Ack.

[Jakub Hrozek]

On Tue, Aug 05, 2014 at 12:47:13PM +0200, Pavel Březina wrote:
> On 08/05/2014 12:43 PM, Jakub Hrozek wrote:
> > On Mon, Jul 21, 2014 at 12:33:16PM +0200, Jakub Hrozek wrote:
> > > On Mon, Jul 21, 2014 at 10:42:05AM +0200, Pavel Březina wrote:
> > > > > > if (!(debug_level & SSSDBG_TRACE_INTERNAL)) {
> > > > >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > >    type of this expression is integer and not boolean :-)
> > > >
> > > > We have DEBUG_IS_SET macro for this type of tests.
> > >
> > > OK, I removed the explicit '\0' and used DEBUG_IS_SET().
> >
> > Pavel mentioned on IRC I managed to reverse the condition of
> > DEBUG_IS_SET.
>
> Ack.

Thank you, pushed to master:
    d5903e1982525d27cb135899c8827ec253599d8c
    f47a1a7add1bcf78f26e9be457ff1a771e7377df