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

[Changbin Du <changbin.du@gmail.com>]

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
> >

-- 
Cheers,
Changbin Du