On Fri, Mar 25, 2011 at 3:32 PM, Alexander Best <arun...@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.
--
Xin LI <delp...@delphij.net> http://www.delphij.net
Index: humanize_number.c
===================================================================
--- humanize_number.c (revision 220009)
+++ humanize_number.c (working copy)
@@ -42,45 +42,59 @@
#include <locale.h>
#include <libutil.h>
+static const int maxscale = 7;
+
int
humanize_number(char *buf, size_t len, int64_t quotient,
const char *suffix, int scale, int flags)
{
const char *prefixes, *sep;
- int i, r, remainder, maxscale, s1, s2, sign;
+ int i, r, remainder, s1, s2, sign;
int64_t divisor, max;
size_t baselen;
assert(buf != NULL);
assert(suffix != NULL);
assert(scale >= 0);
+ assert(scale < maxscale || (((scale & (HN_AUTOSCALE | HN_GETSCALE)) != 0)));
+ assert(!((flags & HN_DIVISOR_1000) && (flags & HN_IEC_PREFIXES)));
remainder = 0;
- if (flags & HN_DIVISOR_1000) {
- /* SI for decimal multiplies */
- divisor = 1000;
- if (flags & HN_B)
- prefixes = "B\0k\0M\0G\0T\0P\0E";
- else
- prefixes = "\0\0k\0M\0G\0T\0P\0E";
- } else {
+ if (flags & HN_IEC_PREFIXES) {
+ baselen = 2;
/*
- * binary multiplies
- * XXX IEC 60027-2 recommends Ki, Mi, Gi...
+ * Use the prefixes for power of two recommended by
+ * the International Electrotechnical Commission
+ * (IEC) in IEC 80000-3 (superseeding IEC 60027-2)
+ * (i.e. Ki, Mi, Gi...).
+ *
+ * HN_IEC_PREFIXES implies a divisor of 1024 here
+ * (use of HN_DIVISOR_1000 would have triggered
+ * an assertion earlier).
*/
divisor = 1024;
if (flags & HN_B)
- prefixes = "B\0K\0M\0G\0T\0P\0E";
+ prefixes = "B\0\0Ki\0Mi\0Gi\0Ti\0Pi\0Ei";
else
- prefixes = "\0\0K\0M\0G\0T\0P\0E";
+ prefixes = "\0\0Ki\0Mi\0Gi\0Ti\0Pi\0Ei";
+ } else {
+ baselen = 1;
+ if (flags & HN_DIVISOR_1000)
+ divisor = 1000;
+ else
+ divisor = 1024;
+
+ if (flags & HN_B)
+ prefixes = "B\0\0k\0\0M\0\0G\0\0T\0\0P\0\0E";
+ else
+ prefixes = "\0\0\0k\0\0M\0\0G\0\0T\0\0P\0\0E";
}
-#define SCALE2PREFIX(scale) (&prefixes[(scale) << 1])
- maxscale = 7;
+#define SCALE2PREFIX(scale) (&prefixes[(scale) * 3])
- if (scale >= maxscale &&
- (scale & (HN_AUTOSCALE | HN_GETSCALE)) == 0)
+ if (scale < 0 || (scale >= maxscale &&
+ (scale & (HN_AUTOSCALE | HN_GETSCALE)) == 0))
return (-1);
if (buf == NULL || suffix == NULL)
@@ -91,10 +105,10 @@
if (quotient < 0) {
sign = -1;
quotient = -quotient;
- baselen = 3; /* sign, digit, prefix */
+ baselen += 2; /* sign, digit */
} else {
sign = 1;
- baselen = 2; /* digit, prefix */
+ baselen += 1; /* digit */
}
if (flags & HN_NOSPACE)
sep = "";
Index: libutil.h
===================================================================
--- libutil.h (revision 220009)
+++ libutil.h (working copy)
@@ -220,7 +220,9 @@
#define HN_NOSPACE 0x02
#define HN_B 0x04
#define HN_DIVISOR_1000 0x08
+#define HN_IEC_PREFIXES 0x10
+/* maxscale = 0x07 */
#define HN_GETSCALE 0x10
#define HN_AUTOSCALE 0x20
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
