On Sat, 2011-10-01 at 12:33 -0700, Joe Perches wrote:
> On Fri, 2011-09-30 at 17:08 -0700, Dave Hansen wrote:
> > Instead of explicitly storing the entire string for each
> > possible units, just store the thing that varies: the
> > first character.
>
> trivia
I'm not sure what you mean by that.
> > diff -puN lib/string_helpers.c~string_get_size-pow2 lib/string_helpers.c
> > --- linux-2.6.git/lib/string_helpers.c~string_get_size-pow2 2011-09-30 16:50:31.628981352 -0700
> > +++ linux-2.6.git-dave/lib/string_helpers.c 2011-09-30 17:04:02.211607364 -0700
> > @@ -8,6 +8,23 @@
> > #include <linux/module.h>
> > #include <linux/string_helpers.h>
> >
> > +static const char byte_units[] = "_KMGTPEZY";
>
> u64 could be up to ~1.8**19 decimal
> zetta and yotta are not possible or necessary.
> u128 maybe someday, but then other changes
> would be necessary too.
Right, but we're only handling u64.
> > +static char *__units_str(enum string_size_units unit, char *buf, int index)
> > +{
> > + int place = 0;
> > +
> > + /* index=0 is plain 'B' with no other unit */
> > + if (index) {
> > + buf[place++] = byte_units[index];
>
> index is unbounded (doesn't matter currently, it will for u128)
It's bound by the division or the log2 at least. You do have to know
what you're passing in to __units_str, just like you had to know what
you were indexing with in to units_2[] and units_10[].
Is there something else you'd like to see done here? We can
bounds-check index, but that seems a bit unnecessary since it's static
and the two callers are visible on the same page of code.
> > @@ -23,26 +40,19 @@
> > int string_get_size(u64 size, const enum string_size_units units,
> > char *buf, int len)
> []
> > const unsigned int divisor[] = {
> > [STRING_UNITS_10] = 1000,
> > [STRING_UNITS_2] = 1024,
> > };
>
> static const or it might be better to use
> unsigned int divisor = (string_size_units == STRING_UNITS_2) ? 1024 : 1000;
> as that would make the code clearer in a
> couple of uses of divisor[] later.
>
> > @@ -61,7 +71,7 @@ int string_get_size(u64 size, const enum
> > }
> >
> > snprintf(buf, len, "%lld%s %s", (unsigned long long)size,
>
> %llu
These two are about existing code, and not really necessary for this
set. They'd make good follow-on patches, though.
-- Dave
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>