Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term

Jiri Olsa
2020-01-10T15:04:27Z

On Thu, Jan 09, 2020 at 09:34:24AM -0700, Mathieu Poirier wrote:

SNIP

>
> If we are to deal with all flields of the union, I think it should be as below:
>
> union {
> bool cfg_bool;
> int cfg_int;
> unsigned long cfg_ulong;
> u32 cfg_u32;
> char *cfg_str;
> } val;
>
> But just dealing with the "char *" as below would also be fine with me:
>
> union {
> u64 period;
> u64 freq;
> bool time;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> u64 num;
> char *str;
> } val;
>
> >
> > struct perf_evsel_config_term {
> > struct list_head list;
> > enum evsel_term_type type;
> > union {
> > u64 period;
> > u64 freq;
> > bool time;
> > char *callgraph;
> > char *drv_cfg;
> > u64 stack_user;
> > int max_stack;
> > bool inherit;
> > bool overwrite;
> > char *branch;
> > unsigned long max_events;
> > bool percore;
> > bool aux_output;
> > u32 aux_sample_size;
> > u64 cfg_chg;
> > + u64 num;
> > + char *str;
> > } val;
> > bool weak;
> > };
> >
> > > I will let Jiri make the
> > > final call but if we are to proceed this way I think we should have a
> > > member per type to avoid casting issues.
> >
> > Yeah, let's see what's Jiri thinking.
> >
> > Just note, with this change, I don't see any casting warning or errors
> > when built perf on arm64/arm32.
>
> At this time you may not, but they will happen and it will be very hard to
> debug.

hi,
sry for late reply..

I think ;-) we should either add all different types to the union
or just add 'str' pointer to handle strings correctly.. which seems
better, because it's less changes and there's no real issue that
would need that other bigger change

jirka