| Message ID | 20141222143710.GA29096@krava.brq.redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Jiri Olsa [jolsa@redhat.com] wrote:
| On Sun, Dec 21, 2014 at 11:49:24PM -0800, Sukadev Bhattiprolu wrote:
|
| SNIP
|
| > + }
| >
| > switch (format->value) {
| > case PERF_PMU_FORMAT_VALUE_CONFIG:
| > @@ -592,11 +629,16 @@ static int pmu_config_term(struct list_head *formats,
| > }
| >
| > /*
| > - * XXX If we ever decide to go with string values for
| > - * non-hardcoded terms, here's the place to translate
| > - * them into value.
| > + * Either directly use a numeric term, or try to translate string terms
| > + * using event parameters.
| > */
| > - pmu_format_value(format->bits, term->val.num, vp, zero);
| > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
| > + val = term->val.num;
| > + else
| > + if (pmu_resolve_param_term(term, head_terms, &val))
| > + return -EINVAL;
| > +
|
| I'm ok with the change logic, but I'm missing here check for the 'term'
| string value to be '?', so we force subst terms to have '?' as value..
| I believe thats what we decided in the previous set discussion, right?
The =? is not a user input, so I did not think of validating that.
perf tool expects kernel/sysfs to show entries like 'core=?'. Are you
saying that we should error out if kernel mistakenly displays 'core=$val'
or 'core=?val' ?
If a required parameter is missing, we catch that in pmu_resolve_param_term().
If a bogus parameter is specified we catch that above in pmu_config_term().
|
| I guess the it'd be nice to parse it directly in the bison code like
| below (could be done later), but I'd be ok with simple check on this
| place for now.
|
| thanks,
| jirka
|
|
| ---
| diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
| index 93c4c9fbc922..7e021c64d5cc 100644
| --- a/tools/perf/util/parse-events.y
| +++ b/tools/perf/util/parse-events.y
| @@ -484,6 +484,14 @@ PE_TERM '=' PE_VALUE
| $$ = term;
| }
| |
| +PE_TERM '=' PE_SUBST
| +{
| + struct parse_events_term *term;
| +
| + ABORT_ON(parse_events_term__subst(&term, (int)$1, NULL, NULL));
| + $$ = term;
| +}
| +|
| PE_TERM
| {
| struct parse_events_term *term;
On Mon, Dec 22, 2014 at 11:30:45AM -0800, Sukadev Bhattiprolu wrote: > Jiri Olsa [jolsa@redhat.com] wrote: > | On Sun, Dec 21, 2014 at 11:49:24PM -0800, Sukadev Bhattiprolu wrote: > | > | SNIP > | > | > + } > | > > | > switch (format->value) { > | > case PERF_PMU_FORMAT_VALUE_CONFIG: > | > @@ -592,11 +629,16 @@ static int pmu_config_term(struct list_head *formats, > | > } > | > > | > /* > | > - * XXX If we ever decide to go with string values for > | > - * non-hardcoded terms, here's the place to translate > | > - * them into value. > | > + * Either directly use a numeric term, or try to translate string terms > | > + * using event parameters. > | > */ > | > - pmu_format_value(format->bits, term->val.num, vp, zero); > | > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) > | > + val = term->val.num; > | > + else > | > + if (pmu_resolve_param_term(term, head_terms, &val)) > | > + return -EINVAL; > | > + > | > | I'm ok with the change logic, but I'm missing here check for the 'term' > | string value to be '?', so we force subst terms to have '?' as value.. > | I believe thats what we decided in the previous set discussion, right? > > The =? is not a user input, so I did not think of validating that. > > perf tool expects kernel/sysfs to show entries like 'core=?'. Are you > saying that we should error out if kernel mistakenly displays 'core=$val' > or 'core=?val' ? I think the we should at least try to have interface unambiguous from the beginning > If a required parameter is missing, we catch that in pmu_resolve_param_term(). > If a bogus parameter is specified we catch that above in pmu_config_term(). but the value of that param is unspecified, and if we later want to add another type of string values we would be screwed.. as I described in the previous reply for your other patch. jirka
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index 93c4c9fbc922..7e021c64d5cc 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -484,6 +484,14 @@ PE_TERM '=' PE_VALUE $$ = term; } | +PE_TERM '=' PE_SUBST +{ + struct parse_events_term *term; + + ABORT_ON(parse_events_term__subst(&term, (int)$1, NULL, NULL)); + $$ = term; +} +| PE_TERM { struct parse_events_term *term;