[Freeipa-devel] [PATCH 0098] Log failures detected in CHECK() macro

[Adam Tkac]

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 <pspacek at 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 <pspacek at 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.