Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 10 Jan 2020 16:04:10 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     Leo Yan <leo.yan@linaro.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andi Kleen <ak@linux.intel.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mike Leach <mike.leach@linaro.org>
Subject: Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
Message-ID: <20200110150410.GG82989@krava>
References: <20200108142010.11269-1-leo.yan@linaro.org>
 <CANLsYkzv2Di-qeU1Q3M4Ro21hQ09eE26FBjeP1A9uSsA_W2Uww@mail.gmail.com>
 <20200109050753.GA24741@leoy-ThinkPad-X240s>
 <20200109163424.GA5721@xps15>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200109163424.GA5721@xps15>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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