Re: [PATCH 1/2] trace-cmd: remove parsing_failures APIs from libtraceevent

[Steven Rostedt <rostedt@goodmis.org>]

Hi Tzvetomir,

A couple of nits.

In the subject line, we follow Linux standards (well, Linus's
preferences), which is to capitalize the first letter in the subject
description:

  trace-cmd: Remove parsing_failures APIs from libtraceevent


On Mon, 15 Apr 2019 15:58:22 +0300
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

> The application, which uses libtraceevent, should track itself the number of
> failures while parsing the event files. The libtraceevent APIs return error
> in case of such failure. The application can check for it and track the
> parcing failures, if needed.

 "parsing"

> The patch changes:

Also, the kernel community doesn't care for mentioning the patch in the
change log. As they feel its redundant. They prefer something like:

parsing failures, if needed.
  The following is performed:

Some more nits below.

>  - traceecent library: remove the parsing failures APIs and logic.
>  - trace-cmd library:
> 	added new parameter "int *parsing_failures" to tracecmd_fill_local_events().
> 	implemented new API tracecmd_read_headers_failures(), which returns
> 		the number of failures while parsing the event files.
>  - trace-cmd application:
> 	use the new trace-cmd library APIs to track the number of parsing failures.
> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
>  include/trace-cmd/trace-cmd.h      |  5 +-
>  include/traceevent/event-parse.h   |  2 -
>  lib/trace-cmd/trace-input.c        | 76 +++++++++++++++++++++---------
>  lib/trace-cmd/trace-util.c         | 16 ++++---
>  lib/traceevent/event-parse-api.c   | 27 -----------
>  lib/traceevent/event-parse-local.h |  2 -
>  tracecmd/trace-check-events.c      |  6 +--
>  tracecmd/trace-read.c              |  7 +--
>  8 files changed, 73 insertions(+), 68 deletions(-)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index ca4452b..c0eb7e1 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -32,7 +32,8 @@ void tracecmd_unload_plugins(struct tep_plugin_list *list, struct tep_handle *pe
>  char **tracecmd_event_systems(const char *tracing_dir);
>  char **tracecmd_system_events(const char *tracing_dir, const char *system);
>  struct tep_handle *tracecmd_local_events(const char *tracing_dir);
> -int tracecmd_fill_local_events(const char *tracing_dir, struct tep_handle *pevent);
> +int tracecmd_fill_local_events(const char *tracing_dir,
> +			       struct tep_handle *pevent, int *parsing_failures);
>  char **tracecmd_local_plugins(const char *tracing_dir);
>  
>  char **tracecmd_add_list(char **list, const char *name, int len);
> @@ -107,6 +108,8 @@ struct tracecmd_input *tracecmd_open_fd(int fd);
>  void tracecmd_ref(struct tracecmd_input *handle);
>  void tracecmd_close(struct tracecmd_input *handle);
>  int tracecmd_read_headers(struct tracecmd_input *handle);
> +int tracecmd_read_headers_failures(struct tracecmd_input *handle,
> +				   int *parsing_failures);
>  int tracecmd_long_size(struct tracecmd_input *handle);
>  int tracecmd_page_size(struct tracecmd_input *handle);
>  int tracecmd_cpus(struct tracecmd_input *handle);
> diff --git a/include/traceevent/event-parse.h b/include/traceevent/event-parse.h
> index bded403..5e0fd19 100644
> --- a/include/traceevent/event-parse.h
> +++ b/include/traceevent/event-parse.h
> @@ -557,8 +557,6 @@ void tep_set_local_bigendian(struct tep_handle *tep, enum tep_endian endian);
>  bool tep_is_latency_format(struct tep_handle *tep);
>  void tep_set_latency_format(struct tep_handle *tep, int lat);
>  int tep_get_header_page_size(struct tep_handle *tep);
> -void tep_set_parsing_failures(struct tep_handle *tep, int parsing_failures);
> -int tep_get_parsing_failures(struct tep_handle *tep);
>  int tep_get_header_timestamp_size(struct tep_handle *tep);
>  bool tep_is_old_format(struct tep_handle *tep);
>  void tep_set_print_raw(struct tep_handle *tep, int print_raw);
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index d5ee371..565675f 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -395,7 +395,8 @@ static int regex_event_buf(const char *file, int size, regex_t *epreg)
>  
>  static int read_ftrace_file(struct tracecmd_input *handle,
>  			    unsigned long long size,
> -			    int print, regex_t *epreg)
> +			    int print, regex_t *epreg,
> +			    int *parsing_failures)
>  {
>  	struct tep_handle *pevent = handle->pevent;
>  	char *buf;
> @@ -412,8 +413,9 @@ static int read_ftrace_file(struct tracecmd_input *handle,
>  		if (print || regex_event_buf(buf, size, epreg))
>  			printf("%.*s\n", (int)size, buf);
>  	} else {
> -		if (tep_parse_event(pevent, buf, size, "ftrace"))
> -			tep_set_parsing_failures(pevent, 1);
> +		if (tep_parse_event(pevent, buf, size, "ftrace") &&
> +		    parsing_failures)

This is one of those cases where breaking the line for the 80 character
rule makes the code harder to read, and shouldn't be done.

> +			(*parsing_failures)++;
>  	}
>  	free(buf);
>  
> @@ -423,7 +425,7 @@ static int read_ftrace_file(struct tracecmd_input *handle,
>  static int read_event_file(struct tracecmd_input *handle,
>  			   char *system, unsigned long long size,
>  			   int print, int *sys_printed,
> -			   regex_t *epreg)
> +			   regex_t *epreg, int *parsing_failures)
>  {
>  	struct tep_handle *pevent = handle->pevent;
>  	char *buf;
> @@ -446,8 +448,9 @@ static int read_event_file(struct tracecmd_input *handle,
>  			printf("%.*s\n", (int)size, buf);
>  		}
>  	} else {
> -		if (tep_parse_event(pevent, buf, size, system))
> -			tep_set_parsing_failures(pevent, 1);
> +		if (tep_parse_event(pevent, buf, size, system) &&
> +		    parsing_failures)

Same here.

I've gotten the OK from Linus to ignore that rule for cases like
these :-)

In fact, he told me to try to convince others that we should up it to
100 characters! I haven't tried yet, because I already got push back
from some developers. But perhaps I still might.

> +			(*parsing_failures)++;
>  	}
>  	free(buf);
>  
> @@ -497,7 +500,8 @@ static int make_preg_files(const char *regex, regex_t *system,
>  	return ret;
>  }
>  

[..]

> +/**
> + * tracecmd_read_headers - read the header information from trace.dat
> + * @handle: input handle for the trace.dat file
> + *
> + * This reads the trace.dat file for various information. Like the
> + * format of the ring buffer, event formats, ftrace formats, kallsyms
> + * and printk.
> + */
> +int tracecmd_read_headers(struct tracecmd_input *handle)
> +{
> +	return _tracecmd_read_headers(handle, NULL);
> +}
> +
> +/**
> + * tracecmd_read_headers_failures - read the header information from trace.dat
> + * @handle: input handle for the trace.dat file
> + * @parsing_failures: return number of failures while parsing the event files
> + *
> + * This reads the trace.dat file for various information. Like the
> + * format of the ring buffer, event formats, ftrace formats, kallsyms
> + * and printk.
> + */
> +int tracecmd_read_headers_failures(struct tracecmd_input *handle,
> +				   int *parsing_failures)

Let's not add this. Instead add a "parsing_failures" to the
tracecmd_input handle, and add:

int tracecmd_get_parsing_failures(struct tracecmd_input *handle)
{
	return handle->parsing_failures;
}

> +{
> +	return _tracecmd_read_headers(handle, parsing_failures);
> +}
> +

[..]

> diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
> index 52fa1bd..fe116cc 100644
> --- a/tracecmd/trace-read.c
> +++ b/tracecmd/trace-read.c
> @@ -1417,6 +1417,7 @@ void trace_report (int argc, char **argv)
>  	unsigned long long tsoffset = 0;
>  	unsigned long long ts2secs = 0;
>  	unsigned long long ts2sc;
> +	int parsing_failures;
>  	int show_stat = 0;
>  	int show_funcs = 0;
>  	int show_endian = 0;
> @@ -1714,10 +1715,10 @@ void trace_report (int argc, char **argv)
>  			tracecmd_print_events(handle, print_event);
>  			return;
>  		}
> -
> -		ret = tracecmd_read_headers(handle);
> +		parsing_failures = 0;
> +		ret = tracecmd_read_headers_failures(handle, &parsing_failures);

Here we should do:

		ret = tracecmd_read_headers(handle);

>  		if (check_event_parsing) {
> -			if (ret || tep_get_parsing_failures(pevent))

			if (ret || tracecmd_get_parsing_failures(handle))

-- Steve

> +			if (ret || parsing_failures)
>  				exit(EINVAL);
>  			else
>  				exit(0);