lkml.org 
[lkml]   [2011]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFCv3][PATCH 1/4] replace string_get_size() arrays
From
Date
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



\
 
 \ /
  Last update: 2011-10-04 21:37    [W:0.049 / U:8.980 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site