Thanks! This is part of the v5 patch set, specifically: https://lkml.org/lkml/2019/10/30/1001 Let me know if there's anything else blocking this. Thanks, Ian On Mon, Oct 28, 2019 at 2:36 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote: > > On Mon, Oct 28, 2019 at 02:06:24PM -0700, Ian Rogers wrote: > > On Mon, Oct 28, 2019 at 12:32 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote: > > > > > > On Fri, Oct 25, 2019 at 08:14:36AM -0700, Ian Rogers wrote: > > > > On Fri, Oct 25, 2019 at 12:58 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote: > > > > > > > > > > On Thu, Oct 24, 2019 at 12:01:54PM -0700, Ian Rogers wrote: > > > > > > Parse event error handling may overwrite one error string with another > > > > > > creating memory leaks and masking errors. Introduce a helper routine > > > > > > that appends error messages and avoids the memory leak. > > > > > > > > > > > > A reproduction of this problem can be seen with: > > > > > > perf stat -e c/c/ > > > > > > After this change this produces: > > > > > > event syntax error: 'c/c/' > > > > > > \___ unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: unknown term (previous error: Cannot find PMU `c'. Missing kernel support?)(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,pc,in_tx,edge,any,offcore_rsp,in_tx_cp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore))(help: valid terms: event,filter_rem,filter_opc0,edge,filter_isoc,filter_tid,filter_loc,filter_nc,inv,umask,filter_opc1,tid_en,thresh,filter_all_op,filter_not_nm,filter_state,filter_nm,config,config1,config2,name,period,percore)) > > > > > > > > > > > > > > > hum... I'd argue that the previous state was better: > > > > > > > > > > [jolsa@krava perf]$ ./perf stat -e c/c/ > > > > > event syntax error: 'c/c/' > > > > > \___ unknown term > > > > > > > > > > > > > > > jirka > > > > > > > > I am agnostic. We can either have the previous state or the new state, > > > > I'm keen to resolve the memory leak. Another alternative is to warn > > > > that multiple errors have occurred before dropping or printing the > > > > previous error. As the code is shared in memory places the approach > > > > taken here was to try to not conceal anything that could potentially > > > > be useful. Given this, is the preference to keep the status quo > > > > without any warning? > > > > > > if the other alternative is string above, yes.. but perhaps > > > keeping just the first error would be the best way? > > > > > > here it seems to be the: > > > "Cannot find PMU `c'. Missing kernel support?)(help: valid..." > > > > I think this is a reasonable idea. I'd propose doing it as an > > additional patch, the purpose of this patch is to avoid a possible > > memory leak. I can write the patch and base it on this series. > > To resolve the issue, I'd add an extra first error to the struct > > parse_events_error. All callers would need to be responsible for > > cleaning this up when present, which is why I'd rather not make it > > part of this patch. > > Does this sound reasonable? > > yep, sounds good > > jirka >
