From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [Patch 1/3] CLD: End-to-end verbosity Date: Tue, 06 Apr 2010 10:40:33 -0400 Message-ID: <4BBB47E1.1080901@garzik.org> References: <20100331184302.6ec6e69c@redhat.com> 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=624OgOl1eMInXAfqty6zeZY7K0vT+IEdWeTq/YWbdxw=; b=ErWhZ0fycFK+SM8M1G/mPDRwwsLHH6HrQ6Dn9ibhP0/kY9clF2I3LY3MH9E8194KY3 2T0zQC7CnMmoRnasozPyLRtZrMBqrgHw4qUMX9wkTtuoFXUFmEliF+78/ksuDe+VfBh6 +EhhXlqiaGUQF9xFBsP2kAOpT/av4lDNc98eU= In-Reply-To: <20100331184302.6ec6e69c@redhat.com> Sender: hail-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Pete Zaitcev Cc: Project Hail List 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