by Changbin Du

[permalink] [raw]

Subject: Re: [PATCH v4 08/17] perf: util: add general function to parse sublevel options

On Fri, Jul 10, 2020 at 11:29:49PM +0900, Namhyung Kim wrote:
> On Fri, Jul 10, 2020 at 10:44 PM Changbin Du <[email protected]> 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 <[email protected]>
> >
> > ---
> > 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
> >

--
Cheers,
Changbin Du