Re: [RFC] perf/jevents: Add new structure to pass json fields.

[John Garry]

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.

                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