[Patch 1/3] CLD: End-to-end verbosity

[Patch 1/3] CLD: End-to-end verbosity

[Pete Zaitcev]

It has been observed that it's not possible to enable the session-level
verbose dump with -v switch of cldcli anymore (post XDR and ncld).

The main issue is that it is impossible to set the session verbosity by
an application control. Secondary issue is that if application attempts
to change the verbosity level, it cannot do so for newly-created sessions,
because the session structure is allocated when session is created.

Contributing to the confusion is the desire not to enable excessive
verbosity in the CLD sessions whenever debugging is requested.

To fix these issues, the patch does the following:

 - Drops all commented-out inserts that assign additional verbosity
   from libraries. Permits top-level applications to specify verbosity
   (what cldcli -v was intended to do).

 - Splits per-packet verbosity away from debugging messages.

 - Establishes -v to mean "CLD protocol verbosity" across applications
   and the daemon. The existing -D now controls the program debugging
   only, and there is no bit mask. However in case of cld it continues
   to accept a numeric argument. It is compatible, does not hurt anything
   and may be used in the future.

Coincidentially this fixes the crash whenever there's a resolution
failure for an SRV record.

Signed-Off-By: Pete Zaitcev <zaitcev@redhat.com>

---
 include/cldc.h       |    1 
 include/hail_log.h   |   24 +++++----
 include/ncld.h       |    2 
 lib/cldc.c           |  104 ++++++++++++++++++++++++-----------------
 server/server.c      |   26 +++++++---
 test/basic-io.c      |    4 -
 test/basic-session.c |    2 
 test/lock-file.c     |    2 
 tools/cldcli.c       |   14 +++--
 tools/cldfuse.c      |    2 
 10 files changed, 109 insertions(+), 72 deletions(-)

commit 433d6a8b6e3c5ecadf54b3142ce6f60e95b00b8a
Author: Master <zaitcev@lembas.zaitcev.lan>
Date:   Wed Mar 31 18:07:21 2010 -0600

    End-to-end verbosity.

diff --git a/include/cldc.h b/include/cldc.h
index b3e4f07..12acd32 100644
--- a/include/cldc.h
+++ b/include/cldc.h
@@ -97,7 +97,6 @@ struct cldc_ops {
 				const void *buf, size_t buflen);
 	void		(*event)(void *private, struct cldc_session *,
 				 struct cldc_fh *, uint32_t);
-	void		(*errlog)(int prio, const char *fmt, ...);
 };
 
 /** a single CLD client session */
diff --git a/include/hail_log.h b/include/hail_log.h
index 88074a3..53a6780 100644
--- a/include/hail_log.h
+++ b/include/hail_log.h
@@ -10,31 +10,37 @@
 #endif
 
 struct hail_log {
-	void (*func)(int prio, const char *fmt, ...)
-		ATTR_PRINTF(2,3);
-	bool verbose;
+	void (*func)(int prio, const char *fmt, ...) ATTR_PRINTF(2,3);
+	bool debug;		/* unmutes HAIL_DEBUG */
+	bool verbose;		/* enables CLD session verbosity */
 };
 
-/** Print out a debug message if 'verbose' is enabled */
-#define HAIL_DEBUG(log, ...) \
+/** Print out a CLD session debug message if enabled */
+#define HAIL_VERBOSE(log, ...) \
 	if ((log)->verbose) { \
 		(log)->func(LOG_DEBUG, __VA_ARGS__); \
 	}
 
+/** Print out an application debug message if enabled */
+#define HAIL_DEBUG(log, ...) \
+	if ((log)->debug) { \
+		(log)->func(LOG_DEBUG, __VA_ARGS__); \
+	}
+
 /** Print out an informational log message */
 #define HAIL_INFO(log, ...) \
-	(log)->func(LOG_INFO, __VA_ARGS__);
+	(log)->func(LOG_INFO, __VA_ARGS__)
 
 /** Print out a warning message */
 #define HAIL_WARN(log, ...) \
-	(log)->func(LOG_WARNING, __VA_ARGS__);
+	(log)->func(LOG_WARNING, __VA_ARGS__)
 
 /** Print out an error message */
 #define HAIL_ERR(log, ...) \
-	(log)->func(LOG_ERR, __VA_ARGS__);
+	(log)->func(LOG_ERR, __VA_ARGS__)
 
 /** Print out a critical warning message */
 #define HAIL_CRIT(log, ...) \
-	(log)->func(LOG_CRIT, __VA_ARGS__);
+	(log)->func(LOG_CRIT, __VA_ARGS__)
 
 #endif /* __HAIL_LOG_H__ */
diff --git a/include/ncld.h b/include/ncld.h
index dbf126a..05f57a2 100644
--- a/include/ncld.h
+++ b/include/ncld.h
@@ -72,7 +72,7 @@ struct ncld_read {
 
 extern struct ncld_sess *ncld_sess_open(const char *host, int port,
 	int *error, void (*event)(void *, unsigned int), void *ev_arg,
-	const char *cld_user, const char *cld_key);
+	const char *cld_user, const char *cld_key, struct hail_log *log);
 extern struct ncld_fh *ncld_open(struct ncld_sess *s, const char *fname,
 	unsigned int mode, int *error, unsigned int events,
 	void (*event)(void *, unsigned int), void *ev_arg);
diff --git a/lib/cldc.c b/lib/cldc.c
index afe88ab..c7419e1 100644
--- a/lib/cldc.c
+++ b/lib/cldc.c
@@ -183,29 +183,26 @@ static int rxmsg_generic(struct cldc_session *sess,
 	while (tmp) {
 		req = tmp->data;
 
-		HAIL_DEBUG(&sess->log, "%s: comparing req->xid (%llu) "
-			   "with resp.xid_in (%llu)",
-			   __func__,
-			   (unsigned long long) req->xid,
-			   (unsigned long long) resp.xid_in);
+		HAIL_VERBOSE(&sess->log, "%s: comparing req->xid (%llu) "
+			     "with resp.xid_in (%llu)", __func__,
+			     (unsigned long long) req->xid,
+			     (unsigned long long) resp.xid_in);
 
 		if (req->xid == resp.xid_in)
 			break;
 		tmp = tmp->next;
 	}
 	if (!tmp) {
-		HAIL_DEBUG(&sess->log, "%s: no match found with "
-			   "xid_in %llu",
-			   __func__,
-			   (unsigned long long) resp.xid_in);
+		HAIL_VERBOSE(&sess->log, "%s: no match found with xid_in %llu",
+			     __func__, (unsigned long long) resp.xid_in);
 		return -1005;
 	}
 
 	if (req->done) {
-		HAIL_DEBUG(&sess->log, "%s: re-acking", __func__);
+		HAIL_VERBOSE(&sess->log, "%s: re-acking", __func__);
 	} else {
-		HAIL_DEBUG(&sess->log, "%s: issuing completion, acking",
-			   __func__);
+		HAIL_VERBOSE(&sess->log, "%s: issuing completion, acking",
+			     __func__);
 
 		req->done = true;
 
@@ -264,9 +261,9 @@ static int rxmsg_ack_frag(struct cldc_session *sess,
 			if (seqid != ack_msg.seqid)
 				continue;
 
-			HAIL_DEBUG(&sess->log, "%s: seqid %llu, expiring",
-				   __func__,
-				   (unsigned long long) ack_msg.seqid);
+			HAIL_VERBOSE(&sess->log, "%s: seqid %llu, expiring",
+				     __func__,
+				     (unsigned long long) ack_msg.seqid);
 
 			req->pkt_info[i] = NULL;
 			free(pi);
@@ -287,8 +284,8 @@ static int rxmsg_event(struct cldc_session *sess,
 
 	xdrmem_create(&xdrs, sess->msg_buf, sess->msg_buf_len, XDR_DECODE);
 	if (!xdr_cld_msg_event(&xdrs, &ev)) {
-		HAIL_INFO(&sess->log, "%s: failed to decode cld_msg_event",
-			  __func__);
+		HAIL_DEBUG(&sess->log, "%s: failed to decode cld_msg_event",
+			   __func__);
 		xdr_destroy(&xdrs);
 		return -1008;
 	}
@@ -397,8 +394,8 @@ static int accept_seqid(struct cldc_session *sess, uint64_t seqid,
 		sess->next_seqid_in = seqid + 1;
 		sess->next_seqid_in_tr =
 			sess->next_seqid_in - CLDC_MSG_REMEMBER;
-		HAIL_DEBUG(&sess->log, "%s: setting next_seqid_in to %llu",
-			   __func__, (unsigned long long) seqid);
+		HAIL_VERBOSE(&sess->log, "%s: setting next_seqid_in to %llu",
+			     __func__, (unsigned long long) seqid);
 		return 0;
 
 	case CMO_NOT_MASTER:
@@ -513,9 +510,8 @@ int cldc_receive_pkt(struct cldc_session *sess,
 	sess->expire_time = current_time + CLDC_SESS_EXPIRE;
 
 	if (pkt.mi.order & CLD_PKT_IS_LAST) {
-		HAIL_DEBUG(&sess->log, "%s: receiving complete message of "
-			   "op %s", __func__,
-			   __cld_opstr(sess->msg_buf_op));
+		HAIL_VERBOSE(&sess->log, "%s: receiving complete message of "
+			     "op %s", __func__, __cld_opstr(sess->msg_buf_op));
 		return rx_complete(sess, &pkt, foot);
 	} else {
 		return ack_seqid(sess, foot->seqid);
@@ -821,12 +817,12 @@ static ssize_t new_sess_cb(struct cldc_msg *msg, const void *resp_p,
 	return 0;
 }
 
-int cldc_new_sess(const struct cldc_ops *ops,
-		  const struct cldc_call_opts *copts,
-		  const void *addr, size_t addr_len,
-		  const char *user, const char *secret_key,
-		  void *private,
-		  struct cldc_session **sess_out)
+static int cldc_new_sess_log(const struct cldc_ops *ops,
+			     const struct cldc_call_opts *copts,
+			     const void *addr, size_t addr_len,
+			     const char *user, const char *secret_key,
+			     void *private, struct hail_log *log,
+			     struct cldc_session **sess_out)
 {
 	struct cldc_session *sess;
 	struct cldc_msg *msg;
@@ -845,13 +841,9 @@ int cldc_new_sess(const struct cldc_ops *ops,
 	if (!sess)
 		return -ENOMEM;
 
-#if 0
-	sess->log.verbose = true;
-#endif
-
 	sess->private = private;
 	sess->ops = ops;
-	sess->log.func = ops->errlog ? ops->errlog : cldc_errlog;
+	sess->log = *log;		/* save off caller's stack */
 	sess->fh = g_array_sized_new(FALSE, TRUE, sizeof(struct cldc_fh), 16);
 	strcpy(sess->user, user);
 	strcpy(sess->secret_key, secret_key);
@@ -886,6 +878,22 @@ int cldc_new_sess(const struct cldc_ops *ops,
 	return sess_send(sess, msg);
 }
 
+int cldc_new_sess(const struct cldc_ops *ops,
+		  const struct cldc_call_opts *copts,
+		  const void *addr, size_t addr_len,
+		  const char *user, const char *secret_key,
+		  void *private,
+		  struct cldc_session **sess_out)
+{
+	struct hail_log log;
+
+	log.debug = false;
+	log.verbose = false;
+	log.func = cldc_errlog;
+	return cldc_new_sess_log(ops, copts, addr, addr_len, user, secret_key,
+				 private, &log, sess_out);
+}
+
 /*
  * Force-clean the slate in the library. This may leave the server confused.
  */
@@ -1296,7 +1304,8 @@ char *cldc_dirent_name(struct cld_dirent_cur *dc)
 /*
  * On error, return the code (not negated code like a kernel function would).
  */
-static int ncld_getsrv(char **hostp, unsigned short *portp)
+static int ncld_getsrv(char **hostp, unsigned short *portp,
+		       struct hail_log *log)
 {
 	enum { hostsz = 64 };
 	char hostb[hostsz];
@@ -1308,7 +1317,7 @@ static int ncld_getsrv(char **hostp, unsigned short *portp)
 		return errno;
 	hostb[hostsz-1] = 0;
 
-	if (cldc_getaddr(&host_list, hostb, NULL))
+	if (cldc_getaddr(&host_list, hostb, log))
 		return 1001;
 
 	/*
@@ -1501,7 +1510,6 @@ static struct cldc_ops ncld_ops = {
 	.timer_ctl	= ncld_p_timer_ctl,
 	.pkt_send	= ncld_p_pkt_send,
 	.event		= ncld_p_event,
-	.errlog		= NULL,
 };
 
 static int ncld_new_sess(struct cldc_call_opts *copts, enum cle_err_codes errc)
@@ -1547,18 +1555,28 @@ static int ncld_wait_session(struct ncld_sess *nsess)
  * @param ev_arg User-supplied argument to the session event function
  * @param cld_user The user identifier to be used to authentication
  * @param cld_key The user key to be used to authentication
+ * @param log The application log descriptor (ok to be NULL)
  */
 struct ncld_sess *ncld_sess_open(const char *host, int port, int *error,
 				 void (*ev_func)(void *, unsigned int),
-				 void *ev_arg, const char *cld_user,
-				 const char *cld_key)
+				 void *ev_arg,
+				 const char *cld_user, const char *cld_key,
+				 struct hail_log *log)
 {
 	struct ncld_sess *nsess;
+	struct hail_log nlog;
 	struct cldc_call_opts copts;
 	int err;
 	GError *gerr;
 	int rc;
 
+	if (!log) {
+		nlog.debug = false;
+		nlog.verbose = false;
+		nlog.func = cldc_errlog;
+		log = &nlog;
+	}
+
 	err = ENOMEM;
 	nsess = malloc(sizeof(struct ncld_sess));
 	if (!nsess)
@@ -1574,7 +1592,7 @@ struct ncld_sess *ncld_sess_open(const char *host, int port, int *error,
 		goto out_cond;
 
 	if (!host) {
-		err = ncld_getsrv(&nsess->host, &nsess->port);
+		err = ncld_getsrv(&nsess->host, &nsess->port, log);
 		if (err)
 			goto out_srv;
 	} else {
@@ -1605,14 +1623,14 @@ struct ncld_sess *ncld_sess_open(const char *host, int port, int *error,
 	memset(&copts, 0, sizeof(copts));
 	copts.cb = ncld_new_sess;
 	copts.private = nsess;
-	if (cldc_new_sess(&ncld_ops, &copts, nsess->udp->addr, nsess->udp->addr_len,
-			  cld_user, cld_key, nsess, &nsess->udp->sess)) {
+	if (cldc_new_sess_log(&ncld_ops, &copts,
+			      nsess->udp->addr, nsess->udp->addr_len,
+			      cld_user, cld_key, nsess, log,
+			      &nsess->udp->sess)) {
 		err = 1024;
 		goto out_session;
 	}
 
-	/* nsess->udp->sess->log.verbose = 1; */
-
 	rc = ncld_wait_session(nsess);
 	if (rc) {
 		err = 1100 + rc % 1000;
diff --git a/server/server.c b/server/server.c
index 3208e0f..2d68ee6 100644
--- a/server/server.c
+++ b/server/server.c
@@ -55,7 +55,7 @@ static struct argp_option options[] = {
 	  "Store database environment in DIRECTORY.  Default: "
 	  CLD_DEF_DATADIR },
 	{ "debug", 'D', "LEVEL", 0,
-	  "Set debug output to LEVEL (0 = off, 2 = max)" },
+	  "Set debug output to LEVEL (0 = off, 1 = debugging)" },
 	{ "stderr", 'E', NULL, 0,
 	  "Switch the log to standard error" },
 	{ "foreground", 'F', NULL, 0,
@@ -64,6 +64,8 @@ static struct argp_option options[] = {
 	  "Bind to UDP port PORT.  Default: " CLD_DEF_PORT },
 	{ "pid", 'P', "FILE", 0,
 	  "Write daemon process id to FILE.  Default: " CLD_DEF_PIDFN },
+	{ "verbose", 'v', NULL, 0,
+	  "Enable the session-level verbosity" },
 	{ "strict-free", 1001, NULL, 0,
 	  "For memory-checker runs.  When shutting down server, free local "
 	  "heap, rather than simply exit(2)ing and letting OS clean up." },
@@ -117,7 +119,6 @@ static void applog(int prio, const char *fmt, ...)
 
 struct hail_log srv_log = {
 	.func = applog,
-	.verbose = 0,
 };
 
 int udp_tx(int sock_fd, struct sockaddr *addr, socklen_t addr_len,
@@ -918,9 +919,14 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
 		cld_srv.data_dir = arg;
 		break;
 	case 'D':
-		if (atoi(arg) >= 0 && atoi(arg) <= 2)
-			srv_log.verbose = atoi(arg);
-		else {
+		switch (atoi(arg)) {
+		case 0:
+			srv_log.debug = false;
+			break;
+		case 1:
+			srv_log.debug = true;
+			break;
+		default:
 			fprintf(stderr, "invalid debug level: '%s'\n", arg);
 			argp_usage(state);
 		}
@@ -949,6 +955,9 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
 	case 'P':
 		cld_srv.pid_file = arg;
 		break;
+	case 'v':
+		srv_log.verbose = true;
+		break;
 
 	case 1001:			/* --strict-free */
 		strict_free = true;
@@ -1120,9 +1129,10 @@ int main (int argc, char *argv[])
 	if (rc)
 		goto err_out_pid;
 
-	HAIL_INFO(&srv_log, "initialized: verbose %u%s",
-	       srv_log.verbose,
-	       strict_free ? ", strict-free" : "");
+	HAIL_INFO(&srv_log, "initialized: %s%s%s",
+		  srv_log.debug ? "debug" : "nodebug",
+		  srv_log.verbose ? ", verbose" : "",
+		  strict_free ? ", strict-free" : "");
 
 	/*
 	 * execute main loop
diff --git a/test/basic-io.c b/test/basic-io.c
index 31bfcab..1741590 100644
--- a/test/basic-io.c
+++ b/test/basic-io.c
@@ -40,7 +40,7 @@ static int test_write(int port)
 	int error;
 
 	nsess = ncld_sess_open(TEST_HOST, port, &error, NULL, NULL,
-			     TEST_USER, TEST_USER_KEY);
+			     TEST_USER, TEST_USER_KEY, NULL);
 	if (!nsess) {
 		fprintf(stderr, "ncld_sess_open(host %s port %u) failed: %d\n",
 			TEST_HOST, port, error);
@@ -74,7 +74,7 @@ static int test_read(int port)
 	int error;
 
 	nsess = ncld_sess_open(TEST_HOST, port, &error, NULL, NULL,
-			     TEST_USER, TEST_USER_KEY);
+			     TEST_USER, TEST_USER_KEY, NULL);
 	if (!nsess) {
 		fprintf(stderr, "ncld_sess_open(host %s port %u) failed: %d\n",
 			TEST_HOST, port, error);
diff --git a/test/basic-session.c b/test/basic-session.c
index 6caf046..c3e2a98 100644
--- a/test/basic-session.c
+++ b/test/basic-session.c
@@ -45,7 +45,7 @@ int main (int argc, char *argv[])
 		return -1;
 
 	nsess = ncld_sess_open(TEST_HOST, port, &error, NULL, NULL,
-			     TEST_USER, TEST_USER_KEY);
+			     TEST_USER, TEST_USER_KEY, NULL);
 	if (!nsess) {
 		fprintf(stderr, "ncld_sess_open(host %s port %u) failed: %d\n",
 			TEST_HOST, port, error);
diff --git a/test/lock-file.c b/test/lock-file.c
index d0ea9c1..36cc62c 100644
--- a/test/lock-file.c
+++ b/test/lock-file.c
@@ -50,7 +50,7 @@ int main (int argc, char *argv[])
 		return -1;
 
 	nsess = ncld_sess_open(TEST_HOST, port, &error, NULL, NULL,
-			     TEST_USER, TEST_USER_KEY);
+			     TEST_USER, TEST_USER_KEY, NULL);
 	if (!nsess) {
 		fprintf(stderr, "ncld_sess_open(host %s port %u) failed: %d\n",
 			TEST_HOST, port, error);
diff --git a/tools/cldcli.c b/tools/cldcli.c
index 2a2c82b..78887fc 100644
--- a/tools/cldcli.c
+++ b/tools/cldcli.c
@@ -98,7 +98,6 @@ static void applog(int prio, const char *fmt, ...)
 
 static struct hail_log cli_log = {
 	.func = applog,
-	.verbose = 0,
 };
 
 static void sess_event(void *private, uint32_t what)
@@ -630,9 +629,14 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
 {
 	switch(key) {
 	case 'D':
-		if (atoi(arg) >= 0 && atoi(arg) <= 2)
-			cli_log.verbose = atoi(arg);
-		else {
+		switch (atoi(arg)) {
+		case 0:
+			cli_log.debug = false;
+			break;
+		case 1:
+			cli_log.debug = true;
+			break;
+		default:
 			fprintf(stderr, TAG ": invalid debug level: '%s'\n",
 				arg);
 			argp_usage(state);
@@ -711,7 +715,7 @@ int main (int argc, char *argv[])
 	dr = host_list->data;
 
 	nsess = ncld_sess_open(dr->host, dr->port, &error, sess_event, NULL,
-			     "cldcli", "cldcli");
+			     "cldcli", "cldcli", &cli_log);
 	if (!nsess) {
 		if (error < 1000) {
 			fprintf(stderr, TAG ": cannot open CLD session: %s\n",
diff --git a/tools/cldfuse.c b/tools/cldfuse.c
index f40ad75..2cba917 100644
--- a/tools/cldfuse.c
+++ b/tools/cldfuse.c
@@ -375,7 +375,7 @@ int main(int argc, char *argv[])
 	dr = param.host_list->data;
 
 	sess = ncld_sess_open(dr->host, dr->port, &error, sess_event, NULL,
-			     "cldfuse", "cldfuse");
+			     "cldfuse", "cldfuse", &cldfuse_log);
 	if (!sess) {
 		if (error < 1000) {
 			fprintf(stderr, TAG ": cannot open CLD session: %s\n",

Re: [Patch 1/3] CLD: End-to-end verbosity

[Jeff Garzik]

On 03/31/2010 08:43 PM, Pete Zaitcev wrote:
> diff --git a/server/server.c b/server/server.c
> index 3208e0f..2d68ee6 100644
> --- a/server/server.c
> +++ b/server/server.c
> @@ -55,7 +55,7 @@ static struct argp_option options[] = {
>   	  "Store database environment in DIRECTORY.  Default: "
>   	  CLD_DEF_DATADIR },
>   	{ "debug", 'D', "LEVEL", 0,
> -	  "Set debug output to LEVEL (0 = off, 2 = max)" },
> +	  "Set debug output to LEVEL (0 = off, 1 = debugging)" },
>   	{ "stderr", 'E', NULL, 0,
>   	  "Switch the log to standard error" },
>   	{ "foreground", 'F', NULL, 0,
> @@ -64,6 +64,8 @@ static struct argp_option options[] = {
>   	  "Bind to UDP port PORT.  Default: " CLD_DEF_PORT },
>   	{ "pid", 'P', "FILE", 0,
>   	  "Write daemon process id to FILE.  Default: " CLD_DEF_PIDFN },
> +	{ "verbose", 'v', NULL, 0,
> +	  "Enable the session-level verbosity" },
>   	{ "strict-free", 1001, NULL, 0,
>   	  "For memory-checker runs.  When shutting down server, free local "
>   	  "heap, rather than simply exit(2)ing and letting OS clean up." },


As is hinted by the current code's debugging switch being an integer 
'level' value, the server [and client?] has increasing levels of 
verbosity.  The debug levels are

	0: key messages affecting server operation, only
	1: debugging output enabled, sans per-packet output
	2: debugging output enabled, including per-packet output

ie. clearly ordered by increasing value == increased verbosity.

As is clearly illustrated when I cut the patch down to the above 
snippet, the user interface you have created gives the user two "knobs" 
for log verbosity, and it is not clear to a casual user which knob 
controls which sets of messages.  That makes for a -more- confusing user 
interface, because the user must constantly ask themselves the question 
"do I need debug?  or verbose?  I don't know!"

Additionally, this interface changes runs counter to other tools, which 
increase verbosity with added "-v" switches -- analagous to the existing 
integer-based debug level interface.

If it is truly your desire to permit fine-grained selection of certain 
classes of messages, then don't dick around!  Go ahead and create a 
bitmap "log mask" which permits fine-grained selection of various 
messages, much like netif_msg_* and netif_msg_init() in the kernel's 
include/linux/netdevice.h.

Having two switches, -d and -v, for different, undocumented classes of 
message just increases confusion.  Put yourself in the mind of a user 
trying to figure out which is which.

I readily admit the __internal implementation__ resulting from your 
patches is a useful cleanup, but at a macro level, it merely increases 
logging user interface confusion.

	Jeff

Re: [Patch 1/3] CLD: End-to-end verbosity

[Pete Zaitcev]

On Tue, 06 Apr 2010 10:40:33 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> The debug levels are
> 
> 	0: key messages affecting server operation, only
> 	1: debugging output enabled, sans per-packet output
> 	2: debugging output enabled, including per-packet output

The previous patch did just that:

@@ -918,9 +917,17 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
 		cld_srv.data_dir = arg;
 		break;
 	case 'D':
-		if (atoi(arg) >= 0 && atoi(arg) <= 2)
-			srv_log.verbose = atoi(arg);
-		else {
+		switch (atoi(arg)) {
+		case 0:
+			break;
+		case 1:
+			srv_log.debug = true;
+			break;
+		case 2:
+			srv_log.debug = true;
+			srv_log.verbose = true;
+			break;
+		default:
 			fprintf(stderr, "invalid debug level: '%s'\n", arg);
 			argp_usage(state);
 		}

Why did you reject it?

> ... the user interface you have created gives the user two "knobs" 

I was your idea, not mine. Do you want me to REMOVE -v from
cldcli now?

-- Pete

Re: [Patch 1/3] CLD: End-to-end verbosity

[Jeff Garzik]

On 04/06/2010 11:32 PM, Pete Zaitcev wrote:
> On Tue, 06 Apr 2010 10:40:33 -0400
> Jeff Garzik<jeff@garzik.org>  wrote:
>
>> The debug levels are
>>
>> 	0: key messages affecting server operation, only
>> 	1: debugging output enabled, sans per-packet output
>> 	2: debugging output enabled, including per-packet output
>
> The previous patch did just that:
> Why did you reject it?

That's a damned good question.  I have no idea.  Did I ever reply to 
that patch?  It looks like I fscked up and missed it?

	Jeff

Re: [Patch 1/3] CLD: End-to-end verbosity

[Pete Zaitcev]

On Wed, 07 Apr 2010 00:36:32 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> On 04/06/2010 11:32 PM, Pete Zaitcev wrote:
> > On Tue, 06 Apr 2010 10:40:33 -0400
> > Jeff Garzik<jeff@garzik.org>  wrote:
> >
> >> The debug levels are
> >>
> >> 	0: key messages affecting server operation, only
> >> 	1: debugging output enabled, sans per-packet output
> >> 	2: debugging output enabled, including per-packet output
> >
> > The previous patch did just that:
> > Why did you reject it?
> 
> That's a damned good question.  I have no idea.  Did I ever reply to 
> that patch?  It looks like I fscked up and missed it?

Apparently no reply (by e-mail):
http://marc.info/?l=hail-devel&m=126575343714155&w=2

I recall you said that "users hate bitmasks" on IRC or in other thread,
so I thought that -D and -v would be right (effectively a bitmask but
no hexadecimal at least).

-- Pete