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

[Adam Tkac]

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).

Regards, Adam

> From 2d17a81506c8851d5aa6e6f17a9cf72146b4b9c9 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at redhat.com>
> Date: Tue, 20 Nov 2012 13:58:32 +0100
> Subject: [PATCH] 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.
> 
> Signed-off-by: Petr Spacek <pspacek at redhat.com>
> ---
>  src/ldap_helper.c | 12 ++++++------
>  src/util.h        | 17 ++++++++---------
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index ef0fb69413d0febe8334bb5156d879fa1f0b9f16..7da07eabd5c712006276021d56e5972b4de9a3e1 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1687,10 +1687,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);
> @@ -1814,8 +1813,9 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
>  	CHECK(str_new(mctx, &string));
>  	CHECK(dnsname_to_dn(ldap_inst->zone_register, name, string));
>  
> -	CHECK(ldap_query(ldap_inst, NULL, &ldap_qresult, str_buf(string),
> -			 LDAP_SCOPE_BASE, NULL, 0, "(objectClass=idnsRecord)"));
> +	CHECK_CONDLOG(ldap_query(ldap_inst, NULL, &ldap_qresult, str_buf(string),
> +		      LDAP_SCOPE_BASE, NULL, 0, "(objectClass=idnsRecord)"),
> +		      ISC_R_NOTFOUND);
>  
>  	if (EMPTY(ldap_qresult->ldap_entries)) {
>  		result = ISC_R_NOTFOUND;
> diff --git a/src/util.h b/src/util.h
> index c61f4e7a4930717cfd7729caa2c2f36854d1903f..12d77df68d96556b9c8e42b3cd176a1cd3386c91 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -29,18 +29,17 @@
>  		goto cleanup;					\
>  	} while(0)
>  
> -#define CHECK(op)						\
> -	do {							\
> -		result = (op);					\
> -		if (result != ISC_R_SUCCESS)			\
> -			goto cleanup;				\
> -	} while (0)
> +#define CHECK(op)	CHECK_CONDLOG(op, ISC_R_SUCCESS)
>  
> -#define CHECK_NEXT(op)						\
> +#define CHECK_CONDLOG(op, ignored_code)				\
>  	do {							\
>  		result = (op);					\
> -		if (result != ISC_R_SUCCESS)			\
> -			goto next;				\
> +		if (result != ISC_R_SUCCESS) {			\
> +			if (result != ignored_code)		\
> +				log_error_position("check failed: %s",		\
> +						   dns_result_totext(result));	\
> +			goto cleanup;				\
> +		}						\
>  	} while (0)
>  
>  #define CHECKED_MEM_ALLOCATE(m, target_ptr, s)			\
> -- 
> 1.7.11.7
> 


-- 
Adam Tkac, Red Hat, Inc.