Re: [RFCv2] cld: replace "if (verbose) { act_log }" with CLD_DEBUG

[Jeff Garzik <jeff@garzik.org>]

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@alumni.cmu.edu>
> ---
>   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