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