[v4,08/17] perf: util: add general function to parse sublevel options
diff mbox series

Message ID 20200710134322.15400-9-changbin.du@gmail.com
State New
Headers show
Series
  • perf: ftrace enhancement
Related show

Commit Message

Changbin Du July 10, 2020, 1:43 p.m. UTC
This factors out a general function perf_parse_sublevel_options() to parse
sublevel options. The 'sublevel' options is something like the '--debug'
options which allow more sublevel options.

Signed-off-by: Changbin Du <changbin.du@gmail.com>

---
v2: add util/parse-sublevel-options.c
---
 tools/perf/util/Build                    |  1 +
 tools/perf/util/debug.c                  | 61 +++++++----------------
 tools/perf/util/parse-sublevel-options.c | 63 ++++++++++++++++++++++++
 tools/perf/util/parse-sublevel-options.h | 11 +++++
 4 files changed, 92 insertions(+), 44 deletions(-)
 create mode 100644 tools/perf/util/parse-sublevel-options.c
 create mode 100644 tools/perf/util/parse-sublevel-options.h

Comments

Namhyung Kim July 10, 2020, 2:29 p.m. UTC | #1
On Fri, Jul 10, 2020 at 10:44 PM Changbin Du <changbin.du@gmail.com> wrote:
>
> This factors out a general function perf_parse_sublevel_options() to parse
> sublevel options. The 'sublevel' options is something like the '--debug'
> options which allow more sublevel options.
>
> Signed-off-by: Changbin Du <changbin.du@gmail.com>
>
> ---
> v2: add util/parse-sublevel-options.c
> ---
[snip]
> diff --git a/tools/perf/util/parse-sublevel-options.c b/tools/perf/util/parse-sublevel-options.c
> new file mode 100644
> index 000000000000..39798568547c
> --- /dev/null
> +++ b/tools/perf/util/parse-sublevel-options.c
> @@ -0,0 +1,63 @@
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "util/debug.h"
> +#include "util/parse-sublevel-options.h"
> +
> +static int parse_one_sublevel_option(const char *str,
> +                                    struct sublevel_option *opts)
> +{
> +       struct sublevel_option *opt = &opts[0];
> +       char *vstr, *s = strdup(str);
> +       int v = 1;

I know you just copied the code, but let's add a check for
the return value of strdup().

Also I think you can just use the opts pointer..


> +
> +       vstr = strchr(s, '=');
> +       if (vstr)
> +               *vstr++ = 0;
> +
> +       while (opt->name) {
> +               if (!strcmp(s, opt->name))
> +                       break;
> +               opt++;
> +       }
> +
> +       if (!opt->name) {
> +               pr_err("Unknown option name '%s'\n", s);
> +               free(s);
> +               return -1;
> +       }
> +
> +       if (vstr)
> +               v = atoi(vstr);
> +
> +       *opt->value_ptr = v;
> +       free(s);
> +       return 0;
> +}
> +
> +/* parse options like --foo a=<n>,b,c... */
> +int perf_parse_sublevel_options(const char *str, struct sublevel_option *opts)
> +{
> +       char *s = strdup(str);
> +       char *p = NULL;
> +       int ret;
> +
> +       if (!s)
> +               return -1;
> +
> +       p = strtok(s, ",");
> +       while (p) {
> +               ret = parse_one_sublevel_option(p, opts);
> +               if (ret) {
> +                       free(s);
> +                       return ret;
> +               }
> +
> +               p = strtok(NULL, ",");
> +       }
> +
> +       free(s);
> +       return 0;
> +}
> diff --git a/tools/perf/util/parse-sublevel-options.h b/tools/perf/util/parse-sublevel-options.h
> new file mode 100644
> index 000000000000..9b9efcc2aaad
> --- /dev/null
> +++ b/tools/perf/util/parse-sublevel-options.h
> @@ -0,0 +1,11 @@
> +#ifndef _PERF_PARSE_SUBLEVEL_OPTIONS_H
> +#define _PERF_PARSE_SUBLEVEL_OPTIONS_H
> +
> +struct sublevel_option {
> +       const char *name;
> +       int *value_ptr;
> +};
> +
> +int perf_parse_sublevel_options(const char *str, struct sublevel_option *opts);
> +
> +#endif
> \ No newline at end of file

Please add a newline at the end.

Thanks
Namhyung


> --
> 2.25.1
>
Changbin Du July 11, 2020, 12:37 p.m. UTC | #2
On Fri, Jul 10, 2020 at 11:29:49PM +0900, Namhyung Kim wrote:
> On Fri, Jul 10, 2020 at 10:44 PM Changbin Du <changbin.du@gmail.com> wrote:
> >
> > This factors out a general function perf_parse_sublevel_options() to parse
> > sublevel options. The 'sublevel' options is something like the '--debug'
> > options which allow more sublevel options.
> >
> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
> >
> > ---
> > v2: add util/parse-sublevel-options.c
> > ---
> [snip]
> > diff --git a/tools/perf/util/parse-sublevel-options.c b/tools/perf/util/parse-sublevel-options.c
> > new file mode 100644
> > index 000000000000..39798568547c
> > --- /dev/null
> > +++ b/tools/perf/util/parse-sublevel-options.c
> > @@ -0,0 +1,63 @@
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <stdio.h>
> > +
> > +#include "util/debug.h"
> > +#include "util/parse-sublevel-options.h"
> > +
> > +static int parse_one_sublevel_option(const char *str,
> > +                                    struct sublevel_option *opts)
> > +{
> > +       struct sublevel_option *opt = &opts[0];
> > +       char *vstr, *s = strdup(str);
> > +       int v = 1;
> 
> I know you just copied the code, but let's add a check for
> the return value of strdup().
>
yes, the 's' should be checked here.

> Also I think you can just use the opts pointer..
> 
For this, personally I prefer to opt as it's singular form. Because we are
dealing with single element.

> 
> > +
> > +       vstr = strchr(s, '=');
> > +       if (vstr)
> > +               *vstr++ = 0;
> > +
> > +       while (opt->name) {
> > +               if (!strcmp(s, opt->name))
> > +                       break;
> > +               opt++;
> > +       }
> > +
> > +       if (!opt->name) {
> > +               pr_err("Unknown option name '%s'\n", s);
> > +               free(s);
> > +               return -1;
> > +       }
> > +
> > +       if (vstr)
> > +               v = atoi(vstr);
> > +
> > +       *opt->value_ptr = v;
> > +       free(s);
> > +       return 0;
> > +}
> > +
> > +/* parse options like --foo a=<n>,b,c... */
> > +int perf_parse_sublevel_options(const char *str, struct sublevel_option *opts)
> > +{
> > +       char *s = strdup(str);
> > +       char *p = NULL;
> > +       int ret;
> > +
> > +       if (!s)
> > +               return -1;
> > +
> > +       p = strtok(s, ",");
> > +       while (p) {
> > +               ret = parse_one_sublevel_option(p, opts);
> > +               if (ret) {
> > +                       free(s);
> > +                       return ret;
> > +               }
> > +
> > +               p = strtok(NULL, ",");
> > +       }
> > +
> > +       free(s);
> > +       return 0;
> > +}
> > diff --git a/tools/perf/util/parse-sublevel-options.h b/tools/perf/util/parse-sublevel-options.h
> > new file mode 100644
> > index 000000000000..9b9efcc2aaad
> > --- /dev/null
> > +++ b/tools/perf/util/parse-sublevel-options.h
> > @@ -0,0 +1,11 @@
> > +#ifndef _PERF_PARSE_SUBLEVEL_OPTIONS_H
> > +#define _PERF_PARSE_SUBLEVEL_OPTIONS_H
> > +
> > +struct sublevel_option {
> > +       const char *name;
> > +       int *value_ptr;
> > +};
> > +
> > +int perf_parse_sublevel_options(const char *str, struct sublevel_option *opts);
> > +
> > +#endif
> > \ No newline at end of file
> 
> Please add a newline at the end.
> 
> Thanks
> Namhyung
> 
> 
> > --
> > 2.25.1
> >

Patch
diff mbox series

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 8d18380ecd10..e86607ada0b5 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -117,6 +117,7 @@  endif
 perf-y += parse-branch-options.o
 perf-y += dump-insn.o
 perf-y += parse-regs-options.o
+perf-y += parse-sublevel-options.o
 perf-y += term.o
 perf-y += help-unknown-cmd.o
 perf-y += mem-events.o
diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index adb656745ecc..5cda5565777a 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -20,6 +20,7 @@ 
 #include "target.h"
 #include "ui/helpline.h"
 #include "ui/ui.h"
+#include "util/parse-sublevel-options.h"
 
 #include <linux/ctype.h>
 
@@ -173,65 +174,37 @@  void trace_event(union perf_event *event)
 		     trace_event_printer, event);
 }
 
-static struct debug_variable {
-	const char *name;
-	int *ptr;
-} debug_variables[] = {
-	{ .name = "verbose",		.ptr = &verbose },
-	{ .name = "ordered-events",	.ptr = &debug_ordered_events},
-	{ .name = "stderr",		.ptr = &redirect_to_stderr},
-	{ .name = "data-convert",	.ptr = &debug_data_convert },
-	{ .name = "perf-event-open",	.ptr = &debug_peo_args },
+static struct sublevel_option debug_opts[] = {
+	{ .name = "verbose",		.value_ptr = &verbose },
+	{ .name = "ordered-events",	.value_ptr = &debug_ordered_events},
+	{ .name = "stderr",		.value_ptr = &redirect_to_stderr},
+	{ .name = "data-convert",	.value_ptr = &debug_data_convert },
+	{ .name = "perf-event-open",	.value_ptr = &debug_peo_args },
 	{ .name = NULL, }
 };
 
 int perf_debug_option(const char *str)
 {
-	struct debug_variable *var = &debug_variables[0];
-	char *vstr, *s = strdup(str);
-	int v = 1;
-
-	vstr = strchr(s, '=');
-	if (vstr)
-		*vstr++ = 0;
-
-	while (var->name) {
-		if (!strcmp(s, var->name))
-			break;
-		var++;
-	}
-
-	if (!var->name) {
-		pr_err("Unknown debug variable name '%s'\n", s);
-		free(s);
-		return -1;
-	}
+	int ret;
 
-	if (vstr) {
-		v = atoi(vstr);
-		/*
-		 * Allow only values in range (0, 10),
-		 * otherwise set 0.
-		 */
-		v = (v < 0) || (v > 10) ? 0 : v;
-	}
+	ret = perf_parse_sublevel_options(str, debug_opts);
+	if (ret)
+		return ret;
 
-	if (quiet)
-		v = -1;
+	/* Allow only verbose value in range (0, 10), otherwise set 0. */
+	verbose = (verbose < 0) || (verbose > 10) ? 0 : verbose;
 
-	*var->ptr = v;
-	free(s);
 	return 0;
 }
 
 int perf_quiet_option(void)
 {
-	struct debug_variable *var = &debug_variables[0];
+	struct sublevel_option *opt = &debug_opts[0];
 
 	/* disable all debug messages */
-	while (var->name) {
-		*var->ptr = -1;
-		var++;
+	while (opt->name) {
+		*opt->value_ptr = -1;
+		opt++;
 	}
 
 	return 0;
diff --git a/tools/perf/util/parse-sublevel-options.c b/tools/perf/util/parse-sublevel-options.c
new file mode 100644
index 000000000000..39798568547c
--- /dev/null
+++ b/tools/perf/util/parse-sublevel-options.c
@@ -0,0 +1,63 @@ 
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "util/debug.h"
+#include "util/parse-sublevel-options.h"
+
+static int parse_one_sublevel_option(const char *str,
+				     struct sublevel_option *opts)
+{
+	struct sublevel_option *opt = &opts[0];
+	char *vstr, *s = strdup(str);
+	int v = 1;
+
+	vstr = strchr(s, '=');
+	if (vstr)
+		*vstr++ = 0;
+
+	while (opt->name) {
+		if (!strcmp(s, opt->name))
+			break;
+		opt++;
+	}
+
+	if (!opt->name) {
+		pr_err("Unknown option name '%s'\n", s);
+		free(s);
+		return -1;
+	}
+
+	if (vstr)
+		v = atoi(vstr);
+
+	*opt->value_ptr = v;
+	free(s);
+	return 0;
+}
+
+/* parse options like --foo a=<n>,b,c... */
+int perf_parse_sublevel_options(const char *str, struct sublevel_option *opts)
+{
+	char *s = strdup(str);
+	char *p = NULL;
+	int ret;
+
+	if (!s)
+		return -1;
+
+	p = strtok(s, ",");
+	while (p) {
+		ret = parse_one_sublevel_option(p, opts);
+		if (ret) {
+			free(s);
+			return ret;
+		}
+
+		p = strtok(NULL, ",");
+	}
+
+	free(s);
+	return 0;
+}
diff --git a/tools/perf/util/parse-sublevel-options.h b/tools/perf/util/parse-sublevel-options.h
new file mode 100644
index 000000000000..9b9efcc2aaad
--- /dev/null
+++ b/tools/perf/util/parse-sublevel-options.h
@@ -0,0 +1,11 @@ 
+#ifndef _PERF_PARSE_SUBLEVEL_OPTIONS_H
+#define _PERF_PARSE_SUBLEVEL_OPTIONS_H
+
+struct sublevel_option {
+	const char *name;
+	int *value_ptr;
+};
+
+int perf_parse_sublevel_options(const char *str, struct sublevel_option *opts);
+
+#endif
\ No newline at end of file