Messages in this thread | | | Date | Sun, 15 Nov 2009 09:57:11 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH v2] perf tools: New function to parse string representing size in bytes |
| |
* Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp> wrote:
> This patch modifies util/string.[ch] to add new function: bytesexp2int() > to parse string representing size in bytes. > > This is version 2. Acording to Frederic and Ingo's advice, > I removed static function digit() and rewrote to use > isdigit() macro of util.h. > > Below is the description of bytesexp2int(). > > Parse (\d+)(b|B|kb|KB|mb|MB|gb|GB) (e.g. "256MB") > and return its numeric value. (e.g. 268435456) > > The parameter str is not changed before and after calling, > but it changed temporally and internally for atoi(). > So type of str is char *, not const char *. > > Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Frederic Weisbecker <fweisbec@gmail.com> > --- > tools/perf/util/string.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++ > tools/perf/util/string.h | 1 + > 2 files changed, 83 insertions(+), 0 deletions(-)
Please run checkpatch on patches in the future, for this patch it shows this small problem:
ERROR: trailing whitespace #68: FILE: tools/perf/util/string.c:66: +^I$
> diff --git a/tools/perf/util/string.c b/tools/perf/util/string.c > index 04743d3..1ee7b17 100644 > --- a/tools/perf/util/string.c > +++ b/tools/perf/util/string.c > @@ -1,4 +1,5 @@ > #include <string.h> > +#include <stdlib.h> > #include "string.h" > > static int hex(char ch) > @@ -43,3 +44,84 @@ char *strxfrchar(char *s, char from, char to) > > return s; > } > + > +#define K 1024 > +/* > + * bytesexp2int() > + * Parse (\d+)(b|B|kb|KB|mb|MB|gb|GB) (e.g. "256MB") > + * and return its numeric value > + * > + * The parameter str is not changed before and after calling, > + * but it changed temporally and internally for atoi(). > + * So type of str is char *, not const char *. > + */ > +int bytesexp2int(char *str)
why not to longlong?
> +{ > + int i, unit = 1; > + char shelter = '\0'; > + size_t length = -1; > + > + if (!isdigit(str[0])) > + goto err; > + > + for (i = 1; i < (int)strlen(str); i++) {
This extra '(int)' cast should be eliminated. In the kernel we avoid unnecessary casts - here i suspect it can be done by changing 'int i' to 'unsigned int i'.
> + switch (str[i]) { > + case 'B': > + case 'b': > + break; > + case 'K': > + if (str[i + 1] != 'B') > + goto err; > + else > + goto kilo; > + case 'k': > + if (str[i + 1] != 'b') > + goto err; > +kilo: > + unit = K; > + break; > + case 'M': > + if (str[i + 1] != 'B') > + goto err; > + else > + goto mega; > + case 'm': > + if (str[i + 1] != 'b') > + goto err; > +mega: > + unit = K * K; > + break; > + case 'G': > + if (str[i + 1] != 'B') > + goto err; > + else > + goto giga; > + case 'g': > + if (str[i + 1] != 'b') > + goto err; > +giga: > + unit = K * K * K; > + break; > + case '\0': /* only specified figures */ > + unit = 1; > + break; > + default: > + if (!isdigit(str[i])) > + goto err; > + break;
(Might want to add tera as well, just in case this gets librarized into the rest of the kernel some day.)
> + } > + } > + > + shelter = str[i]; > + str[i] = (char)'\0';
a spurious type cast again - they should really be avoided. Here it's unnecessary, character literals should auto-convert to the proper type just fine.
> + length = atoi(str) * unit; > + if (shelter != '\0') > + str[i] = shelter; > + > + goto end; > + > +err: > + length = -1; > +end: > + return length; > +}
small naming nit, please use 'out_err:' and 'out:' labels. (which is how we generally name such exception exit points)
Thanks,
Ingo
| |