From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751531AbdGZQro (ORCPT ); Wed, 26 Jul 2017 12:47:44 -0400 Received: from mga01.intel.com ([192.55.52.88]:50038 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750781AbdGZQrn (ORCPT ); Wed, 26 Jul 2017 12:47:43 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,416,1496127600"; d="scan'208";a="291752847" Message-ID: <1501087661.3214.20.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: [PATCH 29/32] tracing: Add 'last error' error facility for hist triggers From: Tom Zanussi To: Namhyung Kim Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org, vedang.patel@intel.com, linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org, kernel-team@lge.com Date: Wed, 26 Jul 2017 11:47:41 -0500 In-Reply-To: <20170726043902.GC32043@sejong> References: <20170726043902.GC32043@sejong> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Namhyung, On Wed, 2017-07-26 at 13:39 +0900, Namhyung Kim wrote: > On Mon, Jun 26, 2017 at 05:49:30PM -0500, Tom Zanussi wrote: > > With the addition of variables and actions, it's become necessary to > > provide more detailed error information to users about syntax errors. > > > > Add a 'last error' facility accessible via the erroring event's 'hist' > > file. Reading the hist file after an error will display more detailed > > information about what went wrong, if information is available. This > > extended error information will be available until the next hist > > trigger command for that event. > > > > # echo xxx > /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger > > echo: write error: Invalid argument > > > > # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/hist > > > > ERROR: Couldn't yyy: zzz > > Last command: xxx > > > > Also add specific error messages for variable and action errors. > > > > Signed-off-by: Tom Zanussi > > --- > > Documentation/trace/events.txt | 19 ++++ > > kernel/trace/trace_events_hist.c | 181 ++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 188 insertions(+), 12 deletions(-) > > > > diff --git a/Documentation/trace/events.txt b/Documentation/trace/events.txt > > index 9717688..f271d87 100644 > > --- a/Documentation/trace/events.txt > > +++ b/Documentation/trace/events.txt > > @@ -686,6 +686,25 @@ The following commands are supported: > > interpreted as microseconds. > > cpu int - the cpu on which the event occurred. > > > > + Extended error information > > + -------------------------- > > + > > + For some error conditions encountered when invoking a hist trigger > > + command, extended error information is available via the > > + corresponding event's 'hist' file. Reading the hist file after an > > + error will display more detailed information about what went wrong, > > + if information is available. This extended error information will > > + be available until the next hist trigger command for that event. > > + > > + If available for a given error condition, the extended error > > + information and usage takes the following form: > > + > > + # echo xxx > /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger > > + echo: write error: Invalid argument > > + > > + # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/hist > > + ERROR: Couldn't yyy: zzz > > + Last command: xxx > > > > 6.2 'hist' trigger examples > > --------------------------- > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > index 799b95e..06049f8 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -288,6 +288,7 @@ struct hist_trigger_data { > > struct field_var *max_vars[SYNTH_FIELDS_MAX]; > > unsigned int n_max_vars; > > unsigned int n_max_var_str; > > + char *last_err; > > I couldn't find where it's used. > It's not, will remove. > > }; > > > > struct synth_field { > > @@ -332,6 +333,83 @@ struct action_data { > > struct hist_field *onmax_var; > > }; > > > > + > > +static char *hist_err_str; > > +static char *last_hist_cmd; > > + > > +static int hist_err_alloc(void) > > +{ > > + int ret = 0; > > + > > + last_hist_cmd = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL); > > + hist_err_str = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL); > > + if (!last_hist_cmd || !hist_err_str) > > + ret = -ENOMEM; > > This makes me uncomfortable.. > > > + > > + return ret; > > +} > > + > > +static void last_cmd_set(char *str) > > +{ > > + if (!last_hist_cmd || !str) > > + return; > > + > > + if (strlen(last_hist_cmd) > MAX_FILTER_STR_VAL - 1) > > + return; > > Hmm.. why does it check 'last_hist_cmd' rather than 'str'? > > > + > > + strcpy(last_hist_cmd, str); > > +} > > + > > +static void hist_err(char *str, char *var) > > +{ > > + int maxlen = MAX_FILTER_STR_VAL - 1; > > + > > + if (strlen(hist_err_str)) > > + return; > > Shouldn't it move to after the NULL check below? > Yes, will fix this and the others above. Thanks, Tom