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

[John Garry]

On 25/08/2020 08:40, Kajol Jain wrote:
> This patch adds new structure called 'json_event' inside jevents.h
> file to improve the callback prototype inside jevent files.
> Initially, whenever user want to add new field, they need to update
> in all function callback which make it more and more complex with
> increased number of parmeters.
> With this change, we just need to add it in new structure 'json_event'.
>
>
> Signed-off-by: Kajol Jain <kjain@xxxxxxxxxxxxx>
> ---
>   tools/perf/pmu-events/jevents.c | 188 +++++++++++++-------------------
>   tools/perf/pmu-events/jevents.h |  28 +++--
>   2 files changed, 94 insertions(+), 122 deletions(-)
>
> This is the initial prototype to include structure for passing
> json fileds. Please, Let me know if this is the right direction
> to go forward.
> This patch doen't include all the changes, like percore/perchip
> field addition. If this looks fine I can send new patch set with
> those changes.
>
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index fa86c5f997cc..606805af69fe 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -318,12 +318,7 @@ static void print_events_table_prefix(FILE *fp, const char *tblname)
>   	close_table = 1;
>   }
>   
> -static int print_events_table_entry(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)

Right, too many paramaetrs of the same type, so looks reasonable to pass 
a struct

[...]

>   		if (arch_std) {
>   			/*
>   			 * An arch standard event is referenced, so try to
>   			 * fixup any unassigned values.
>   			 */
> -			err = try_fixup(fn, arch_std, &event, &desc, &name,
> -					&long_desc, &pmu, &filter, &perpkg,
> -					&unit, &metric_expr, &metric_name,
> -					&metric_group, eventcode,
> -					&deprecated, &metric_constraint);
> +			err = try_fixup(fn, arch_std, eventcode, je);
>   			if (err)
>   				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);
>   		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...

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

> +};
> +
>   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);
> +
>   char *get_cpu_str(void);
>   
>   #ifndef min