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

[Jeff Garzik <jeff@garzik.org>]

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