On Tue, Dec 04, 2012 at 01:24:39PM +0100, Petr Spacek wrote:
> On 11/22/2012 02:05 PM, Adam Tkac wrote:
> >On Tue, Nov 20, 2012 at 02:44:49PM +0100, Petr Spacek wrote:
> >>Hello,
> >>
> >>     Log failures detected in CHECK() macro.
> >>
> >>     Function ldap_query() can return ISC_R_NOTFOUND legitimately.
> >>     For this and similar cases CHECK_CONDLOG macro was introduced.
> >>     It will not log if result != ISC_R_SUCCESS but == ignored_code.
> >>     Nested condition will be eliminated by optimizing compiler
> >>     in cases where ignored_code == ISC_R_SUCCESS.
> >>
> >>     Function add_soa_record() is now called only for zones to prevent
> >>     false error messages.
> >
> >Nack.
> >
> >I don't like second part of the patch much, it adds huge amount of logging
> >and now we will log every error twice because we already log errors 
> >explicitly.
> >
> >In my opinion better will be to add new configuration option, for example
> >"debug", and with this option we can emit log messages from CHECK macros (I
> >haven't though about implementation details, yet). Otherwise we should avoid
> >logging because it's useless to log all errors, they are expected in 
> >production
> >environment.
> >
> >I also don't like CHECK_CONDLOG macro, it's not intuitive and with it we can 
> >end
> >with so called spaghetti code... As I wrote above I would log every CHECK
> >failure with debugging on.
> >
> >However the first patch of the patch is fine (the add_soa_record part).
> Ok, reworked patch is attached. Logging is enabled only if
> configuration option 'verbose_checks yes' is present. I
> decommissioned CHECK_CONDLOG(), so each request for non-existing
> record will log failure: not found (when verbose mode is enabled).

This looks fine for me. In future we might consider to add some rate-limiting
patch for log_error_position() calls because in production environment debug log
can be too huge but this is not blocker for the patch.

Ack

Regards, Adam

> From 0efaa684d8c536805762d9a835897889cac87d25 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspa...@redhat.com>
> Date: Tue, 4 Dec 2012 13:12:53 +0100
> Subject: [PATCH] Add option to log all failures detected in CHECK() macro.
> 
> Logging will be enabled if 'verbose_checks' option is set to 'yes'.
> 
> Function add_soa_record() is now called only for zones to prevent
> false error messages.
> 
> Signed-off-by: Petr Spacek <pspa...@redhat.com>
> ---
>  src/ldap_helper.c  |  7 +++----
>  src/settings.c     |  1 +
>  src/util.h         | 15 +++++++--------
>  src/zone_manager.c |  2 ++
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 
> 7d1febb68a4773d4e1127e8135d30fd855ded6a6..436985247803240f9ec4f2c3e5118adf8466beec
>  100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1694,10 +1694,9 @@ ldap_parse_rrentry(isc_mem_t *mctx, ldap_entry_t 
> *entry,
>       const char *dn = "<NULL entry>";
>       const char *data = "<NULL data>";
>  
> -     result = add_soa_record(mctx, qresult, origin, entry,
> -                             rdatalist, fake_mname);
> -     if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND)
> -             goto cleanup;
> +     if ((ldap_entry_getclass(entry) & LDAP_ENTRYCLASS_ZONE) != 0)
> +             CHECK(add_soa_record(mctx, qresult, origin, entry,
> +                                  rdatalist, fake_mname));
>  
>       rdclass = ldap_entry_getrdclass(entry);
>       ttl = ldap_entry_getttl(entry);
> diff --git a/src/settings.c b/src/settings.c
> index 
> 25578ce2687bf12e3a2d387caf0b26ed1a3083b6..08164766172f5f915584ae51b43e3d64798eed71
>  100644
> --- a/src/settings.c
> +++ b/src/settings.c
> @@ -32,6 +32,7 @@
>  #include "util.h"
>  #include "types.h"
>  
> +isc_boolean_t verbose_checks = ISC_FALSE; /* log each failure in CHECK() 
> macro */
>  
>  /*
>   * Forward declarations.
> diff --git a/src/util.h b/src/util.h
> index 
> c61f4e7a4930717cfd7729caa2c2f36854d1903f..d6d3c73e6d25657805eee904e6047c542e52a656
>  100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -21,6 +21,8 @@
>  #ifndef _LD_UTIL_H_
>  #define _LD_UTIL_H_
>  
> +extern isc_boolean_t verbose_checks; /* from settings.c */
> +
>  #include "log.h"
>  
>  #define CLEANUP_WITH(result_code)                            \
> @@ -32,15 +34,12 @@
>  #define CHECK(op)                                            \
>       do {                                                    \
>               result = (op);                                  \
> -             if (result != ISC_R_SUCCESS)                    \
> +             if (result != ISC_R_SUCCESS) {                  \
> +                     if (verbose_checks == ISC_TRUE)         \
> +                             log_error_position("check failed: %s",          
> \
> +                                                dns_result_totext(result));  
> \
>                       goto cleanup;                           \
> -     } while (0)
> -
> -#define CHECK_NEXT(op)                                               \
> -     do {                                                    \
> -             result = (op);                                  \
> -             if (result != ISC_R_SUCCESS)                    \
> -                     goto next;                              \
> +             }                                               \
>       } while (0)
>  
>  #define CHECKED_MEM_ALLOCATE(m, target_ptr, s)                       \
> diff --git a/src/zone_manager.c b/src/zone_manager.c
> index 
> 08ef91907a35564520b8ccb8d9993b49fc88a391..c19c3b6c91ff8114fcb15eacba0f74ec46047986
>  100644
> --- a/src/zone_manager.c
> +++ b/src/zone_manager.c
> @@ -121,6 +121,7 @@ manager_create_db_instance(isc_mem_t *mctx, const char 
> *name,
>       setting_t manager_settings[] = {
>               { "zone_refresh", default_uint(0) },
>               { "psearch", default_boolean(0) },
> +             { "verbose_checks", default_boolean(0) },
>               end_of_settings
>       };
>  
> @@ -139,6 +140,7 @@ manager_create_db_instance(isc_mem_t *mctx, const char 
> *name,
>       /* Parse settings. */
>       manager_settings[0].target = &zone_refresh;
>       manager_settings[1].target = &psearch;
> +     manager_settings[2].target = &verbose_checks; /* global variable */
>       CHECK(set_settings(manager_settings, argv));
>  
>       CHECKED_MEM_GET_PTR(mctx, db_inst);
> -- 
> 1.7.11.7
> 


-- 
Adam Tkac, Red Hat, Inc.

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to