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
