Messages in this thread | | | Date | Sun, 15 Nov 2009 18:52:20 +0900 (JST) | Subject | Re: [PATCH v2] perf tools: New function to parse string representing size in bytes | From | Hitoshi Mitake <> |
| |
From: Ingo Molnar <mingo@elte.hu> Subject: Re: [PATCH v2] perf tools: New function to parse string representing size in bytes Date: Sun, 15 Nov 2009 09:57:11 +0100
Thanks for your review, and sorry for my lots of fault... I'll send version 3 later.
> * 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 >
| |