Messages in this thread | | | Subject | Re: [RFC] perf/jevents: Add new structure to pass json fields. | From | kajoljain <> | Date | Thu, 27 Aug 2020 18:27:17 +0530 |
| |
On 8/26/20 5:03 PM, John Garry wrote: > On 26/08/2020 12:24, kajoljain wrote: >> >> >> On 8/26/20 4:30 PM, Jiri Olsa wrote: >>> On Tue, Aug 25, 2020 at 09:14:11AM +0100, John Garry wrote: >>> >>> SNIP >>> >>>>> goto free_strings; >>>>> } >>>>> - err = func(data, name, real_event(name, event), desc, long_desc, >>>>> - pmu, unit, perpkg, metric_expr, metric_name, >>>>> - metric_group, deprecated, metric_constraint); >>>>> + je->event = real_event(je->name, je->event); >>>>> + err = func(data, je); >>>>> free_strings: >>>>> - free(event); >>>>> - free(desc); >>>>> - free(name); >>>>> - free(long_desc); >>>>> free(extra_desc); >>>>> - free(pmu); >>>>> free(filter); >>>>> - free(perpkg); >>>>> - free(deprecated); >>>>> - free(unit); >>>>> - free(metric_expr); >>>>> - free(metric_name); >>>>> - free(metric_group); >>>>> - free(metric_constraint); > > Hi Kajol Jain, > > Do we need to free je->metric_name and the rest still? From a glance, that memory is still separately alloc'ed in addfield.
Hi John, yes right we should free them as well. Thanks for pointing it, I will update.
Thanks, Kajol Jain > >>>>> free(arch_std); >>>>> + free(je); >>>>> if (err) >>>>> break; >>>>> diff --git a/tools/perf/pmu-events/jevents.h b/tools/perf/pmu-events/jevents.h >>>>> index 2afc8304529e..e696edf70e9a 100644 >>>>> --- a/tools/perf/pmu-events/jevents.h >>>>> +++ b/tools/perf/pmu-events/jevents.h >>>> >>>> Somewhat unrelated - this file only seems to be included in jevents.c, so I >>>> don't see why it exists... >>> >>> ah right.. I won't mind getting rid of it >> >> Hi John and Jiri >> Thanks for reviewing the patch. I can remove this file and add these structure inside jevents.c > > thanks > >> >> Thanks, >> Kajol Jain >>> >>>>> @@ -2,14 +2,28 @@ >>>>> #ifndef JEVENTS_H >>>>> #define JEVENTS_H 1 >>>>> +#include "pmu-events.h" >>>>> + >>>>> +struct json_event { >>>>> + char *name; >>>>> + char *event; >>>>> + char *desc; >>>>> + char *topic; >>>>> + char *long_desc; >>>>> + char *pmu; >>>>> + char *unit; >>>>> + char *perpkg; >>>>> + char *metric_expr; >>>>> + char *metric_name; >>>>> + char *metric_group; >>>>> + char *deprecated; >>>>> + char *metric_constraint; >>>> >>>> This looks very much like struct event_struct, so could look to consolidate: >>>> >>>> struct event_struct { >>>> struct list_head list; >>>> 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; >>>> }; >>> >>> as Andi said they come from different layers, I think it's >>> better to keep them separated even if they share some fields > > I was just suggesting to make: > struct event_struct { > struct list_head list; > struct json_event je; > } > > No biggie if against this. > > Cheers, > John
| |