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 <pspa...@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 <pspa...@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.

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

Reply via email to