From owner-freebsd-hackers@FreeBSD.ORG Tue Dec 25 19:46:29 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id AA12963D for ; Tue, 25 Dec 2012 19:46:29 +0000 (UTC) (envelope-from cliftonr@volcano.org) Received: from hrndva-omtalb.mail.rr.com (hrndva-omtalb.mail.rr.com [71.74.56.122]) by mx1.freebsd.org (Postfix) with ESMTP id 5CCFF8FC17 for ; Tue, 25 Dec 2012 19:46:29 +0000 (UTC) X-Authority-Analysis: v=2.0 cv=O9a7TWBW c=1 sm=0 a=nLSj6wnnDCA9frvL/Tt8NQ==:17 a=3pwD0LKv0YcA:10 a=z1TLwsU0kBEA:10 a=WthmilfBrBsA:10 a=sLPLfFCmqb0A:10 a=kj9zAlcOel0A:10 a=GjEiR67sAAAA:8 a=vbqy14apGeAA:10 a=6I5d2MoRAAAA:8 a=4FFUHJPBAAAA:8 a=KPhu4TmfTkWQfB3IJaIA:9 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 a=WglDhLyllLQA:10 a=uGOeqZtyLdsmXGBv:21 a=rwX60XwcmPY_VYHc:21 a=nLSj6wnnDCA9frvL/Tt8NQ==:117 X-Cloudmark-Score: 0 X-Authenticated-User: X-Originating-IP: 70.95.171.134 Received: from [70.95.171.134] ([70.95.171.134:39643] helo=oz.volcano.org) by hrndva-oedge04.mail.rr.com (envelope-from ) (ecelerity 2.2.3.46 r()) with ESMTP id BF/D1-01424-3920AD05; Tue, 25 Dec 2012 19:46:28 +0000 Received: by oz.volcano.org (Postfix, from userid 1001) id DC36F5082E; Tue, 25 Dec 2012 09:46:26 -1000 (HST) Date: Tue, 25 Dec 2012 09:46:26 -1000 From: Clifton Royston To: freebsd-hackers@freebsd.org, John-Mark Gurney Subject: Re: looking for someone to fix humanize_number (test cases included) Message-ID: <20121225194626.GA57447@volcano.org> Mail-Followup-To: freebsd-hackers@freebsd.org, John-Mark Gurney References: <20121225172037.GA56225@volcano.org> <20121225182355.GA56732@volcano.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20121225182355.GA56732@volcano.org> User-Agent: Mutt/1.4.2.3i X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Dec 2012 19:46:29 -0000 On Tue, Dec 25, 2012 at 08:23:55AM -1000, Clifton Royston wrote: > On Tue, Dec 25, 2012 at 07:20:37AM -1000, Clifton Royston wrote: > > On Mon, Dec 24, 2012 at 12:00:01PM +0000, freebsd-hackers-request@freebsd.org wrote: > > > From: John-Mark Gurney > > > To: hackers@FreeBSD.org > > > Subject: looking for someone to fix humanize_number (test cases > > > included) > > > > > > I'm looking for a person who is interested in fixing up humanize_number. > ... > > > So I decided to write a test program to test the output, and now I'm even > > > more surprised by the output... Neither 7.2-R nor 10-current give what > > > I expect are the correct results... > ... > > I am bemused. I correct myself: the function works fine, and there are no bugs I could find, though it's clear the man page could emphasize the correct usage a bit more. I had to read the source several times and start on debugging it before I understood the correct usage of the flag values with the scale and flags parameters, despite the man page stating: The following flags may be passed in scale: HN_AUTOSCALE Format the buffer using the lowest multiplier pos- sible. HN_GETSCALE Return the prefix index number (the number of times number must be divided to fit) instead of formatting it to the buffer. The following flags may be passed in flags: HN_DECIMAL If the final result is less than 10, display it using one digit. ... HN_DIVISOR_1000 Divide number with 1000 instead of 1024. That is, certain flags must be passed in flags and others must only be passed in scale - a bit counter-intuitive. Also, scale == 0 is clearly not interpreted as AUTOSCALE, but I am not yet clear how it is being handled - it seems somewhat like AUTOSCALE but not identical. When the test program constant table is updated to pass the scale flags as specified, as well as fixing the bugs mentioned in the previous emails, it all passes except for the one (intentional?) inconsistency that "k" is used in place of "K" if HN_DECIMAL is enabled. The bug in the transfer speed results which prompted this inquiry suggests that perhaps some clients of humanize_number in the codebase are also passing the scale parameters incorrectly. I would propose accepting HN_AUTOSCALE and HN_GETSCALE in the flags field (they don't overlap with other values) while continuing to accept them in the scale field for backwards compatibility. Trivial diff below. -- Clifton -- --- /usr/src/lib/libutil/humanize_number.c 2010-12-28 09:36:31.000000000 -1000 +++ humanize_number.c 2012-12-25 09:36:36.000000000 -1000 @@ -54,7 +54,7 @@ const char *suffix, int scale, int flags) { const char *prefixes, *sep; - int b, i, r, maxscale, s1, s2, sign; + int b, i, r, maxscale, s1, s2, sign, autoscale, getscale; int64_t divisor, max; size_t baselen; @@ -84,8 +84,10 @@ #define SCALE2PREFIX(scale) (&prefixes[(scale) << 1]) maxscale = 7; + autoscale = (flags | scale) & HN_AUTOSCALE; + getscale = (flags | scale) & HN_GETSCALE; if (scale >= maxscale && - (scale & (HN_AUTOSCALE | HN_GETSCALE)) == 0) + (autoscale | getscale) == 0) return (-1); if (buf == NULL || suffix == NULL) @@ -114,7 +116,7 @@ if (len < baselen + 1) return (-1); - if (scale & (HN_AUTOSCALE | HN_GETSCALE)) { + if (autoscale | getscale) { /* See if there is additional columns can be used. */ for (max = 100, i = len - baselen; i-- > 0;) max *= 10; @@ -127,7 +129,7 @@ for (i = 0; bytes >= max - 50 && i < maxscale; i++) bytes /= divisor; - if (scale & HN_GETSCALE) + if (getscale) return (i); } else for (i = 0; i < scale && i < maxscale; i++)