Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] trace-cmd: fix extract output option
@ 2020-09-29 17:49 vincent.donnefort
  2020-09-29 20:31 ` Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: vincent.donnefort @ 2020-09-29 17:49 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: Vincent Donnefort

From: Vincent Donnefort <vincent.donnefort@arm.com>

During the introduction of instance's output_file copy:

  3a206ca ("trace-cmd: Have instances include a copy of its output file")

The extract path has been omitted, leading to a broken output option:

  $ trace-cmd extract -o /foo/bar.dat # Will fallback to ./trace.dat

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index bd00457..72a5c8c 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -6622,6 +6622,9 @@ void trace_extract(int argc, char **argv)
 
 	/* Save the state of tracing_on before starting */
 	for_all_instances(instance) {
+		instance->output_file = strdup(ctx.output);
+		if (!instance->output_file)
+			die("Failed to allocate output file name for instance");
 
 		if (!ctx.manual && instance->flags & BUFFER_FL_PROFILE)
 			enable_profile(ctx.instance);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] trace-cmd: fix extract output option
  2020-09-29 17:49 [PATCH] trace-cmd: fix extract output option vincent.donnefort
@ 2020-09-29 20:31 ` Steven Rostedt
  2020-09-30  8:31   ` Vincent Donnefort
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2020-09-29 20:31 UTC (permalink / raw)
  To: vincent.donnefort; +Cc: linux-trace-devel

On Tue, 29 Sep 2020 18:49:26 +0100
vincent.donnefort@arm.com wrote:

> From: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> During the introduction of instance's output_file copy:
> 
>   3a206ca ("trace-cmd: Have instances include a copy of its output file")
> 
> The extract path has been omitted, leading to a broken output option:
> 
>   $ trace-cmd extract -o /foo/bar.dat # Will fallback to ./trace.dat

When I tried this it worked fine to me. But then I walked through the logic
via gdb and found that the intermediate step (the one that writes the
individual buffers directly), which can be an issue if you happen to
execute this in a directory that you can not write to, or doesn't have
enough space to hold all the data. Thus your patch is correct, but the
change log is not.

Do you really see "trace.dat" at the end of that command? Because I
see /foo/bar.dat.

But if I try to run the extract in /sys/kernel/tracing, it will fail
because it will try to write "(null).cpuX" where X is the CPU number.

But the creation of the actual file uses ctx->output, which is what we want.

Anyway, I'll update the change log to this:

    During the introduction of instance's output_file copy:

      3a206ca ("trace-cmd: Have instances include a copy of its output file")

    The extract path has been omitted, causing the temp files created to be
    written in the same directory using the null "output_file" of the
    instance, to create "(null).cpuX" files. If this is executed in a
    directory that is not writable (like /sys/kernel/tracing) or does not
    have enough space to hold the temp files, then it will fail to write.

Fair?

-- Steve


> 
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index bd00457..72a5c8c 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -6622,6 +6622,9 @@ void trace_extract(int argc, char **argv)
>  
>  	/* Save the state of tracing_on before starting */
>  	for_all_instances(instance) {
> +		instance->output_file = strdup(ctx.output);
> +		if (!instance->output_file)
> +			die("Failed to allocate output file name for instance");
>  
>  		if (!ctx.manual && instance->flags & BUFFER_FL_PROFILE)
>  			enable_profile(ctx.instance);


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] trace-cmd: fix extract output option
  2020-09-29 20:31 ` Steven Rostedt
@ 2020-09-30  8:31   ` Vincent Donnefort
  0 siblings, 0 replies; 3+ messages in thread
From: Vincent Donnefort @ 2020-09-30  8:31 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel

On Tue, Sep 29, 2020 at 04:31:52PM -0400, Steven Rostedt wrote:
> On Tue, 29 Sep 2020 18:49:26 +0100
> vincent.donnefort@arm.com wrote:
> 
> > From: Vincent Donnefort <vincent.donnefort@arm.com>
> > 
> > During the introduction of instance's output_file copy:
> > 
> >   3a206ca ("trace-cmd: Have instances include a copy of its output file")
> > 
> > The extract path has been omitted, leading to a broken output option:
> > 
> >   $ trace-cmd extract -o /foo/bar.dat # Will fallback to ./trace.dat
> 
> When I tried this it worked fine to me. But then I walked through the logic
> via gdb and found that the intermediate step (the one that writes the
> individual buffers directly), which can be an issue if you happen to
> execute this in a directory that you can not write to, or doesn't have
> enough space to hold all the data. Thus your patch is correct, but the
> change log is not.
> 
> Do you really see "trace.dat" at the end of that command? Because I
> see /foo/bar.dat.
> 
> But if I try to run the extract in /sys/kernel/tracing, it will fail
> because it will try to write "(null).cpuX" where X is the CPU number.
> 
> But the creation of the actual file uses ctx->output, which is what we want.
> 
> Anyway, I'll update the change log to this:
> 
>     During the introduction of instance's output_file copy:
> 
>       3a206ca ("trace-cmd: Have instances include a copy of its output file")
> 
>     The extract path has been omitted, causing the temp files created to be
>     written in the same directory using the null "output_file" of the
>     instance, to create "(null).cpuX" files. If this is executed in a
>     directory that is not writable (like /sys/kernel/tracing) or does not
>     have enough space to hold the temp files, then it will fail to write.
> 
> Fair?
> 
> -- Steve

Apologies, the description (... and my testing) was indeed incorrect. The
output is working fine, the problem being only the temporary files.

No problem with the updated commit description. We probably need to update
the short description as well:

  "trace-cmd: Fix extract temporary files path" ?

-- 
Vincent


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 17:49 [PATCH] trace-cmd: fix extract output option vincent.donnefort
2020-09-29 20:31 ` Steven Rostedt
2020-09-30  8:31   ` Vincent Donnefort

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git