On Wed, 3 Jun 2020 15:15:02 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> There is a resrtiction of "trace-cmd start" which does not allow it to
> run a command, specified by "-F" option or at the end of the command
> line. However, there are use cases where this is useful.
> The resrtiction is removed, it allows "trace-cmd start" to run commands
> just like "trace-cmd record".
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
> Documentation/trace-cmd-start.1.txt | 2 +-
> tracecmd/trace-record.c | 28 ++++++++++++++++++++--------
> tracecmd/trace-usage.c | 2 +-
> 3 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/trace-cmd-start.1.txt b/Documentation/trace-cmd-start.1.txt
> index 2b027b01..63d2034c 100644
> --- a/Documentation/trace-cmd-start.1.txt
> +++ b/Documentation/trace-cmd-start.1.txt
> @@ -21,7 +21,7 @@ system or can be extracted with trace-cmd-extract(1).
> OPTIONS
> -------
> The options are the same as 'trace-cmd-record(1)', except that it does not
> -take options specific to recording (*-s*, *-o*, *-F*, *-N*, and *-t*).
> +take options specific to recording (*-s*, *-o*, *-N*, and *-t*).
>
> SEE ALSO
> --------
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 3242368e..f345c6d6 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -1598,7 +1598,8 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
> /* child */
> update_task_filter();
> tracecmd_enable_tracing();
> - enable_ptrace();
> + if (type != TRACE_TYPE_START)
> + enable_ptrace();
> /*
> * If we are using stderr for stdout, switch
> * it back to the saved stdout for the code we run.
> @@ -1619,6 +1620,8 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
> die("Failed to exec %s", argv[0]);
> }
> }
> + if (type & TRACE_TYPE_START)
> + exit(0);
After playing with this a bit, I think we should not just fork and
ignore the executable. We should wait for it to finish. This means we
can be able to ptrace via trace-cmd start as well.
The reason I say this, is the few times I used it, I didn't think it
was working. For example, I did:
# trace-cmd start -e all ls -ltr /bin
And it returned then printed the output of ls. Because of that, it
looked like it hung:
[root@bxtest ~]# trace-cmd start -e all ls -ltr ~/bin
[root@bxtest ~]# total 48716
-rwxr-xr-x 1 root root 50320 Apr 24 18:12 bootconfig
-rwxr-xr-x 3 root root 24902584 May 27 12:31 trace
-rwxr-xr-x 3 root root 24902584 May 27 12:31 perf
-rwxr-xr-x 1 root root 20860 May 27 12:31 perf-read-vdso32
And I'm waiting there for my prompt, not noticing that the prompt was
already printed before the output.
Although, we are not recording, it should by default wait for the
execed program to finish. We can add an option to change that, like:
# trace-cmd start --fork -e all ls -ltr ~/bin
Which would let the user to explicitly ask for this behavior. But it
shouldn't be the default behavior. I basically think it hangs each time
I do this, even though I know why it does this.
-- Steve
> if (do_ptrace) {
> ptrace_attach(NULL, pid);
> ptrace_wait(type);
> @@ -5782,6 +5785,14 @@ static void add_arg(struct buffer_instance *instance,
> /* Not found? */
> }
>
> +static inline void cmd_check_die(struct common_record_context *ctx,
> + enum trace_cmd id, char *cmd, char *param)
> +{
> + if (ctx->curr_cmd == id)
> + die("%s has no effect with the command %s\n"
> + "Did you mean 'record'?", param, cmd);
> +}
> +
> static void parse_record_options(int argc,
> char **argv,
> enum trace_cmd curr_cmd,
> @@ -6114,6 +6125,7 @@ static void parse_record_options(int argc,
> die("Failed to allocate user name");
> break;
> case OPT_procmap:
> + cmd_check_die(ctx, CMD_start, *(argv+1), "--proc-map");
> ctx->instance->get_procmap = 1;
> break;
> case OPT_date:
> @@ -6221,9 +6233,6 @@ static void parse_record_options(int argc,
> die(" -c can only be used with -F (or -P with event-fork support)");
>
> if ((argc - optind) >= 2) {
> - if (IS_START(ctx))
> - die("Command start does not take any commands\n"
> - "Did you mean 'record'?");
> if (IS_EXTRACT(ctx))
> die("Command extract does not take any commands\n"
> "Did you mean 'record'?");
> @@ -6244,6 +6253,9 @@ static void parse_record_options(int argc,
> }
> }
>
> + if (do_ptrace && IS_START(ctx))
> + die("ptrace not supported with command start");
> +
> for_all_instances(instance) {
> if (instance->get_procmap && !instance->nr_filter_pids) {
> warning("--proc-map is ignored for instance %s, "
> @@ -6410,10 +6422,6 @@ static void record_trace(int argc, char **argv,
> signal(SIGINT, finish);
> if (!latency)
> start_threads(type, ctx);
> - } else {
> - update_task_filter();
> - tracecmd_enable_tracing();
> - exit(0);
> }
>
> if (ctx->run_command) {
> @@ -6428,6 +6436,10 @@ static void record_trace(int argc, char **argv,
>
> update_task_filter();
> tracecmd_enable_tracing();
> +
> + if (type & TRACE_TYPE_START)
> + exit(0);
> +
> /* We don't ptrace ourself */
> if (do_ptrace) {
> for_all_instances(instance) {
> diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> index d43ca490..58bca9e4 100644
> --- a/tracecmd/trace-usage.c
> +++ b/tracecmd/trace-usage.c
> @@ -69,7 +69,7 @@ static struct usage_help usage_help[] = {
> "start",
> "start tracing without recording into a file",
> " %s start [-e event][-p plugin][-d][-O option ][-P pid]\n"
> - " Uses same options as record, but does not run a command.\n"
> + " Uses same options as record.\n"
> " It only enables the tracing and exits\n"
> },
> {
|