Messages in this thread | | | Subject | Re: [RFC] perf/jevents: Add new structure to pass json fields. | From | kajoljain <> | Date | Thu, 27 Aug 2020 18:28:02 +0530 |
| |
On 8/26/20 5:29 PM, Jiri Olsa wrote: > On Wed, Aug 26, 2020 at 05:02:04PM +0530, kajoljain wrote: >> >> >> On 8/26/20 4:26 PM, Jiri Olsa wrote: >>> On Tue, Aug 25, 2020 at 01:10:41PM +0530, Kajol Jain wrote: >>> >>> SNIP >>> >>>> { >>>> /* try to find matching event from arch standard values */ >>>> struct event_struct *es; >>>> @@ -498,8 +486,7 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc, >>>> if (!strcmp(arch_std, es->name)) { >>>> if (!eventcode && es->event) { >>>> /* allow EventCode to be overridden */ >>>> - free(*event); >>>> - *event = NULL; >>>> + je->event = NULL; >>>> } >>>> FOR_ALL_EVENT_STRUCT_FIELDS(TRY_FIXUP_FIELD); >>>> return 0; >>>> @@ -513,13 +500,8 @@ try_fixup(const char *fn, char *arch_std, char **event, char **desc, >>>> >>>> /* Call func with each event in the json file */ >>>> int json_events(const char *fn, >>>> - int (*func)(void *data, char *name, char *event, char *desc, >>>> - char *long_desc, >>>> - char *pmu, char *unit, char *perpkg, >>>> - char *metric_expr, >>>> - char *metric_name, char *metric_group, >>>> - char *deprecated, char *metric_constraint), >>>> - void *data) >>>> + int (*func)(void *data, struct json_event *je), >>>> + void *data) >>>> { >>>> int err; >>>> size_t size; >>>> @@ -537,24 +519,16 @@ int json_events(const char *fn, >>>> EXPECT(tokens->type == JSMN_ARRAY, tokens, "expected top level array"); >>>> tok = tokens + 1; >>>> for (i = 0; i < tokens->size; i++) { >>>> - char *event = NULL, *desc = NULL, *name = NULL; >>>> - char *long_desc = NULL; >>>> char *extra_desc = NULL; >>>> - char *pmu = NULL; >>>> char *filter = NULL; >>>> - char *perpkg = NULL; >>>> - char *unit = NULL; >>>> - char *metric_expr = NULL; >>>> - char *metric_name = NULL; >>>> - char *metric_group = NULL; >>>> - char *deprecated = NULL; >>>> - char *metric_constraint = NULL; >>>> + struct json_event *je; >>>> char *arch_std = NULL; >>>> unsigned long long eventcode = 0; >>>> struct msrmap *msr = NULL; >>>> jsmntok_t *msrval = NULL; >>>> jsmntok_t *precise = NULL; >>>> jsmntok_t *obj = tok++; >>>> + je = (struct json_event *)calloc(1, sizeof(struct json_event)); >>> >>> hum, you don't check je pointer in here.. but does it need to be allocated? >>> looks like you could just have je on stack as well.. >> >> Hi Jiri, >> Yes I will add check for je pointer here.The reason for allocating memory to 'je' is, >> later we are actually referring one by one to its field and in case if won't allocate memory >> we will get segmentaion fault as otherwise je will be NULL. Please let me know if I am >> getting correct. > > I don't see reason why not to use automatic variable in here, > I tried and it seems to work.. below is diff to your changes, > feel free to squash it with your changes
Hi Jiri, Thanks for the changes, I will update.
Thanks, Kajol Jain > > jirka > > --- > diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c > index 606805af69fe..eaac5c126a52 100644 > --- a/tools/perf/pmu-events/jevents.c > +++ b/tools/perf/pmu-events/jevents.c > @@ -521,14 +521,13 @@ int json_events(const char *fn, > for (i = 0; i < tokens->size; i++) { > char *extra_desc = NULL; > char *filter = NULL; > - struct json_event *je; > + struct json_event je = {}; > char *arch_std = NULL; > unsigned long long eventcode = 0; > struct msrmap *msr = NULL; > jsmntok_t *msrval = NULL; > jsmntok_t *precise = NULL; > jsmntok_t *obj = tok++; > - je = (struct json_event *)calloc(1, sizeof(struct json_event)); > > EXPECT(obj->type == JSMN_OBJECT, obj, "expected object"); > for (j = 0; j < obj->size; j += 2) { > @@ -544,7 +543,7 @@ int json_events(const char *fn, > "Expected string value"); > > nz = !json_streq(map, val, "0"); > - if (match_field(map, field, nz, &je->event, val)) { > + if (match_field(map, field, nz, &je.event, val)) { > /* ok */ > } else if (json_streq(map, field, "EventCode")) { > char *code = NULL; > @@ -557,14 +556,14 @@ int json_events(const char *fn, > eventcode |= strtoul(code, NULL, 0) << 21; > free(code); > } else if (json_streq(map, field, "EventName")) { > - addfield(map, &je->name, "", "", val); > + addfield(map, &je.name, "", "", val); > } else if (json_streq(map, field, "BriefDescription")) { > - addfield(map, &je->desc, "", "", val); > - fixdesc(je->desc); > + addfield(map, &je.desc, "", "", val); > + fixdesc(je.desc); > } else if (json_streq(map, field, > "PublicDescription")) { > - addfield(map, &je->long_desc, "", "", val); > - fixdesc(je->long_desc); > + addfield(map, &je.long_desc, "", "", val); > + fixdesc(je.long_desc); > } else if (json_streq(map, field, "PEBS") && nz) { > precise = val; > } else if (json_streq(map, field, "MSRIndex") && nz) { > @@ -584,34 +583,34 @@ int json_events(const char *fn, > > ppmu = field_to_perf(unit_to_pmu, map, val); > if (ppmu) { > - je->pmu = strdup(ppmu); > + je.pmu = strdup(ppmu); > } else { > - if (!je->pmu) > - je->pmu = strdup("uncore_"); > - addfield(map, &je->pmu, "", "", val); > - for (s = je->pmu; *s; s++) > + if (!je.pmu) > + je.pmu = strdup("uncore_"); > + addfield(map, &je.pmu, "", "", val); > + for (s = je.pmu; *s; s++) > *s = tolower(*s); > } > - addfield(map, &je->desc, ". ", "Unit: ", NULL); > - addfield(map, &je->desc, "", je->pmu, NULL); > - addfield(map, &je->desc, "", " ", NULL); > + addfield(map, &je.desc, ". ", "Unit: ", NULL); > + addfield(map, &je.desc, "", je.pmu, NULL); > + addfield(map, &je.desc, "", " ", NULL); > } else if (json_streq(map, field, "Filter")) { > addfield(map, &filter, "", "", val); > } else if (json_streq(map, field, "ScaleUnit")) { > - addfield(map, &je->unit, "", "", val); > + addfield(map, &je.unit, "", "", val); > } else if (json_streq(map, field, "PerPkg")) { > - addfield(map, &je->perpkg, "", "", val); > + addfield(map, &je.perpkg, "", "", val); > } else if (json_streq(map, field, "Deprecated")) { > - addfield(map, &je->deprecated, "", "", val); > + addfield(map, &je.deprecated, "", "", val); > } else if (json_streq(map, field, "MetricName")) { > - addfield(map, &je->metric_name, "", "", val); > + addfield(map, &je.metric_name, "", "", val); > } else if (json_streq(map, field, "MetricGroup")) { > - addfield(map, &je->metric_group, "", "", val); > + addfield(map, &je.metric_group, "", "", val); > } else if (json_streq(map, field, "MetricConstraint")) { > - addfield(map, &je->metric_constraint, "", "", val); > + addfield(map, &je.metric_constraint, "", "", val); > } else if (json_streq(map, field, "MetricExpr")) { > - addfield(map, &je->metric_expr, "", "", val); > - for (s = je->metric_expr; *s; s++) > + addfield(map, &je.metric_expr, "", "", val); > + for (s = je.metric_expr; *s; s++) > *s = tolower(*s); > } else if (json_streq(map, field, "ArchStdEvent")) { > addfield(map, &arch_std, "", "", val); > @@ -620,7 +619,7 @@ int json_events(const char *fn, > } > /* ignore unknown fields */ > } > - if (precise && je->desc && !strstr(je->desc, "(Precise Event)")) { > + if (precise && je.desc && !strstr(je.desc, "(Precise Event)")) { > if (json_streq(map, precise, "2")) > addfield(map, &extra_desc, " ", > "(Must be precise)", NULL); > @@ -629,34 +628,33 @@ int json_events(const char *fn, > "(Precise event)", NULL); > } > snprintf(buf, sizeof buf, "event=%#llx", eventcode); > - addfield(map, &je->event, ",", buf, NULL); > - if (je->desc && extra_desc) > - addfield(map, &je->desc, " ", extra_desc, NULL); > - if (je->long_desc && extra_desc) > - addfield(map, &je->long_desc, " ", extra_desc, NULL); > + addfield(map, &je.event, ",", buf, NULL); > + if (je.desc && extra_desc) > + addfield(map, &je.desc, " ", extra_desc, NULL); > + if (je.long_desc && extra_desc) > + addfield(map, &je.long_desc, " ", extra_desc, NULL); > if (filter) > - addfield(map, &je->event, ",", filter, NULL); > + addfield(map, &je.event, ",", filter, NULL); > if (msr != NULL) > - addfield(map, &je->event, ",", msr->pname, msrval); > - if (je->name) > - fixname(je->name); > + addfield(map, &je.event, ",", msr->pname, msrval); > + if (je.name) > + fixname(je.name); > > if (arch_std) { > /* > * An arch standard event is referenced, so try to > * fixup any unassigned values. > */ > - err = try_fixup(fn, arch_std, eventcode, je); > + err = try_fixup(fn, arch_std, eventcode, &je); > if (err) > goto free_strings; > } > - je->event = real_event(je->name, je->event); > - err = func(data, je); > + je.event = real_event(je.name, je.event); > + err = func(data, &je); > free_strings: > free(extra_desc); > free(filter); > free(arch_std); > - free(je); > > if (err) > break; >
| |