From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [RFCv2] cld: replace "if (verbose) { act_log }" with CLD_DEBUG Date: Mon, 07 Dec 2009 17:03:14 -0500 Message-ID: <4B1D7BA2.9020102@garzik.org> References: <1260114065-14473-1-git-send-email-cmccabe@alumni.cmu.edu> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:message-id:date:from :user-agent:mime-version:to:cc:subject:references:in-reply-to :content-type:content-transfer-encoding; bh=zxQh+gYF2A8u8cojNQGgf2kNpmcq2Ztbk+swJHZBxGU=; b=mjOMBaDBj/Kiq5Qemq6WJYsyDAMEhTEsL4X6bWJvd7T5NcX+ITZJKpN/fklIRwxtaa KPgROfpTUeP7VNfCjcjMy4vekKokHzkAB8JnaGlWzKUUS/+L8TuworGbNeVvLgzq6hJ/ EjNhcTHFtRyg1TTyRyORkqQ8LOID20RgPi394= In-Reply-To: <1260114065-14473-1-git-send-email-cmccabe@alumni.cmu.edu> Sender: hail-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Colin McCabe Cc: Project Hail List , Pete Zaitcev 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 > --- > 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 > #include > #include > +#include > > 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 > +#include > + > +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