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

--
Petr^2 Spacek
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

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

Reply via email to