On 12/06/2009 10:41 AM, Colin McCabe wrote:
Move prototypes for common.c functions out of cld_msg.h and into a new header
file, common.h. Create a structure that represents the current log level and
also the function to use for logging.
Create CLD_DEBUG and CLD_LOG macros to print debugging and informational log
messages, respectively. CLD_DEBUG will not evaluate its parameters unless
log->verbose is true. This patch converts over the client code to use these
macros.
Rationale:
1. Some functions in common.c can be called from the server _and_ the client
code. If these functions want to print a debug message, they would currently
need two extra parameters-- the logging function, and the verbose flag.
It's cleaner to condense this into one parameter.
2. The if (verbose) sess->act_log pattern leads to an excessive level of
indentation, which tends to make things more unclear.
version 2: add common.h
Signed-off-by: Colin McCabe<cmccabe@xxxxxxxxxxxxxx>
---
include/cld_msg.h | 10 -----
include/cldc.h | 5 +--
include/common.h | 27 ++++++++++++++
lib/cldc.c | 98 +++++++++++++++++++++-------------------------------
tools/cldcli.c | 2 +-
5 files changed, 70 insertions(+), 72 deletions(-)
create mode 100644 include/common.h
diff --git a/include/cld_msg.h b/include/cld_msg.h
index 641b857..01837b7 100644
--- a/include/cld_msg.h
+++ b/include/cld_msg.h
@@ -250,14 +250,4 @@ struct cld_msg_event {
uint8_t res[4];
};
-/*
- * function prototypes for lib/common.c;
- * ideally these should not be in cld_msg.h
- */
-
-extern unsigned long long cld_sid2llu(const uint8_t *sid);
-extern void __cld_rand64(void *p);
-extern const char *cld_errstr(enum cle_err_codes ecode);
-extern int cld_readport(const char *fname);
-
#endif /* __CLD_MSG_H__ */
diff --git a/include/cldc.h b/include/cldc.h
index 9ae6dfe..70062cf 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -5,6 +5,7 @@
#include<stdbool.h>
#include<glib.h>
#include<cld_msg.h>
+#include<common.h>
struct cldc_session;
@@ -84,10 +85,8 @@ struct cldc_ops {
struct cldc_session {
uint8_t sid[CLD_SID_SZ]; /* client id */
- bool verbose;
-
const struct cldc_ops *ops;
- void (*act_log)(int prio, const char *fmt, ...);
+ struct cld_log log;
void *private;
uint8_t addr[64]; /* server address */
diff --git a/include/common.h b/include/common.h
new file mode 100644
index 0000000..c98f4ab
--- /dev/null
+++ b/include/common.h
@@ -0,0 +1,27 @@
+#ifndef __CLD_COMMON_H__
+#define __CLD_COMMON_H__
+
+#include<stdint.h>
+#include<glib.h>
+
+unsigned long long cld_sid2llu(const uint8_t *sid);
+void __cld_rand64(void *p);
+const char *cld_errstr(enum cle_err_codes ecode);
+int cld_readport(const char *fname);
+
+/** Print out a debug message if 'verbose' is enabled */
+#define CLD_DEBUG(cld_log, ...) \
+ if ((cld_log).verbose) { \
+ (cld_log).fun(LOG_DEBUG, __VA_ARGS__); \
+ }
+
+/** Print out an informational log message */
+#define CLD_LOG(cld_log, ...) \
+ (cld_log).fun(LOG_INFO, __VA_ARGS__);
+
+struct cld_log {
+ void (*fun)(int prio, const char *fmt, ...);
+ bool verbose;
No major objections. I would rather call the hook 'cb' or 'func' or
something other than 'fun', even though I do consider Project Hail fun :)
And thinking long-term, the ideal target is giving the admin the ability
to selectively enable or disable various classes of logging messages.
In a very rudimentary form, this means a "log_level" variable where
increasing values imply increasing verbosity. In a more refined form, a
la ISC's BIND server, the admin may mask log classes A, B, C, G, H and I
into /var/log/foobar, and mask log classes B, C, D, E anf F into
/var/log/bazbang.
What does that mean for your patch? Probably nothing :) But maybe we
might want 'verbose' to be an unsigned integer log_level or log_mask.
But that's optional... this email is more for future readers than
present programmers, I think.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
