[PATCH] trace-cmd split: Copy trace_clock from input handler to output handler

[PATCH] trace-cmd split: Copy trace_clock from input handler to output handler

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

When splitting a trace file, the clock is needed to create a output file
from the input file. But the output descriptor clock is never set, and the
clock returned from retrieving the descriptor is NULL (unless you are
root, in which case the code will read the machine trace clock instead,
and continue as if nothing was wrong). This caused a failure, and once
again, trace-cmd split, errored out without failure, but just did not
append all the data.

Although this fixes the commit listed below, the problem was there before,
as the code before that commit tried to read the clock from the file
system, but just wouldn't error out if it couldn't read it.

Add a new function to retrieve the trace_clock from the input handle, to
be used to pass it to the output handle before appending the CPU data.

Reported-by: Julia Lawall <julia.lawall@inria.fr>
Fixes: 72670886 ("trace-cmd: Save only the selected clock in the trace.dat file")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/trace-cmd/include/private/trace-cmd-private.h |  3 ++-
 lib/trace-cmd/trace-input.c                       | 13 +++++++++++++
 lib/trace-cmd/trace-output.c                      |  2 +-
 tracecmd/trace-split.c                            |  1 +
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 7194cb30..6440084d 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -87,7 +87,8 @@ tracecmd_plugin_context_output(struct trace_plugin_context *trace_context);
 
 void tracecmd_set_quiet(struct tracecmd_output *handle, bool set_quiet);
 bool tracecmd_get_quiet(struct tracecmd_output *handle);
-void tracecmd_set_out_clock(struct tracecmd_output *handle, char *clock);
+void tracecmd_set_out_clock(struct tracecmd_output *handle, const char *clock);
+const char *tracecmd_get_trace_clock(struct tracecmd_input *handle);
 
 static inline int tracecmd_host_bigendian(void)
 {
diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 655bd654..ac57bc4f 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -4075,6 +4075,19 @@ size_t tracecmd_get_options_offset(struct tracecmd_input *handle)
 	return handle->options_start;
 }
 
+/**
+ * tracecmd_get_trace_clock - return the saved trace clock
+ * @handle: input handle for the trace.dat file
+ *
+ * Returns a string of the clock that was saved in the trace.dat file.
+ * The string should not be freed, as it points to the internal
+ * structure data.
+ */
+const char *tracecmd_get_trace_clock(struct tracecmd_input *handle)
+{
+	return handle->trace_clock;
+}
+
 /**
  * tracecmd_get_show_data_func - return the show data func
  * @handle: input handle for the trace.dat file
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
index 78a25350..a8de107c 100644
--- a/lib/trace-cmd/trace-output.c
+++ b/lib/trace-cmd/trace-output.c
@@ -122,7 +122,7 @@ void tracecmd_set_quiet(struct tracecmd_output *handle, bool set_quiet)
 		handle->quiet = set_quiet;
 }
 
-void tracecmd_set_out_clock(struct tracecmd_output *handle, char *clock)
+void tracecmd_set_out_clock(struct tracecmd_output *handle, const char *clock)
 {
 	if (handle && clock)
 		handle->trace_clock = strdup(clock);
diff --git a/tracecmd/trace-split.c b/tracecmd/trace-split.c
index 8366d128..10d0943d 100644
--- a/tracecmd/trace-split.c
+++ b/tracecmd/trace-split.c
@@ -384,6 +384,7 @@ static double parse_file(struct tracecmd_input *handle,
 	for (cpu = 0; cpu < cpus; cpu ++)
 		cpu_list[cpu] = cpu_data[cpu].file;
 
+	tracecmd_set_out_clock(ohandle, tracecmd_get_trace_clock(handle));
 	tracecmd_append_cpu_data(ohandle, cpus, cpu_list);
 
 	current = end;
-- 
2.29.2

Re: [PATCH] trace-cmd split: Copy trace_clock from input handler to output handler

[Julia Lawall]

On Tue, 22 Jun 2021, Steven Rostedt wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> When splitting a trace file, the clock is needed to create a output file
> from the input file. But the output descriptor clock is never set, and the
> clock returned from retrieving the descriptor is NULL (unless you are
> root, in which case the code will read the machine trace clock instead,

Thanks for the fix.  I should have mentioned another difference between
the machine where it worked and the machine where it didn't, which is that
I was root on the former and not root on the latter.

> and continue as if nothing was wrong). This caused a failure, and once
> again, trace-cmd split, errored out without failure, but just did not
> append all the data.
>
> Although this fixes the commit listed below, the problem was there before,
> as the code before that commit tried to read the clock from the file
> system, but just wouldn't error out if it couldn't read it.
>
> Add a new function to retrieve the trace_clock from the input handle, to
> be used to pass it to the output handle before appending the CPU data.

Just to confirm, is it still possible to run trace-cmd split without being
root?

thanks,
julia

> Reported-by: Julia Lawall <julia.lawall@inria.fr>
> Fixes: 72670886 ("trace-cmd: Save only the selected clock in the trace.dat file")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  lib/trace-cmd/include/private/trace-cmd-private.h |  3 ++-
>  lib/trace-cmd/trace-input.c                       | 13 +++++++++++++
>  lib/trace-cmd/trace-output.c                      |  2 +-
>  tracecmd/trace-split.c                            |  1 +
>  4 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index 7194cb30..6440084d 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -87,7 +87,8 @@ tracecmd_plugin_context_output(struct trace_plugin_context *trace_context);
>
>  void tracecmd_set_quiet(struct tracecmd_output *handle, bool set_quiet);
>  bool tracecmd_get_quiet(struct tracecmd_output *handle);
> -void tracecmd_set_out_clock(struct tracecmd_output *handle, char *clock);
> +void tracecmd_set_out_clock(struct tracecmd_output *handle, const char *clock);
> +const char *tracecmd_get_trace_clock(struct tracecmd_input *handle);
>
>  static inline int tracecmd_host_bigendian(void)
>  {
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 655bd654..ac57bc4f 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -4075,6 +4075,19 @@ size_t tracecmd_get_options_offset(struct tracecmd_input *handle)
>  	return handle->options_start;
>  }
>
> +/**
> + * tracecmd_get_trace_clock - return the saved trace clock
> + * @handle: input handle for the trace.dat file
> + *
> + * Returns a string of the clock that was saved in the trace.dat file.
> + * The string should not be freed, as it points to the internal
> + * structure data.
> + */
> +const char *tracecmd_get_trace_clock(struct tracecmd_input *handle)
> +{
> +	return handle->trace_clock;
> +}
> +
>  /**
>   * tracecmd_get_show_data_func - return the show data func
>   * @handle: input handle for the trace.dat file
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 78a25350..a8de107c 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -122,7 +122,7 @@ void tracecmd_set_quiet(struct tracecmd_output *handle, bool set_quiet)
>  		handle->quiet = set_quiet;
>  }
>
> -void tracecmd_set_out_clock(struct tracecmd_output *handle, char *clock)
> +void tracecmd_set_out_clock(struct tracecmd_output *handle, const char *clock)
>  {
>  	if (handle && clock)
>  		handle->trace_clock = strdup(clock);
> diff --git a/tracecmd/trace-split.c b/tracecmd/trace-split.c
> index 8366d128..10d0943d 100644
> --- a/tracecmd/trace-split.c
> +++ b/tracecmd/trace-split.c
> @@ -384,6 +384,7 @@ static double parse_file(struct tracecmd_input *handle,
>  	for (cpu = 0; cpu < cpus; cpu ++)
>  		cpu_list[cpu] = cpu_data[cpu].file;
>
> +	tracecmd_set_out_clock(ohandle, tracecmd_get_trace_clock(handle));
>  	tracecmd_append_cpu_data(ohandle, cpus, cpu_list);
>
>  	current = end;
> --
> 2.29.2
>
>

Re: [PATCH] trace-cmd split: Copy trace_clock from input handler to output handler

[Steven Rostedt]

On Tue, 22 Jun 2021 23:25:23 +0200 (CEST)
Julia Lawall <julia.lawall@inria.fr> wrote:

> > Add a new function to retrieve the trace_clock from the input handle, to
> > be used to pass it to the output handle before appending the CPU data.  
> 
> Just to confirm, is it still possible to run trace-cmd split without being
> root?

Yes, it was a bug that you couldn't read it as non-root. And it shouldn't be
reading the local trace_clock anyway, even if you are root.

1) because it could be splitting a trace.dat file from another machine
   (which is what I was doing)

2) the trace_clock of the machine could have changed since the trace was
   run.

-- Steve