Switching to [KMGTPE]i prefixes?

[Alexander Best]

On Fri Mar 25 11, Xin LI wrote:
> On Fri, Mar 25, 2011 at 3:32 PM, Alexander Best <arundel at freebsd.org> wrote:
> > On Fri Mar 25 11, Xin LI wrote:
> >> FYI I have a patch and I have incorporated some of Alexander's idea.
> >>
> >> Difference:
> >>
> >>  - Use of both HN_DIVISOR_1000 and HN_IEC_PREFIXES triggers an
> >> assertion.  I think it doesn't make sense to return since this is an
> >> API violation and we should just tell the caller explicitly;
> >
> > actually i vote for removing all asserts in humanize_number.c and return -1
> > based upon the later checks.
> >
> > the existing
> >
> > assert(buf != NULL);
> > assert(suffix != NULL);
> >
> > checks aren't really needed, since buf and suffix are also checked later on. so
> > just having:
> 
> Well, one of my believes is that a program should crash as early as
> possible, and with clear statement about "Why I crashed", when it's
> compiled with debugging aids, like assertions.  To test or not to test
> these cases in a release binary on the other hand really depends on
> whether there is security or other bad implications.  This generally
> makes developers' life easier, as they don't have to pursue into the
> code to find out why the program crashed, etc.
> 
> Unlike system calls, humanize_number(3) does not indicate what's wrong
> via errno, e.g. it tells you "No I can't" rather than a reason of "Why
> I can't do that".  Assertions here gives it an opportunity to say it
> loudly.
> 
> If however the program is compiled with -DNDEBUG, these assertions
> would became no-op.  At this stage, in my opinion, only basic tests
> should be done and we fall back to returning -1, or at least, not
> crash the program in a mysterious way.
> 
> For this reasons, I think the assertion here against flags is right,
> it does not hurt if we proceed with both flags set, as we do not
> access undefined memory nor overwrite undefined memory.  Furthermore,
> these values are more likely to be hard-wired at caller, where the
> assertion should catch the case.
> 
> >        if (scale <= 0 || (scale >= maxscale &&
> >            (scale & (HN_AUTOSCALE | HN_GETSCALE)) == 0))
> >                return (-1);
> 
> I think this one is good to have for both assertion and tests.  Note
> that I think it should be scale < 0 here, scale == 0 seems to be a
> valid value.
> 
> >        if (buf == NULL || suffix == NULL)
> >                return (-1);
> 
> This duplication is necessary in my opinion.  It's a protection
> against NULL pointer deference at runtime.
> 
> >        if ((flags & (HN_DIVISOR_1000 | HN_IEC_PREFIXES)) == 0)
> >                return (-1);
> 
> I'd vote no for this one for the reason above.
> 
> >>  - DIVISOR_1000 and !1000 cases use just same prefixes, so merge them
> >> while keeping divisor intact;
> >
> > good idea. however i think you should add a comment to point out that the
> > default behavior is !DIVISOR_1000 && !HN_IEC_PREFIXES. one has to look very
> > closely to find out.
> 
> Will do.
> 
> > #define HN_DECIMAL              0x01
> > #define HN_NOSPACE              0x02
> > #define HN_B                    0x04
> > #define HN_DIVISOR_1000         0x08
> > #define HN_IEC_PREFIXES         0x40
> >
> > #define HN_GETSCALE             0x10
> > #define HN_AUTOSCALE            0x20
> 
> Thinking again and I think we are just fine to use HN_IEC_PREFIXES ==
> 0x10 here.  I don't think there is a reason why we can't use 0x10 for
> flags.
> 
> Here is what in my mind.  I have stolen some comments from your
> version of patch to explain the meaning of the HN_IEC_PREFIXES option
> as well.

very nice. to me the patch looks close to perfect and most importantly it
won't break anything. :) even though the patch won't have any direct impact,
it enables all other base utilities to make use of the IEC prefixes. no idea
how that turns out. maybe the community votes to keep the current prefixes. the
main point however is: now there's a chance to chose. :)

i think the patch also requires a few humanize_number(3) man page changes. you
might want to cherry pick from
http://people.freebsd.org/~arundel/drafts/libutil.diff

...my changes to the man page are probably a bit too elaborate.

cheers.
alex

> 
> -- 
> Xin LI <delphij at delphij.net> http://www.delphij.net



-- 
a13x