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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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);
|