Date: Tue, 25 Dec 2012 09:46:26 -1000 From: Clifton Royston <cliftonr@volcano.org> To: freebsd-hackers@freebsd.org, John-Mark Gurney <jmg@funkthat.com> Subject: Re: looking for someone to fix humanize_number (test cases included) Message-ID: <20121225194626.GA57447@volcano.org> In-Reply-To: <20121225182355.GA56732@volcano.org> References: <mailman.15.1356350401.35494.freebsd-hackers@freebsd.org> <20121225172037.GA56225@volcano.org> <20121225182355.GA56732@volcano.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <jmg@funkthat.com>
> > > 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++)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121225194626.GA57447>