[PATCH v2 00/10] trace-cmd: Refactoring trace_record()

[PATCH v2 00/10] trace-cmd: Refactoring trace_record()

[Vladislav Valtchev (VMware)]

The following series of patches is a refactoring of trace_record() trying to
reduce as much as possible its complexity but, at the same time, without making
risky changes. All the patches are relatively small and potentially no-brainer
steps towards the final shape of the code.

The rationale for this effort is to make the code much easier to maintain.

Vladislav Valtchev (VMware) (10):
  trace-cmd: Extract parse_record_options() from trace_record()
  trace-cmd: Replacing cmd flags w/ a trace_cmd enum
  trace-cmd: Extracting record_trace()
  trace-cmd: Rename trace_profile() to do_trace_profile()
  trace-cmd: Making start,extract,stream,profile separate funcs
  trace-cmd: Extr. profile-specific code from record_trace
  trace-cmd: Mov init_common_record_context in parse_record_options
  trace-cmd: Introducing get_trace_cmd_type()
  trace-cmd: Extract finalize_record_trace()
  trace-cmd: Fork record_trace() for the extract case

 trace-cmd.c     |   8 +-
 trace-local.h   |  10 +-
 trace-profile.c |   2 +-
 trace-read.c    |   2 +-
 trace-record.c  | 640 +++++++++++++++++++++++++++++++++-----------------------
 5 files changed, 390 insertions(+), 272 deletions(-)

-- 
2.14.1

[PATCH v2 01/10] trace-cmd: Extract parse_record_options() from trace_record()

[Vladislav Valtchev (VMware)]

In this patch a huge part of trace_record(), dealing with parsing the command
line options, has been extracted in a separate function. That allows the body
of trace_record() to be focused only on the proper actions involved in a given
type of tracing (record, stream, etc.) reducing, this way, the scope and the
complexity of trace_record().

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 335 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 179 insertions(+), 156 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 770b5bd..487d35e 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4358,62 +4358,57 @@ void trace_reset(int argc, char **argv)
 	exit(0);
 }
 
-void trace_record(int argc, char **argv)
+struct common_record_context {
+	struct buffer_instance *instance;
+	const char *output;
+	char *date2ts;
+	char *max_graph_depth;
+	int data_flags;
+
+	int record_all;
+	int total_disable;
+	int disable;
+	int events;
+	int global;
+	int filtered;
+	int date;
+	int manual;
+	int topt;
+	int do_child;
+	int run_command;
+
+	int extract;
+	int start;
+	int stream;
+	int profile;
+	int record;
+};
+
+static void init_common_record_context(struct common_record_context *ctx)
+{
+	memset(ctx, 0, sizeof(*ctx));
+	ctx->instance = &top_instance;
+	init_instance(ctx->instance);
+	cpu_count = count_cpus();
+}
+
+static void parse_record_options(int argc,
+				 char **argv,
+				 struct common_record_context *ctx)
 {
 	const char *plugin = NULL;
-	const char *output = NULL;
 	const char *option;
 	struct event_list *event = NULL;
 	struct event_list *last_event = NULL;
-	struct buffer_instance *instance = &top_instance;
-	enum trace_type type = 0;
 	char *pids;
 	char *pid;
 	char *sav;
-	char *date2ts = NULL;
-	int record_all = 0;
-	int total_disable = 0;
-	int disable = 0;
-	int events = 0;
-	int record = 0;
-	int extract = 0;
-	int stream = 0;
-	int profile = 0;
-	int global = 0;
-	int start = 0;
-	int filtered = 0;
-	int run_command = 0;
 	int neg_event = 0;
-	int date = 0;
-	int manual = 0;
-	char *max_graph_depth = NULL;
-	int topt = 0;
-	int do_child = 0;
-	int data_flags = 0;
-
-	int c;
-
-	init_instance(instance);
-
-	cpu_count = count_cpus();
-
-	if ((record = (strcmp(argv[1], "record") == 0)))
-		; /* do nothing */
-	else if ((start = strcmp(argv[1], "start") == 0))
-		; /* do nothing */
-	else if ((extract = strcmp(argv[1], "extract") == 0))
-		; /* do nothing */
-	else if ((stream = strcmp(argv[1], "stream") == 0))
-		; /* do nothing */
-	else if ((profile = strcmp(argv[1], "profile") == 0)) {
-		handle_init = trace_init_profile;
-		events = 1;
-	} else
-		usage(argv);
 
 	for (;;) {
 		int option_index = 0;
 		int ret;
+		int c;
 		const char *opts;
 		static struct option long_options[] = {
 			{"date", no_argument, NULL, OPT_date},
@@ -4431,7 +4426,7 @@ void trace_record(int argc, char **argv)
 			{NULL, 0, NULL, 0}
 		};
 
-		if (extract)
+		if (ctx->extract)
 			opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT";
 		else
 			opts = "+hae:f:Fp:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q";
@@ -4443,26 +4438,26 @@ void trace_record(int argc, char **argv)
 			usage(argv);
 			break;
 		case 'a':
-			if (extract) {
+			if (ctx->extract) {
 				add_all_instances();
 			} else {
-				record_all = 1;
+				ctx->record_all = 1;
 				record_all_events();
 			}
 			break;
 		case 'e':
-			events = 1;
+			ctx->events = 1;
 			event = malloc(sizeof(*event));
 			if (!event)
 				die("Failed to allocate event %s", optarg);
 			memset(event, 0, sizeof(*event));
 			event->event = optarg;
-			add_event(instance, event);
+			add_event(ctx->instance, event);
 			event->neg = neg_event;
 			event->filter = NULL;
 			last_event = event;
 
-			if (!record_all)
+			if (!ctx->record_all)
 				list_event(optarg);
 			break;
 		case 'f':
@@ -4495,7 +4490,7 @@ void trace_record(int argc, char **argv)
 			filter_task = 1;
 			break;
 		case 'G':
-			global = 1;
+			ctx->global = 1;
 			break;
 		case 'P':
 			test_set_event_pid();
@@ -4518,35 +4513,35 @@ void trace_record(int argc, char **argv)
 				do_ptrace = 1;
 			} else {
 				save_option("event-fork");
-				do_child = 1;
+				ctx->do_child = 1;
 			}
 			break;
 		case 'C':
-			instance->clock = optarg;
+			ctx->instance->clock = optarg;
 			break;
 		case 'v':
 			neg_event = 1;
 			break;
 		case 'l':
-			add_func(&instance->filter_funcs,
-				 instance->filter_mod, optarg);
-			filtered = 1;
+			add_func(&ctx->instance->filter_funcs,
+				 ctx->instance->filter_mod, optarg);
+			ctx->filtered = 1;
 			break;
 		case 'n':
-			add_func(&instance->notrace_funcs,
-				 instance->filter_mod, optarg);
-			filtered = 1;
+			add_func(&ctx->instance->notrace_funcs,
+				 ctx->instance->filter_mod, optarg);
+			ctx->filtered = 1;
 			break;
 		case 'g':
-			add_func(&graph_funcs, instance->filter_mod, optarg);
-			filtered = 1;
+			add_func(&graph_funcs, ctx->instance->filter_mod, optarg);
+			ctx->filtered = 1;
 			break;
 		case 'p':
-			if (instance->plugin)
+			if (ctx->instance->plugin)
 				die("only one plugin allowed");
 			for (plugin = optarg; isspace(*plugin); plugin++)
 				;
-			instance->plugin = plugin;
+			ctx->instance->plugin = plugin;
 			for (optarg += strlen(optarg) - 1;
 			     optarg > plugin && isspace(*optarg); optarg--)
 				;
@@ -4554,25 +4549,25 @@ void trace_record(int argc, char **argv)
 			optarg[0] = '\0';
 			break;
 		case 'D':
-			total_disable = 1;
+			ctx->total_disable = 1;
 			/* fall through */
 		case 'd':
-			disable = 1;
+			ctx->disable = 1;
 			break;
 		case 'o':
 			if (host)
 				die("-o incompatible with -N");
-			if (start)
+			if (ctx->start)
 				die("start does not take output\n"
 				    "Did you mean 'record'?");
-			if (stream)
+			if (ctx->stream)
 				die("stream does not take output\n"
 				    "Did you mean 'record'?");
-			if (output)
+			if (ctx->output)
 				die("only one output file allowed");
-			output = optarg;
+			ctx->output = optarg;
 
-			if (profile) {
+			if (ctx->profile) {
 				int fd;
 
 				/* pipe the output to this file instead of stdout */
@@ -4595,11 +4590,11 @@ void trace_record(int argc, char **argv)
 			save_option("stacktrace");
 			break;
 		case 'H':
-			add_hook(instance, optarg);
-			events = 1;
+			add_hook(ctx->instance, optarg);
+			ctx->events = 1;
 			break;
 		case 's':
-			if (extract) {
+			if (ctx->extract) {
 				if (optarg)
 					usage(argv);
 				recorder_flags |= TRACECMD_RECORD_SNAPSHOT;
@@ -4610,47 +4605,47 @@ void trace_record(int argc, char **argv)
 			sleep_time = atoi(optarg);
 			break;
 		case 'S':
-			manual = 1;
+			ctx->manual = 1;
 			/* User sets events for profiling */
 			if (!event)
-				events = 0;
+				ctx->events = 0;
 			break;
 		case 'r':
 			rt_prio = atoi(optarg);
 			break;
 		case 'N':
-			if (!record)
+			if (!ctx->record)
 				die("-N only available with record");
-			if (output)
+			if (ctx->output)
 				die("-N incompatible with -o");
 			host = optarg;
 			break;
 		case 'm':
 			if (max_kb)
 				die("-m can only be specified once");
-			if (!record)
+			if (!ctx->record)
 				die("only record take 'm' option");
 			max_kb = atoi(optarg);
 			break;
 		case 'M':
-			instance->cpumask = alloc_mask_from_hex(optarg);
+			ctx->instance->cpumask = alloc_mask_from_hex(optarg);
 			break;
 		case 't':
-			if (extract)
-				topt = 1; /* Extract top instance also */
+			if (ctx->extract)
+				ctx->topt = 1; /* Extract top instance also */
 			else
 				use_tcp = 1;
 			break;
 		case 'b':
-			instance->buffer_size = atoi(optarg);
+			ctx->instance->buffer_size = atoi(optarg);
 			break;
 		case 'B':
-			instance = create_instance(optarg);
-			if (!instance)
+			ctx->instance = create_instance(optarg);
+			if (!ctx->instance)
 				die("Failed to create instance");
-			add_instance(instance);
-			if (profile)
-				instance->profile = 1;
+			add_instance(ctx->instance);
+			if (ctx->profile)
+				ctx->instance->profile = 1;
 			break;
 		case 'k':
 			keep = 1;
@@ -4659,10 +4654,10 @@ void trace_record(int argc, char **argv)
 			ignore_event_not_found = 1;
 			break;
 		case OPT_date:
-			date = 1;
-			if (data_flags & DATA_FL_OFFSET)
+			ctx->date = 1;
+			if (ctx->data_flags & DATA_FL_OFFSET)
 				die("Can not use both --date and --ts-offset");
-			data_flags |= DATA_FL_DATE;
+			ctx->data_flags |= DATA_FL_DATE;
 			break;
 		case OPT_funcstack:
 			func_stack = 1;
@@ -4672,8 +4667,8 @@ void trace_record(int argc, char **argv)
 			break;
 		case OPT_profile:
 			handle_init = trace_init_profile;
-			instance->profile = 1;
-			events = 1;
+			ctx->instance->profile = 1;
+			ctx->events = 1;
 			break;
 		case OPT_stderr:
 			/* if -o was used (for profile), ignore this */
@@ -4687,26 +4682,26 @@ void trace_record(int argc, char **argv)
 			trace_profile_set_merge_like_comms();
 			break;
 		case OPT_tsoffset:
-			date2ts = strdup(optarg);
-			if (data_flags & DATA_FL_DATE)
+			ctx->date2ts = strdup(optarg);
+			if (ctx->data_flags & DATA_FL_DATE)
 				die("Can not use both --date and --ts-offset");
-			data_flags |= DATA_FL_OFFSET;
+			ctx->data_flags |= DATA_FL_OFFSET;
 			break;
 		case OPT_max_graph_depth:
-			free(max_graph_depth);
-			max_graph_depth = strdup(optarg);
-			if (!max_graph_depth)
+			free(ctx->max_graph_depth);
+			ctx->max_graph_depth = strdup(optarg);
+			if (!ctx->max_graph_depth)
 				die("Could not allocate option");
 			break;
 		case OPT_debug:
 			debug = 1;
 			break;
 		case OPT_module:
-			if (instance->filter_mod)
-				add_func(&instance->filter_funcs,
-					 instance->filter_mod, "*");
-			instance->filter_mod = optarg;
-			filtered = 0;
+			if (ctx->instance->filter_mod)
+				add_func(&ctx->instance->filter_funcs,
+					 ctx->instance->filter_mod, "*");
+			ctx->instance->filter_mod = optarg;
+			ctx->filtered = 0;
 			break;
 		case OPT_quiet:
 		case 'q':
@@ -4717,104 +4712,131 @@ void trace_record(int argc, char **argv)
 		}
 	}
 
-	if (!filtered && instance->filter_mod)
-		add_func(&instance->filter_funcs,
-			 instance->filter_mod, "*");
+	if (!ctx->filtered && ctx->instance->filter_mod)
+		add_func(&ctx->instance->filter_funcs,
+			 ctx->instance->filter_mod, "*");
 
 	if (do_ptrace && !filter_task && (filter_pid < 0))
 		die(" -c can only be used with -F (or -P with event-fork support)");
-	if (do_child && !filter_task &&! filter_pid)
+	if (ctx->do_child && !filter_task &&! filter_pid)
 		die(" -c can only be used with -P or -F");
 
 	if ((argc - optind) >= 2) {
-		if (start)
+		if (ctx->start)
 			die("Command start does not take any commands\n"
 			    "Did you mean 'record'?");
-		if (extract)
+		if (ctx->extract)
 			die("Command extract does not take any commands\n"
 			    "Did you mean 'record'?");
-		run_command = 1;
+		ctx->run_command = 1;
 	}
 
+}
+
+void trace_record(int argc, char **argv)
+{
+	struct common_record_context ctx;
+	enum trace_type type = 0;
+
+	init_common_record_context(&ctx);
+
+	if ((ctx.record = (strcmp(argv[1], "record") == 0)))
+		; /* do nothing */
+	else if ((ctx.start = strcmp(argv[1], "start") == 0))
+		; /* do nothing */
+	else if ((ctx.extract = strcmp(argv[1], "extract") == 0))
+		; /* do nothing */
+	else if ((ctx.stream = strcmp(argv[1], "stream") == 0))
+		; /* do nothing */
+	else if ((ctx.profile = strcmp(argv[1], "profile") == 0)) {
+		handle_init = trace_init_profile;
+		ctx.events = 1;
+	} else
+		usage(argv);
+
+
+	parse_record_options(argc, argv, &ctx);
+
+
 	/*
 	 * If this is a profile run, and no instances were set,
 	 * then enable profiling on the top instance.
 	 */
-	if (profile && !buffer_instances)
+	if (ctx.profile && !buffer_instances)
 		top_instance.profile = 1;
 
 	/*
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
 	 */
-	if (!extract && !__check_doing_something(&top_instance))
+	if (!ctx.extract && !__check_doing_something(&top_instance))
 		first_instance = buffer_instances;
 	else
-		topt = 1;
+		ctx.topt = 1;
 
-	update_first_instance(instance, topt);
+	update_first_instance(ctx.instance, ctx.topt);
 
-	if (!extract)
+	if (!ctx.extract)
 		check_doing_something();
 	check_function_plugin();
 
-	if (output)
-		output_file = output;
+	if (ctx.output)
+		output_file = ctx.output;
 
 	/* Save the state of tracing_on before starting */
-	for_all_instances(instance) {
+	for_all_instances(ctx.instance) {
 
-		if (!manual && instance->profile)
-			enable_profile(instance);
+		if (!ctx.manual && ctx.instance->profile)
+			enable_profile(ctx.instance);
 
-		instance->tracing_on_init_val = read_tracing_on(instance);
+		ctx.instance->tracing_on_init_val = read_tracing_on(ctx.instance);
 		/* Some instances may not be created yet */
-		if (instance->tracing_on_init_val < 0)
-			instance->tracing_on_init_val = 1;
+		if (ctx.instance->tracing_on_init_val < 0)
+			ctx.instance->tracing_on_init_val = 1;
 	}
 
 	/* Extracting data records all events in the system. */
-	if (extract && !record_all)
+	if (ctx.extract && !ctx.record_all)
 		record_all_events();
 
-	if (!extract)
+	if (!ctx.extract)
 		make_instances();
 
-	if (events)
+	if (ctx.events)
 		expand_event_list();
 
 	page_size = getpagesize();
 
-	if (!extract) {
-		fset = set_ftrace(!disable, total_disable);
+	if (!ctx.extract) {
+		fset = set_ftrace(!ctx.disable, ctx.total_disable);
 		tracecmd_disable_all_tracing(1);
 
-		for_all_instances(instance)
-			set_clock(instance);
+		for_all_instances(ctx.instance)
+			set_clock(ctx.instance);
 
 		/* Record records the date first */
-		if (record && date)
-			date2ts = get_date_to_ts();
+		if (ctx.record && ctx.date)
+			ctx.date2ts = get_date_to_ts();
 
-		for_all_instances(instance) {
-			set_funcs(instance);
-			set_mask(instance);
+		for_all_instances(ctx.instance) {
+			set_funcs(ctx.instance);
+			set_mask(ctx.instance);
 		}
 
-		if (events) {
-			for_all_instances(instance)
-				enable_events(instance);
+		if (ctx.events) {
+			for_all_instances(ctx.instance)
+				enable_events(ctx.instance);
 		}
 		set_buffer_size();
 	}
 
-	if (record)
+	if (ctx.record)
 		type = TRACE_TYPE_RECORD;
-	else if (stream)
+	else if (ctx.stream)
 		type = TRACE_TYPE_STREAM;
-	else if (extract)
+	else if (ctx.extract)
 		type = TRACE_TYPE_EXTRACT;
-	else if (profile)
+	else if (ctx.profile)
 		type = TRACE_TYPE_STREAM;
 	else
 		type = TRACE_TYPE_START;
@@ -4823,10 +4845,10 @@ void trace_record(int argc, char **argv)
 
 	set_options();
 
-	if (max_graph_depth) {
-		for_all_instances(instance)
-			set_max_graph_depth(instance, max_graph_depth);
-		free(max_graph_depth);
+	if (ctx.max_graph_depth) {
+		for_all_instances(ctx.instance)
+			set_max_graph_depth(ctx.instance, ctx.max_graph_depth);
+		free(ctx.max_graph_depth);
 	}
 
 	allocate_seq();
@@ -4834,10 +4856,10 @@ void trace_record(int argc, char **argv)
 	if (type & (TRACE_TYPE_RECORD | TRACE_TYPE_STREAM)) {
 		signal(SIGINT, finish);
 		if (!latency)
-			start_threads(type, global);
+			start_threads(type, ctx.global);
 	}
 
-	if (extract) {
+	if (ctx.extract) {
 		flush_threads();
 
 	} else {
@@ -4847,7 +4869,7 @@ void trace_record(int argc, char **argv)
 			exit(0);
 		}
 
-		if (run_command)
+		if (ctx.run_command)
 			run_cmd(type, (argc - optind) - 1, &argv[optind + 1]);
 		else {
 			update_task_filter();
@@ -4872,17 +4894,17 @@ void trace_record(int argc, char **argv)
 		tracecmd_disable_all_tracing(0);
 
 	/* extract records the date after extraction */
-	if (extract && date) {
+	if (ctx.extract && ctx.date) {
 		/*
 		 * We need to start tracing, don't let other traces
 		 * screw with our trace_marker.
 		 */
 		tracecmd_disable_all_tracing(1);
-		date2ts = get_date_to_ts();
+		ctx.date2ts = get_date_to_ts();
 	}
 
-	if (record || extract) {
-		record_data(date2ts, data_flags);
+	if (ctx.record || ctx.extract) {
+		record_data(ctx.date2ts, ctx.data_flags);
 		delete_thread_data();
 	} else
 		print_stats();
@@ -4902,15 +4924,16 @@ void trace_record(int argc, char **argv)
 	tracecmd_remove_instances();
 
 	/* If tracing_on was enabled before we started, set it on now */
-	for_all_instances(instance) {
-		if (instance->keep)
-			write_tracing_on(instance, instance->tracing_on_init_val);
+	for_all_instances(ctx.instance) {
+		if (ctx.instance->keep)
+			write_tracing_on(ctx.instance,
+					 ctx.instance->tracing_on_init_val);
 	}
 
 	if (host)
 		tracecmd_output_close(network_handle);
 
-	if (profile)
+	if (ctx.profile)
 		trace_profile();
 
 	exit(0);
-- 
2.14.1

[PATCH v2 02/10] trace-cmd: Replacing cmd flags w/ a trace_cmd enum

[Vladislav Valtchev (VMware)]

This patch replaces 5 mutually exclusive flag variables (extract, start, stream,
record, profile) in trace_record() with a more convenient enum. The point of
doing that is to make the code simpler and easier to maintain.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 94 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 42 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 487d35e..9e35de4 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4358,7 +4358,16 @@ void trace_reset(int argc, char **argv)
 	exit(0);
 }
 
+enum trace_cmd {
+	CMD_extract,
+	CMD_start,
+	CMD_stream,
+	CMD_profile,
+	CMD_record
+};
+
 struct common_record_context {
+	enum trace_cmd curr_cmd;
 	struct buffer_instance *instance;
 	const char *output;
 	char *date2ts;
@@ -4376,12 +4385,6 @@ struct common_record_context {
 	int topt;
 	int do_child;
 	int run_command;
-
-	int extract;
-	int start;
-	int stream;
-	int profile;
-	int record;
 };
 
 static void init_common_record_context(struct common_record_context *ctx)
@@ -4392,6 +4395,12 @@ static void init_common_record_context(struct common_record_context *ctx)
 	cpu_count = count_cpus();
 }
 
+#define IS_EXTRACT(ctx) ((ctx)->curr_cmd == CMD_extract)
+#define IS_START(ctx) ((ctx)->curr_cmd == CMD_start)
+#define IS_STREAM(ctx) ((ctx)->curr_cmd == CMD_stream)
+#define IS_PROFILE(ctx) ((ctx)->curr_cmd == CMD_profile)
+#define IS_RECORD(ctx) ((ctx)->curr_cmd == CMD_record)
+
 static void parse_record_options(int argc,
 				 char **argv,
 				 struct common_record_context *ctx)
@@ -4426,7 +4435,7 @@ static void parse_record_options(int argc,
 			{NULL, 0, NULL, 0}
 		};
 
-		if (ctx->extract)
+		if (IS_EXTRACT(ctx))
 			opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT";
 		else
 			opts = "+hae:f:Fp:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q";
@@ -4438,7 +4447,7 @@ static void parse_record_options(int argc,
 			usage(argv);
 			break;
 		case 'a':
-			if (ctx->extract) {
+			if (IS_EXTRACT(ctx)) {
 				add_all_instances();
 			} else {
 				ctx->record_all = 1;
@@ -4557,17 +4566,17 @@ static void parse_record_options(int argc,
 		case 'o':
 			if (host)
 				die("-o incompatible with -N");
-			if (ctx->start)
+			if (IS_START(ctx))
 				die("start does not take output\n"
 				    "Did you mean 'record'?");
-			if (ctx->stream)
+			if (IS_STREAM(ctx))
 				die("stream does not take output\n"
 				    "Did you mean 'record'?");
 			if (ctx->output)
 				die("only one output file allowed");
 			ctx->output = optarg;
 
-			if (ctx->profile) {
+			if (IS_PROFILE(ctx)) {
 				int fd;
 
 				/* pipe the output to this file instead of stdout */
@@ -4594,7 +4603,7 @@ static void parse_record_options(int argc,
 			ctx->events = 1;
 			break;
 		case 's':
-			if (ctx->extract) {
+			if (IS_EXTRACT(ctx)) {
 				if (optarg)
 					usage(argv);
 				recorder_flags |= TRACECMD_RECORD_SNAPSHOT;
@@ -4614,7 +4623,7 @@ static void parse_record_options(int argc,
 			rt_prio = atoi(optarg);
 			break;
 		case 'N':
-			if (!ctx->record)
+			if (!IS_RECORD(ctx))
 				die("-N only available with record");
 			if (ctx->output)
 				die("-N incompatible with -o");
@@ -4623,7 +4632,7 @@ static void parse_record_options(int argc,
 		case 'm':
 			if (max_kb)
 				die("-m can only be specified once");
-			if (!ctx->record)
+			if (!IS_RECORD(ctx))
 				die("only record take 'm' option");
 			max_kb = atoi(optarg);
 			break;
@@ -4631,7 +4640,7 @@ static void parse_record_options(int argc,
 			ctx->instance->cpumask = alloc_mask_from_hex(optarg);
 			break;
 		case 't':
-			if (ctx->extract)
+			if (IS_EXTRACT(ctx))
 				ctx->topt = 1; /* Extract top instance also */
 			else
 				use_tcp = 1;
@@ -4644,7 +4653,7 @@ static void parse_record_options(int argc,
 			if (!ctx->instance)
 				die("Failed to create instance");
 			add_instance(ctx->instance);
-			if (ctx->profile)
+			if (IS_PROFILE(ctx))
 				ctx->instance->profile = 1;
 			break;
 		case 'k':
@@ -4722,10 +4731,10 @@ static void parse_record_options(int argc,
 		die(" -c can only be used with -P or -F");
 
 	if ((argc - optind) >= 2) {
-		if (ctx->start)
+		if (IS_START(ctx))
 			die("Command start does not take any commands\n"
 			    "Did you mean 'record'?");
-		if (ctx->extract)
+		if (IS_EXTRACT(ctx))
 			die("Command extract does not take any commands\n"
 			    "Did you mean 'record'?");
 		ctx->run_command = 1;
@@ -4740,15 +4749,16 @@ void trace_record(int argc, char **argv)
 
 	init_common_record_context(&ctx);
 
-	if ((ctx.record = (strcmp(argv[1], "record") == 0)))
-		; /* do nothing */
-	else if ((ctx.start = strcmp(argv[1], "start") == 0))
-		; /* do nothing */
-	else if ((ctx.extract = strcmp(argv[1], "extract") == 0))
-		; /* do nothing */
-	else if ((ctx.stream = strcmp(argv[1], "stream") == 0))
-		; /* do nothing */
-	else if ((ctx.profile = strcmp(argv[1], "profile") == 0)) {
+	if (strcmp(argv[1], "record") == 0)
+		ctx.curr_cmd = CMD_record;
+	else if (strcmp(argv[1], "start") == 0)
+		ctx.curr_cmd = CMD_start;
+	else if (strcmp(argv[1], "extract") == 0)
+		ctx.curr_cmd = CMD_extract;
+	else if (strcmp(argv[1], "stream") == 0)
+		ctx.curr_cmd = CMD_stream;
+	else if (strcmp(argv[1], "profile") == 0) {
+		ctx.curr_cmd = CMD_profile;
 		handle_init = trace_init_profile;
 		ctx.events = 1;
 	} else
@@ -4762,21 +4772,21 @@ void trace_record(int argc, char **argv)
 	 * If this is a profile run, and no instances were set,
 	 * then enable profiling on the top instance.
 	 */
-	if (ctx.profile && !buffer_instances)
+	if (IS_PROFILE(&ctx) && !buffer_instances)
 		top_instance.profile = 1;
 
 	/*
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
 	 */
-	if (!ctx.extract && !__check_doing_something(&top_instance))
+	if (!IS_EXTRACT(&ctx) && !__check_doing_something(&top_instance))
 		first_instance = buffer_instances;
 	else
 		ctx.topt = 1;
 
 	update_first_instance(ctx.instance, ctx.topt);
 
-	if (!ctx.extract)
+	if (!IS_EXTRACT(&ctx))
 		check_doing_something();
 	check_function_plugin();
 
@@ -4796,10 +4806,10 @@ void trace_record(int argc, char **argv)
 	}
 
 	/* Extracting data records all events in the system. */
-	if (ctx.extract && !ctx.record_all)
+	if (IS_EXTRACT(&ctx) && !ctx.record_all)
 		record_all_events();
 
-	if (!ctx.extract)
+	if (!IS_EXTRACT(&ctx))
 		make_instances();
 
 	if (ctx.events)
@@ -4807,7 +4817,7 @@ void trace_record(int argc, char **argv)
 
 	page_size = getpagesize();
 
-	if (!ctx.extract) {
+	if (!IS_EXTRACT(&ctx)) {
 		fset = set_ftrace(!ctx.disable, ctx.total_disable);
 		tracecmd_disable_all_tracing(1);
 
@@ -4815,7 +4825,7 @@ void trace_record(int argc, char **argv)
 			set_clock(ctx.instance);
 
 		/* Record records the date first */
-		if (ctx.record && ctx.date)
+		if (IS_RECORD(&ctx) && ctx.date)
 			ctx.date2ts = get_date_to_ts();
 
 		for_all_instances(ctx.instance) {
@@ -4830,13 +4840,13 @@ void trace_record(int argc, char **argv)
 		set_buffer_size();
 	}
 
-	if (ctx.record)
+	if (IS_RECORD(&ctx))
 		type = TRACE_TYPE_RECORD;
-	else if (ctx.stream)
+	else if (IS_STREAM(&ctx))
 		type = TRACE_TYPE_STREAM;
-	else if (ctx.extract)
+	else if (IS_EXTRACT(&ctx))
 		type = TRACE_TYPE_EXTRACT;
-	else if (ctx.profile)
+	else if (IS_PROFILE(&ctx))
 		type = TRACE_TYPE_STREAM;
 	else
 		type = TRACE_TYPE_START;
@@ -4859,7 +4869,7 @@ void trace_record(int argc, char **argv)
 			start_threads(type, ctx.global);
 	}
 
-	if (ctx.extract) {
+	if (IS_EXTRACT(&ctx)) {
 		flush_threads();
 
 	} else {
@@ -4894,7 +4904,7 @@ void trace_record(int argc, char **argv)
 		tracecmd_disable_all_tracing(0);
 
 	/* extract records the date after extraction */
-	if (ctx.extract && ctx.date) {
+	if (IS_EXTRACT(&ctx) && ctx.date) {
 		/*
 		 * We need to start tracing, don't let other traces
 		 * screw with our trace_marker.
@@ -4903,7 +4913,7 @@ void trace_record(int argc, char **argv)
 		ctx.date2ts = get_date_to_ts();
 	}
 
-	if (ctx.record || ctx.extract) {
+	if (IS_RECORD(&ctx) || IS_EXTRACT(&ctx)) {
 		record_data(ctx.date2ts, ctx.data_flags);
 		delete_thread_data();
 	} else
@@ -4933,7 +4943,7 @@ void trace_record(int argc, char **argv)
 	if (host)
 		tracecmd_output_close(network_handle);
 
-	if (ctx.profile)
+	if (IS_PROFILE(&ctx))
 		trace_profile();
 
 	exit(0);
-- 
2.14.1

[PATCH v2 03/10] trace-cmd: Extracting record_trace()

[Vladislav Valtchev (VMware)]

This patch moves in a separate function almost all the code in trace_record()
after the call of parse_record_options(). This is the last necessary preparation
step before removing the command-multiplexing code from trace_record and
allowing the commands 'start', 'extract', 'stream' and 'profile' to have an
independent entry-point from 'record'.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 146 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 75 insertions(+), 71 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 9e35de4..b37e073 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4742,111 +4742,90 @@ static void parse_record_options(int argc,
 
 }
 
-void trace_record(int argc, char **argv)
+static void record_trace(int argc, char **argv,
+			 struct common_record_context *ctx)
 {
-	struct common_record_context ctx;
 	enum trace_type type = 0;
 
-	init_common_record_context(&ctx);
-
-	if (strcmp(argv[1], "record") == 0)
-		ctx.curr_cmd = CMD_record;
-	else if (strcmp(argv[1], "start") == 0)
-		ctx.curr_cmd = CMD_start;
-	else if (strcmp(argv[1], "extract") == 0)
-		ctx.curr_cmd = CMD_extract;
-	else if (strcmp(argv[1], "stream") == 0)
-		ctx.curr_cmd = CMD_stream;
-	else if (strcmp(argv[1], "profile") == 0) {
-		ctx.curr_cmd = CMD_profile;
-		handle_init = trace_init_profile;
-		ctx.events = 1;
-	} else
-		usage(argv);
-
-
-	parse_record_options(argc, argv, &ctx);
-
-
 	/*
 	 * If this is a profile run, and no instances were set,
 	 * then enable profiling on the top instance.
 	 */
-	if (IS_PROFILE(&ctx) && !buffer_instances)
+	if (IS_PROFILE(ctx) && !buffer_instances)
 		top_instance.profile = 1;
 
 	/*
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
 	 */
-	if (!IS_EXTRACT(&ctx) && !__check_doing_something(&top_instance))
+	if (!IS_EXTRACT(ctx) && !__check_doing_something(&top_instance))
 		first_instance = buffer_instances;
 	else
-		ctx.topt = 1;
+		ctx->topt = 1;
 
-	update_first_instance(ctx.instance, ctx.topt);
+	update_first_instance(ctx->instance, ctx->topt);
 
-	if (!IS_EXTRACT(&ctx))
+	if (!IS_EXTRACT(ctx))
 		check_doing_something();
 	check_function_plugin();
 
-	if (ctx.output)
-		output_file = ctx.output;
+	if (ctx->output)
+		output_file = ctx->output;
 
 	/* Save the state of tracing_on before starting */
-	for_all_instances(ctx.instance) {
+	for_all_instances(ctx->instance) {
 
-		if (!ctx.manual && ctx.instance->profile)
-			enable_profile(ctx.instance);
+		if (!ctx->manual && ctx->instance->profile)
+			enable_profile(ctx->instance);
 
-		ctx.instance->tracing_on_init_val = read_tracing_on(ctx.instance);
+		ctx->instance->tracing_on_init_val = read_tracing_on(ctx->instance);
 		/* Some instances may not be created yet */
-		if (ctx.instance->tracing_on_init_val < 0)
-			ctx.instance->tracing_on_init_val = 1;
+		if (ctx->instance->tracing_on_init_val < 0)
+			ctx->instance->tracing_on_init_val = 1;
 	}
 
 	/* Extracting data records all events in the system. */
-	if (IS_EXTRACT(&ctx) && !ctx.record_all)
+	if (IS_EXTRACT(ctx) && !ctx->record_all)
 		record_all_events();
 
-	if (!IS_EXTRACT(&ctx))
+	if (!IS_EXTRACT(ctx))
 		make_instances();
 
-	if (ctx.events)
+	if (ctx->events)
 		expand_event_list();
 
 	page_size = getpagesize();
 
-	if (!IS_EXTRACT(&ctx)) {
-		fset = set_ftrace(!ctx.disable, ctx.total_disable);
+	if (!IS_EXTRACT(ctx)) {
+		fset = set_ftrace(!ctx->disable, ctx->total_disable);
 		tracecmd_disable_all_tracing(1);
 
-		for_all_instances(ctx.instance)
-			set_clock(ctx.instance);
+		for_all_instances(ctx->instance)
+			set_clock(ctx->instance);
 
 		/* Record records the date first */
-		if (IS_RECORD(&ctx) && ctx.date)
-			ctx.date2ts = get_date_to_ts();
+		if (IS_RECORD(ctx) && ctx->date)
+			ctx->date2ts = get_date_to_ts();
 
-		for_all_instances(ctx.instance) {
-			set_funcs(ctx.instance);
-			set_mask(ctx.instance);
+		for_all_instances(ctx->instance) {
+			set_funcs(ctx->instance);
+			set_mask(ctx->instance);
 		}
 
-		if (ctx.events) {
-			for_all_instances(ctx.instance)
-				enable_events(ctx.instance);
+		if (ctx->events) {
+			for_all_instances(ctx->instance)
+				enable_events(ctx->instance);
 		}
 		set_buffer_size();
 	}
 
-	if (IS_RECORD(&ctx))
+	if (IS_RECORD(ctx))
 		type = TRACE_TYPE_RECORD;
-	else if (IS_STREAM(&ctx))
+	else if (IS_STREAM(ctx))
 		type = TRACE_TYPE_STREAM;
-	else if (IS_EXTRACT(&ctx))
+	else if (IS_EXTRACT(ctx))
 		type = TRACE_TYPE_EXTRACT;
-	else if (IS_PROFILE(&ctx))
+	else if (IS_PROFILE(ctx))
 		type = TRACE_TYPE_STREAM;
 	else
 		type = TRACE_TYPE_START;
@@ -4855,10 +4834,10 @@ void trace_record(int argc, char **argv)
 
 	set_options();
 
-	if (ctx.max_graph_depth) {
-		for_all_instances(ctx.instance)
-			set_max_graph_depth(ctx.instance, ctx.max_graph_depth);
-		free(ctx.max_graph_depth);
+	if (ctx->max_graph_depth) {
+		for_all_instances(ctx->instance)
+			set_max_graph_depth(ctx->instance, ctx->max_graph_depth);
+		free(ctx->max_graph_depth);
 	}
 
 	allocate_seq();
@@ -4866,10 +4845,10 @@ void trace_record(int argc, char **argv)
 	if (type & (TRACE_TYPE_RECORD | TRACE_TYPE_STREAM)) {
 		signal(SIGINT, finish);
 		if (!latency)
-			start_threads(type, ctx.global);
+			start_threads(type, ctx->global);
 	}
 
-	if (IS_EXTRACT(&ctx)) {
+	if (IS_EXTRACT(ctx)) {
 		flush_threads();
 
 	} else {
@@ -4879,7 +4858,7 @@ void trace_record(int argc, char **argv)
 			exit(0);
 		}
 
-		if (ctx.run_command)
+		if (ctx->run_command)
 			run_cmd(type, (argc - optind) - 1, &argv[optind + 1]);
 		else {
 			update_task_filter();
@@ -4904,17 +4883,17 @@ void trace_record(int argc, char **argv)
 		tracecmd_disable_all_tracing(0);
 
 	/* extract records the date after extraction */
-	if (IS_EXTRACT(&ctx) && ctx.date) {
+	if (IS_EXTRACT(ctx) && ctx->date) {
 		/*
 		 * We need to start tracing, don't let other traces
 		 * screw with our trace_marker.
 		 */
 		tracecmd_disable_all_tracing(1);
-		ctx.date2ts = get_date_to_ts();
+		ctx->date2ts = get_date_to_ts();
 	}
 
-	if (IS_RECORD(&ctx) || IS_EXTRACT(&ctx)) {
-		record_data(ctx.date2ts, ctx.data_flags);
+	if (IS_RECORD(ctx) || IS_EXTRACT(ctx)) {
+		record_data(ctx->date2ts, ctx->data_flags);
 		delete_thread_data();
 	} else
 		print_stats();
@@ -4934,17 +4913,42 @@ void trace_record(int argc, char **argv)
 	tracecmd_remove_instances();
 
 	/* If tracing_on was enabled before we started, set it on now */
-	for_all_instances(ctx.instance) {
-		if (ctx.instance->keep)
-			write_tracing_on(ctx.instance,
-					 ctx.instance->tracing_on_init_val);
+	for_all_instances(ctx->instance) {
+		if (ctx->instance->keep)
+			write_tracing_on(ctx->instance,
+					 ctx->instance->tracing_on_init_val);
 	}
 
 	if (host)
 		tracecmd_output_close(network_handle);
 
-	if (IS_PROFILE(&ctx))
+	if (IS_PROFILE(ctx))
 		trace_profile();
 
 	exit(0);
 }
+
+void trace_record(int argc, char **argv)
+{
+	struct common_record_context ctx;
+
+	init_common_record_context(&ctx);
+
+	if (strcmp(argv[1], "record") == 0)
+		ctx.curr_cmd = CMD_record;
+	else if (strcmp(argv[1], "start") == 0)
+		ctx.curr_cmd = CMD_start;
+	else if (strcmp(argv[1], "extract") == 0)
+		ctx.curr_cmd = CMD_extract;
+	else if (strcmp(argv[1], "stream") == 0)
+		ctx.curr_cmd = CMD_stream;
+	else if (strcmp(argv[1], "profile") == 0) {
+		ctx.curr_cmd = CMD_profile;
+		handle_init = trace_init_profile;
+		ctx.events = 1;
+	} else
+		usage(argv);
+
+	parse_record_options(argc, argv, &ctx);
+	record_trace(argc, argv, &ctx);
+}
-- 
2.14.1

[PATCH v2 04/10] trace-cmd: Rename trace_profile() to do_trace_profile()

[Vladislav Valtchev (VMware)]

The purpose of this renaming is to make room for a new trace_profile() function,
that will handle directly the 'profile' command, following the internal
convention COMMAND_NAME -> trace_{COMMAND_NAME}.

Clearly, in the next steps the top-level trace_profile() function will be made
to call do_trace_profile() after some initialization.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-local.h   | 2 +-
 trace-profile.c | 2 +-
 trace-read.c    | 2 +-
 trace-record.c  | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/trace-local.h b/trace-local.h
index c01ef72..fa5232b 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -96,7 +96,7 @@ struct hook_list;
 
 void trace_init_profile(struct tracecmd_input *handle, struct hook_list *hooks,
 			int global);
-int trace_profile(void);
+int do_trace_profile(void);
 void trace_profile_set_merge_like_comms(void);
 
 struct tracecmd_input *
diff --git a/trace-profile.c b/trace-profile.c
index 0af27cd..a2bb3fc 100644
--- a/trace-profile.c
+++ b/trace-profile.c
@@ -2452,7 +2452,7 @@ static void merge_tasks(struct handle_data *h)
 	}
 }
 
-int trace_profile(void)
+int do_trace_profile(void)
 {
 	struct handle_data *h;
 
diff --git a/trace-read.c b/trace-read.c
index 350a843..af5548a 100644
--- a/trace-read.c
+++ b/trace-read.c
@@ -1227,7 +1227,7 @@ static void read_data_info(struct list_head *handle_list, enum output_type otype
 	} while (last_record);
 
 	if (profile)
-		trace_profile();
+		do_trace_profile();
 
 	list_for_each_entry(handles, handle_list, list) {
 		free_filters(handles->event_filters);
diff --git a/trace-record.c b/trace-record.c
index b37e073..827189d 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4923,7 +4923,7 @@ static void record_trace(int argc, char **argv,
 		tracecmd_output_close(network_handle);
 
 	if (IS_PROFILE(ctx))
-		trace_profile();
+		do_trace_profile();
 
 	exit(0);
 }
-- 
2.14.1

[PATCH v2 05/10] trace-cmd: Making start,extract,stream,profile separate funcs

[Vladislav Valtchev (VMware)]

This simple patch make the above-mentioned commands independent from 'record'.
The point of doing so is to follow the convention of one entry-point per command
that is followed by most of trace-cmd's code. Ultimately that aims to prevent
the complexity to concentrate in a single point, making the code simpler to
maintain.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-cmd.c    |  8 +++----
 trace-local.h  |  8 +++++++
 trace-record.c | 72 ++++++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/trace-cmd.c b/trace-cmd.c
index dd1c108..6c2efa3 100644
--- a/trace-cmd.c
+++ b/trace-cmd.c
@@ -102,11 +102,11 @@ struct command commands[] = {
 	{"stack", trace_stack},
 	{"check-events", trace_check_events},
 	{"record", trace_record},
-	{"start", trace_record},
-	{"extract", trace_record},
+	{"start", trace_start},
+	{"extract", trace_extract},
 	{"stop", trace_stop},
-	{"stream", trace_record},
-	{"profile", trace_record},
+	{"stream", trace_stream},
+	{"profile", trace_profile},
 	{"restart", trace_restart},
 	{"reset", trace_reset},
 	{"stat", trace_stat},
diff --git a/trace-local.h b/trace-local.h
index fa5232b..eaae430 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -64,6 +64,14 @@ void trace_restart(int argc, char **argv);
 
 void trace_reset(int argc, char **argv);
 
+void trace_start(int argc, char **argv);
+
+void trace_extract(int argc, char **argv);
+
+void trace_stream(int argc, char **argv);
+
+void trace_profile(int argc, char **argv);
+
 void trace_report(int argc, char **argv);
 
 void trace_split(int argc, char **argv);
diff --git a/trace-record.c b/trace-record.c
index 827189d..6c12416 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4387,10 +4387,12 @@ struct common_record_context {
 	int run_command;
 };
 
-static void init_common_record_context(struct common_record_context *ctx)
+static void init_common_record_context(struct common_record_context *ctx,
+				       enum trace_cmd curr_cmd)
 {
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->instance = &top_instance;
+	ctx->curr_cmd = curr_cmd;
 	init_instance(ctx->instance);
 	cpu_count = count_cpus();
 }
@@ -4739,9 +4741,12 @@ static void parse_record_options(int argc,
 			    "Did you mean 'record'?");
 		ctx->run_command = 1;
 	}
-
 }
 
+/*
+ * This function contains common code for the following commands:
+ * record, start, extract, stream, profile.
+ */
 static void record_trace(int argc, char **argv,
 			 struct common_record_context *ctx)
 {
@@ -4901,7 +4906,7 @@ static void record_trace(int argc, char **argv,
 	destroy_stats();
 
 	if (keep)
-		exit(0);
+		return;
 
 	update_reset_files();
 	update_reset_triggers();
@@ -4924,7 +4929,49 @@ static void record_trace(int argc, char **argv,
 
 	if (IS_PROFILE(ctx))
 		do_trace_profile();
+}
 
+void trace_start(int argc, char **argv)
+{
+	struct common_record_context ctx;
+
+	init_common_record_context(&ctx, CMD_start);
+	parse_record_options(argc, argv, &ctx);
+	record_trace(argc, argv, &ctx);
+	exit(0);
+}
+
+void trace_extract(int argc, char **argv)
+{
+	struct common_record_context ctx;
+
+	init_common_record_context(&ctx, CMD_extract);
+	parse_record_options(argc, argv, &ctx);
+	record_trace(argc, argv, &ctx);
+	exit(0);
+}
+
+void trace_stream(int argc, char **argv)
+{
+	struct common_record_context ctx;
+
+	init_common_record_context(&ctx, CMD_stream);
+	parse_record_options(argc, argv, &ctx);
+	record_trace(argc, argv, &ctx);
+	exit(0);
+}
+
+void trace_profile(int argc, char **argv)
+{
+	struct common_record_context ctx;
+
+	init_common_record_context(&ctx, CMD_profile);
+
+	handle_init = trace_init_profile;
+	ctx.events = 1;
+
+	parse_record_options(argc, argv, &ctx);
+	record_trace(argc, argv, &ctx);
 	exit(0);
 }
 
@@ -4932,23 +4979,8 @@ void trace_record(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx);
-
-	if (strcmp(argv[1], "record") == 0)
-		ctx.curr_cmd = CMD_record;
-	else if (strcmp(argv[1], "start") == 0)
-		ctx.curr_cmd = CMD_start;
-	else if (strcmp(argv[1], "extract") == 0)
-		ctx.curr_cmd = CMD_extract;
-	else if (strcmp(argv[1], "stream") == 0)
-		ctx.curr_cmd = CMD_stream;
-	else if (strcmp(argv[1], "profile") == 0) {
-		ctx.curr_cmd = CMD_profile;
-		handle_init = trace_init_profile;
-		ctx.events = 1;
-	} else
-		usage(argv);
-
+	init_common_record_context(&ctx, CMD_record);
 	parse_record_options(argc, argv, &ctx);
 	record_trace(argc, argv, &ctx);
+	exit(0);
 }
-- 
2.14.1

[PATCH v2 06/10] trace-cmd: Extr. profile-specific code from record_trace

[Vladislav Valtchev (VMware)]

This patch extracts the code in record_trace() under if (IS_PROFILE(ctx)): now
that profile has its own function, that code could be moved there after removing
the IS_PROFILE(ctx) checks, clearly.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 6c12416..55e81cf 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4752,13 +4752,6 @@ static void record_trace(int argc, char **argv,
 {
 	enum trace_type type = 0;
 
-	/*
-	 * If this is a profile run, and no instances were set,
-	 * then enable profiling on the top instance.
-	 */
-	if (IS_PROFILE(ctx) && !buffer_instances)
-		top_instance.profile = 1;
-
 	/*
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
@@ -4926,9 +4919,6 @@ static void record_trace(int argc, char **argv,
 
 	if (host)
 		tracecmd_output_close(network_handle);
-
-	if (IS_PROFILE(ctx))
-		do_trace_profile();
 }
 
 void trace_start(int argc, char **argv)
@@ -4971,7 +4961,15 @@ void trace_profile(int argc, char **argv)
 	ctx.events = 1;
 
 	parse_record_options(argc, argv, &ctx);
+
+	/*
+	 * If no instances were set, then enable profiling on the top instance.
+	 */
+	if (!buffer_instances)
+		top_instance.profile = 1;
+
 	record_trace(argc, argv, &ctx);
+	do_trace_profile();
 	exit(0);
 }
 
-- 
2.14.1

Re: [PATCH v2 06/10] trace-cmd: Extr. profile-specific code from record_trace

[Steven Rostedt]

I modified the subject to:

 trace-cmd: Extract profile-specific code from record_trace

Let's not abbreviate in the subject if we don't need to. It makes it
harder to read.

Thanks,

-- Steve

Re: [PATCH v2 06/10] trace-cmd: Extr. profile-specific code from record_trace

[Vladislav Valtchev]

On Thu, 2017-11-30 at 12:05 -0500, Steven Rostedt wrote:
> I modified the subject to:
> 
>  trace-cmd: Extract profile-specific code from record_trace
> 
> Let's not abbreviate in the subject if we don't need to. It makes it
> harder to read.
> 

I'm totally fine with not abbreviating so much the subject!

I did that because of posts like this:

https://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting

and because vim shows in yellow the first 50 characters of the summary, to help
people following that limitation. In order words, I've understood that I shall
try to fit in 50 chars and never exceed 72 chars.

That lead me to desperate attempts to save characters, like in patch 7 where
I tried to abbreviate "move" with "mov", but I couldn't add a dot for obvious
reasons. [Until now, I was used to make the summary fit in 80 characters.]

What is the convention for trace-cmd?
In particular, what's the hard-limit I shall never exceed ?

Vlad

[PATCH v2 07/10] trace-cmd: Mov init_common_record_context in parse_record_options

[Vladislav Valtchev (VMware)]

This short patch moves init_common_record_context() into parse_record_option()
after checking that no trace_* function in trace-record.c really needs to do
work after init_common_record_context() but before parse_record_option().
In particular, two statements in trace_profile() have been moved after the
call of parse_record_option(), since they did not actually affect it.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 55e81cf..7688565 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4405,6 +4405,7 @@ static void init_common_record_context(struct common_record_context *ctx,
 
 static void parse_record_options(int argc,
 				 char **argv,
+				 enum trace_cmd curr_cmd,
 				 struct common_record_context *ctx)
 {
 	const char *plugin = NULL;
@@ -4416,6 +4417,8 @@ static void parse_record_options(int argc,
 	char *sav;
 	int neg_event = 0;
 
+	init_common_record_context(ctx, curr_cmd);
+
 	for (;;) {
 		int option_index = 0;
 		int ret;
@@ -4925,8 +4928,7 @@ void trace_start(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx, CMD_start);
-	parse_record_options(argc, argv, &ctx);
+	parse_record_options(argc, argv, CMD_start, &ctx);
 	record_trace(argc, argv, &ctx);
 	exit(0);
 }
@@ -4935,8 +4937,7 @@ void trace_extract(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx, CMD_extract);
-	parse_record_options(argc, argv, &ctx);
+	parse_record_options(argc, argv, CMD_extract, &ctx);
 	record_trace(argc, argv, &ctx);
 	exit(0);
 }
@@ -4945,8 +4946,7 @@ void trace_stream(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx, CMD_stream);
-	parse_record_options(argc, argv, &ctx);
+	parse_record_options(argc, argv, CMD_stream, &ctx);
 	record_trace(argc, argv, &ctx);
 	exit(0);
 }
@@ -4955,13 +4955,11 @@ void trace_profile(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx, CMD_profile);
+	parse_record_options(argc, argv, CMD_profile, &ctx);
 
 	handle_init = trace_init_profile;
 	ctx.events = 1;
 
-	parse_record_options(argc, argv, &ctx);
-
 	/*
 	 * If no instances were set, then enable profiling on the top instance.
 	 */
@@ -4977,8 +4975,7 @@ void trace_record(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx, CMD_record);
-	parse_record_options(argc, argv, &ctx);
+	parse_record_options(argc, argv, CMD_record, &ctx);
 	record_trace(argc, argv, &ctx);
 	exit(0);
 }
-- 
2.14.1

Re: [PATCH v2 07/10] trace-cmd: Mov init_common_record_context in parse_record_options

[Steven Rostedt]

Same here. It's "Move" not "Mov" ;-)

I made the modification while pulling in the patch.

-- Steve

Re: [PATCH v2 07/10] trace-cmd: Mov init_common_record_context in parse_record_options

[Vladislav Valtchev]

On Thu, 2017-11-30 at 12:07 -0500, Steven Rostedt wrote:
> Same here. It's "Move" not "Mov" ;-)
> 

Yep, that was a desperate attempt to save just one character :-)

I couldn't abbreviate the function names nor skip "in" nor
the "trace-cmd: " prefix.

Only the poor "Move" remained.

I couldn't even add a dot because:
	len("move") == len("mov.")

Sad story.

> I made the modification while pulling in the patch.
> 

Thanks for doing that. "Move" it's by far better!


Vlad

[PATCH v2 08/10] trace-cmd: Introducing get_trace_cmd_type()

[Vladislav Valtchev (VMware)]

This patch aims to reduce the size of common_record_commads_code() by removing
a relatively long 'else if' sequence that was used to do the mapping between the
current trace command and the trace_type used by it.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 7688565..ec0eaed 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4746,6 +4746,27 @@ static void parse_record_options(int argc,
 	}
 }
 
+static enum trace_type get_trace_cmd_type(enum trace_cmd cmd)
+{
+	const static struct {
+		enum trace_cmd cmd;
+		enum trace_type ttype;
+	} trace_type_per_command[] = {
+		{CMD_record, TRACE_TYPE_RECORD},
+		{CMD_stream, TRACE_TYPE_STREAM},
+		{CMD_extract, TRACE_TYPE_EXTRACT},
+		{CMD_profile, TRACE_TYPE_STREAM},
+		{CMD_start, TRACE_TYPE_START}
+	};
+
+	for (int i = 0; i < ARRAY_SIZE(trace_type_per_command); i++) {
+		if (trace_type_per_command[i].cmd == cmd)
+			return trace_type_per_command[i].ttype;
+	}
+
+	die("Trace type UNKNOWN for the given cmd_fun");
+}
+
 /*
  * This function contains common code for the following commands:
  * record, start, extract, stream, profile.
@@ -4753,7 +4774,7 @@ static void parse_record_options(int argc,
 static void record_trace(int argc, char **argv,
 			 struct common_record_context *ctx)
 {
-	enum trace_type type = 0;
+	enum trace_type type = get_trace_cmd_type(ctx->curr_cmd);
 
 	/*
 	 * If top_instance doesn't have any plugins or events, then
@@ -4820,17 +4841,6 @@ static void record_trace(int argc, char **argv,
 		set_buffer_size();
 	}
 
-	if (IS_RECORD(ctx))
-		type = TRACE_TYPE_RECORD;
-	else if (IS_STREAM(ctx))
-		type = TRACE_TYPE_STREAM;
-	else if (IS_EXTRACT(ctx))
-		type = TRACE_TYPE_EXTRACT;
-	else if (IS_PROFILE(ctx))
-		type = TRACE_TYPE_STREAM;
-	else
-		type = TRACE_TYPE_START;
-
 	update_plugins(type);
 
 	set_options();
-- 
2.14.1

[PATCH v2 09/10] trace-cmd: Extract finalize_record_trace()

[Vladislav Valtchev (VMware)]

This patch splits record_trace() in two parts by moving its finalization part
in a separate function. This will also allow splitting out trace-cmd extract
code from trace-cmd record code, by using a shared function.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index ec0eaed..749c205 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4767,6 +4767,31 @@ static enum trace_type get_trace_cmd_type(enum trace_cmd cmd)
 	die("Trace type UNKNOWN for the given cmd_fun");
 }
 
+static void finalize_record_trace(struct common_record_context *ctx)
+{
+	if (keep)
+		return;
+
+	update_reset_files();
+	update_reset_triggers();
+	if (clear_function_filters)
+		clear_func_filters();
+
+	set_plugin("nop");
+
+	tracecmd_remove_instances();
+
+	/* If tracing_on was enabled before we started, set it on now */
+	for_all_instances(ctx->instance) {
+		if (ctx->instance->keep)
+			write_tracing_on(ctx->instance,
+					 ctx->instance->tracing_on_init_val);
+	}
+
+	if (host)
+		tracecmd_output_close(network_handle);
+}
+
 /*
  * This function contains common code for the following commands:
  * record, start, extract, stream, profile.
@@ -4910,28 +4935,7 @@ static void record_trace(int argc, char **argv,
 		print_stats();
 
 	destroy_stats();
-
-	if (keep)
-		return;
-
-	update_reset_files();
-	update_reset_triggers();
-	if (clear_function_filters)
-		clear_func_filters();
-
-	set_plugin("nop");
-
-	tracecmd_remove_instances();
-
-	/* If tracing_on was enabled before we started, set it on now */
-	for_all_instances(ctx->instance) {
-		if (ctx->instance->keep)
-			write_tracing_on(ctx->instance,
-					 ctx->instance->tracing_on_init_val);
-	}
-
-	if (host)
-		tracecmd_output_close(network_handle);
+	finalize_record_trace(ctx);
 }
 
 void trace_start(int argc, char **argv)
-- 
2.14.1

[PATCH v2 10/10] trace-cmd: Fork record_trace() for the extract case

[Vladislav Valtchev (VMware)]

This patch inlines record_trace() into trace_extract() by removing the code not
related to extract, by replacing IS_EXTRACT(ctx) with true and then removing the
dead code, as well as the if statements when their condition always evalutates
to true. The opposite change [IS_EXTRACT(ctx) evaluated as false] has been
applyed to record_trace().

The purpose of doing that is to reduce the amount of branches in both the cases
(extract and everything else), making the code simpler to understand and follow
but at the price of having some copy-pasted code.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 trace-record.c | 172 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 102 insertions(+), 70 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 749c205..48426e5 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4794,7 +4794,7 @@ static void finalize_record_trace(struct common_record_context *ctx)
 
 /*
  * This function contains common code for the following commands:
- * record, start, extract, stream, profile.
+ * record, start, stream, profile.
  */
 static void record_trace(int argc, char **argv,
 			 struct common_record_context *ctx)
@@ -4805,15 +4805,13 @@ static void record_trace(int argc, char **argv,
 	 * If top_instance doesn't have any plugins or events, then
 	 * remove it from being processed.
 	 */
-	if (!IS_EXTRACT(ctx) && !__check_doing_something(&top_instance))
+	if (!__check_doing_something(&top_instance))
 		first_instance = buffer_instances;
 	else
 		ctx->topt = 1;
 
 	update_first_instance(ctx->instance, ctx->topt);
-
-	if (!IS_EXTRACT(ctx))
-		check_doing_something();
+	check_doing_something();
 	check_function_plugin();
 
 	if (ctx->output)
@@ -4831,43 +4829,35 @@ static void record_trace(int argc, char **argv,
 			ctx->instance->tracing_on_init_val = 1;
 	}
 
-	/* Extracting data records all events in the system. */
-	if (IS_EXTRACT(ctx) && !ctx->record_all)
-		record_all_events();
-
-	if (!IS_EXTRACT(ctx))
-		make_instances();
+	make_instances();
 
 	if (ctx->events)
 		expand_event_list();
 
 	page_size = getpagesize();
 
-	if (!IS_EXTRACT(ctx)) {
-		fset = set_ftrace(!ctx->disable, ctx->total_disable);
-		tracecmd_disable_all_tracing(1);
+	fset = set_ftrace(!ctx->disable, ctx->total_disable);
+	tracecmd_disable_all_tracing(1);
 
+	for_all_instances(ctx->instance)
+		set_clock(ctx->instance);
+
+	/* Record records the date first */
+	if (IS_RECORD(ctx) && ctx->date)
+		ctx->date2ts = get_date_to_ts();
+
+	for_all_instances(ctx->instance) {
+		set_funcs(ctx->instance);
+		set_mask(ctx->instance);
+	}
+
+	if (ctx->events) {
 		for_all_instances(ctx->instance)
-			set_clock(ctx->instance);
-
-		/* Record records the date first */
-		if (IS_RECORD(ctx) && ctx->date)
-			ctx->date2ts = get_date_to_ts();
-
-		for_all_instances(ctx->instance) {
-			set_funcs(ctx->instance);
-			set_mask(ctx->instance);
-		}
-
-		if (ctx->events) {
-			for_all_instances(ctx->instance)
-				enable_events(ctx->instance);
-		}
-		set_buffer_size();
+			enable_events(ctx->instance);
 	}
 
+	set_buffer_size();
 	update_plugins(type);
-
 	set_options();
 
 	if (ctx->max_graph_depth) {
@@ -4882,53 +4872,36 @@ static void record_trace(int argc, char **argv,
 		signal(SIGINT, finish);
 		if (!latency)
 			start_threads(type, ctx->global);
-	}
-
-	if (IS_EXTRACT(ctx)) {
-		flush_threads();
-
 	} else {
-		if (!(type & (TRACE_TYPE_RECORD | TRACE_TYPE_STREAM))) {
-			update_task_filter();
-			tracecmd_enable_tracing();
-			exit(0);
-		}
-
-		if (ctx->run_command)
-			run_cmd(type, (argc - optind) - 1, &argv[optind + 1]);
-		else {
-			update_task_filter();
-			tracecmd_enable_tracing();
-			/* We don't ptrace ourself */
-			if (do_ptrace && filter_pid >= 0)
-				ptrace_attach(filter_pid);
-			/* sleep till we are woken with Ctrl^C */
-			printf("Hit Ctrl^C to stop recording\n");
-			while (!finished)
-				trace_or_sleep(type);
-		}
+		update_task_filter();
+		tracecmd_enable_tracing();
+		exit(0);
+	}
 
-		tracecmd_disable_tracing();
-		if (!latency)
-			stop_threads(type);
+	if (ctx->run_command)
+		run_cmd(type, (argc - optind) - 1, &argv[optind + 1]);
+	else {
+		update_task_filter();
+		tracecmd_enable_tracing();
+		/* We don't ptrace ourself */
+		if (do_ptrace && filter_pid >= 0)
+			ptrace_attach(filter_pid);
+		/* sleep till we are woken with Ctrl^C */
+		printf("Hit Ctrl^C to stop recording\n");
+		while (!finished)
+			trace_or_sleep(type);
 	}
 
+	tracecmd_disable_tracing();
+	if (!latency)
+		stop_threads(type);
+
 	record_stats();
 
 	if (!keep)
 		tracecmd_disable_all_tracing(0);
 
-	/* extract records the date after extraction */
-	if (IS_EXTRACT(ctx) && ctx->date) {
-		/*
-		 * We need to start tracing, don't let other traces
-		 * screw with our trace_marker.
-		 */
-		tracecmd_disable_all_tracing(1);
-		ctx->date2ts = get_date_to_ts();
-	}
-
-	if (IS_RECORD(ctx) || IS_EXTRACT(ctx)) {
+	if (IS_RECORD(ctx)) {
 		record_data(ctx->date2ts, ctx->data_flags);
 		delete_thread_data();
 	} else
@@ -4950,9 +4923,68 @@ void trace_start(int argc, char **argv)
 void trace_extract(int argc, char **argv)
 {
 	struct common_record_context ctx;
+	enum trace_type type;
 
 	parse_record_options(argc, argv, CMD_extract, &ctx);
-	record_trace(argc, argv, &ctx);
+
+	type = get_trace_cmd_type(ctx.curr_cmd);
+
+	update_first_instance(ctx.instance, 1);
+	check_function_plugin();
+
+	if (ctx.output)
+		output_file = ctx.output;
+
+	/* Save the state of tracing_on before starting */
+	for_all_instances(ctx.instance) {
+
+		if (!ctx.manual && ctx.instance->profile)
+			enable_profile(ctx.instance);
+
+		ctx.instance->tracing_on_init_val = read_tracing_on(ctx.instance);
+		/* Some instances may not be created yet */
+		if (ctx.instance->tracing_on_init_val < 0)
+			ctx.instance->tracing_on_init_val = 1;
+	}
+
+	/* Extracting data records all events in the system. */
+	if (!ctx.record_all)
+		record_all_events();
+
+	if (ctx.events)
+		expand_event_list();
+
+	page_size = getpagesize();
+	update_plugins(type);
+	set_options();
+
+	if (ctx.max_graph_depth) {
+		for_all_instances(ctx.instance)
+			set_max_graph_depth(ctx.instance, ctx.max_graph_depth);
+		free(ctx.max_graph_depth);
+	}
+
+	allocate_seq();
+	flush_threads();
+	record_stats();
+
+	if (!keep)
+		tracecmd_disable_all_tracing(0);
+
+	/* extract records the date after extraction */
+	if (ctx.date) {
+		/*
+		 * We need to start tracing, don't let other traces
+		 * screw with our trace_marker.
+		 */
+		tracecmd_disable_all_tracing(1);
+		ctx.date2ts = get_date_to_ts();
+	}
+
+	record_data(ctx.date2ts, ctx.data_flags);
+	delete_thread_data();
+	destroy_stats();
+	finalize_record_trace(&ctx);
 	exit(0);
 }
 
-- 
2.14.1

Re: [PATCH v2 00/10] trace-cmd: Refactoring trace_record()

[Steven Rostedt]

On Thu, 30 Nov 2017 15:19:47 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> The following series of patches is a refactoring of trace_record() trying to
> reduce as much as possible its complexity but, at the same time, without making
> risky changes. All the patches are relatively small and potentially no-brainer
> steps towards the final shape of the code.
> 
> The rationale for this effort is to make the code much easier to maintain.
> 
> Vladislav Valtchev (VMware) (10):
>   trace-cmd: Extract parse_record_options() from trace_record()
>   trace-cmd: Replacing cmd flags w/ a trace_cmd enum
>   trace-cmd: Extracting record_trace()
>   trace-cmd: Rename trace_profile() to do_trace_profile()
>   trace-cmd: Making start,extract,stream,profile separate funcs
>   trace-cmd: Extr. profile-specific code from record_trace
>   trace-cmd: Mov init_common_record_context in parse_record_options
>   trace-cmd: Introducing get_trace_cmd_type()
>   trace-cmd: Extract finalize_record_trace()
>   trace-cmd: Fork record_trace() for the extract case

Hi Vlad,

I applied and pushed all the patches in this series.

Thanks!

-- Steve

> 
>  trace-cmd.c     |   8 +-
>  trace-local.h   |  10 +-
>  trace-profile.c |   2 +-
>  trace-read.c    |   2 +-
>  trace-record.c  | 640 +++++++++++++++++++++++++++++++++-----------------------
>  5 files changed, 390 insertions(+), 272 deletions(-)
> 

Re: [PATCH v2 00/10] trace-cmd: Refactoring trace_record()

[Vladislav Valtchev]

On Thu, 2017-11-30 at 16:37 -0500, Steven Rostedt wrote:
> Hi Vlad,
> 
> I applied and pushed all the patches in this series.
> 
> Thanks!
> 

That's great!
Thanks a lot to you, Steven!

Vlad