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

[PATCH 00/11] 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) (11):
  trace-cmd: Extract trace_stop() from trace_record()
  trace-cmd: Extract trace_restart() from trace_record()
  trace-cmd: Extract trace_reset() from trace_record()
  trace-cmd: Extract parse_record_options() from trace_record()
  trace-cmd: Rename trace_profile() to trace_profile_int()
  trace-cmd: Replacing cmd flags w/ a trace_cmd enum
  trace-cmd: Extracting record_trace()
  trace-cmd: Making start,extract,stream,profile separate funcs
  trace-cmd: Consolidate ARRAY_SIZE() in trace-cmd.h
  trace-cmd: Making the "die" functions noreturn
  trace-cmd: Introducing get_trace_cmd_type()

 plugin_blk.c    |   1 -
 trace-cmd.c     |  16 +-
 trace-cmd.h     |   4 +
 trace-local.h   |  22 +-
 trace-profile.c |   2 +-
 trace-read.c    |   2 +-
 trace-record.c  | 715 ++++++++++++++++++++++++++++++++------------------------
 trace-util.c    |   8 +-
 8 files changed, 443 insertions(+), 327 deletions(-)

-- 
2.14.1

[PATCH 01/11] trace-cmd: Extract trace_stop() from trace_record()

[Vladislav Valtchev (VMware)]

In this patch, an entire trace_stop() function has been extracted from
trace_record() and added to the commands table in trace-cmd.c, without any
ad-hoc changes. Now trace_record() is a little bit smaller and all the code
handling the 'stop' command is in a dedicated function.

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

diff --git a/trace-cmd.c b/trace-cmd.c
index 5a10605..de5283c 100644
--- a/trace-cmd.c
+++ b/trace-cmd.c
@@ -104,7 +104,7 @@ struct command commands[] = {
 	{"record", trace_record},
 	{"start", trace_record},
 	{"extract", trace_record},
-	{"stop", trace_record},
+	{"stop", trace_stop},
 	{"stream", trace_record},
 	{"profile", trace_record},
 	{"restart", trace_record},
diff --git a/trace-local.h b/trace-local.h
index 157b2c6..8010680 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -58,6 +58,8 @@ int read_trace_files(void);
 
 void trace_record(int argc, char **argv);
 
+void trace_stop(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 06fc0e6..c7f7df7 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -2141,7 +2141,7 @@ static void set_mask(struct buffer_instance *instance)
 		die("could not open %s\n", path);
 
 	write(fd, instance->cpumask, strlen(instance->cpumask));
-	
+
 	close(fd);
  out:
 	tracecmd_put_tracing_file(path);
@@ -4198,7 +4198,48 @@ enum {
 	OPT_module		= 256,
 };
 
-void trace_record (int argc, char **argv)
+void trace_stop(int argc, char **argv)
+{
+	int topt = 0;
+	struct buffer_instance *instance = &top_instance;
+
+	init_instance(instance);
+
+	for (;;) {
+		int c;
+
+		c = getopt(argc-1, argv+1, "hatB:");
+		if (c == -1)
+			break;
+		switch (c) {
+		case 'h':
+			usage(argv);
+			break;
+		case 'B':
+			instance = create_instance(optarg);
+			if (!instance)
+				die("Failed to create instance");
+			add_instance(instance);
+			break;
+		case 'a':
+			add_all_instances();
+			break;
+		case 't':
+			/* Force to use top instance */
+			topt = 1;
+			instance = &top_instance;
+			break;
+		default:
+			usage(argv);
+		}
+
+	}
+	update_first_instance(instance, topt);
+	tracecmd_disable_tracing();
+	exit(0);
+}
+
+void trace_record(int argc, char **argv)
 {
 	const char *plugin = NULL;
 	const char *output = NULL;
@@ -4248,39 +4289,6 @@ void trace_record (int argc, char **argv)
 	else if ((profile = strcmp(argv[1], "profile") == 0)) {
 		handle_init = trace_init_profile;
 		events = 1;
-	} else if (strcmp(argv[1], "stop") == 0) {
-		for (;;) {
-			int c;
-
-			c = getopt(argc-1, argv+1, "hatB:");
-			if (c == -1)
-				break;
-			switch (c) {
-			case 'h':
-				usage(argv);
-				break;
-			case 'B':
-				instance = create_instance(optarg);
-				if (!instance)
-					die("Failed to create instance");
-				add_instance(instance);
-				break;
-			case 'a':
-				add_all_instances();
-				break;
-			case 't':
-				/* Force to use top instance */
-				topt = 1;
-				instance = &top_instance;
-				break;
-			default:
-				usage(argv);
-			}
-
-		}
-		update_first_instance(instance, topt);
-		tracecmd_disable_tracing();
-		exit(0);
 	} else if (strcmp(argv[1], "restart") == 0) {
 		for (;;) {
 			int c;
-- 
2.14.1

[PATCH 02/11] trace-cmd: Extract trace_restart() from trace_record()

[Vladislav Valtchev (VMware)]

In this patch, an entire trace_restart() function has been extracted from
trace_record() and added to the commands table in trace-cmd.c, without any
ad-hoc changes. Now trace_record() is a little bit smaller and all the code
handling the 'restart' command is in a dedicated function.

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

diff --git a/trace-cmd.c b/trace-cmd.c
index de5283c..2b9146e 100644
--- a/trace-cmd.c
+++ b/trace-cmd.c
@@ -107,7 +107,7 @@ struct command commands[] = {
 	{"stop", trace_stop},
 	{"stream", trace_record},
 	{"profile", trace_record},
-	{"restart", trace_record},
+	{"restart", trace_restart},
 	{"reset", trace_record},
 	{"stat", trace_stat},
 	{"options", trace_option},
diff --git a/trace-local.h b/trace-local.h
index 8010680..2b55aa8 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -60,6 +60,8 @@ void trace_record(int argc, char **argv);
 
 void trace_stop(int argc, char **argv);
 
+void trace_restart(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 c7f7df7..e9cce12 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4239,6 +4239,47 @@ void trace_stop(int argc, char **argv)
 	exit(0);
 }
 
+void trace_restart(int argc, char **argv)
+{
+	int topt = 0;
+	struct buffer_instance *instance = &top_instance;
+
+	init_instance(instance);
+
+	for (;;) {
+		int c;
+
+		c = getopt(argc-1, argv+1, "hatB:");
+		if (c == -1)
+			break;
+		switch (c) {
+		case 'h':
+			usage(argv);
+			break;
+		case 'B':
+			instance = create_instance(optarg);
+			if (!instance)
+				die("Failed to create instance");
+			add_instance(instance);
+			break;
+		case 'a':
+			add_all_instances();
+			break;
+		case 't':
+			/* Force to use top instance */
+			topt = 1;
+			instance = &top_instance;
+			break;
+		default:
+			usage(argv);
+		}
+
+	}
+	update_first_instance(instance, topt);
+	tracecmd_enable_tracing();
+	exit(0);
+}
+
 void trace_record(int argc, char **argv)
 {
 	const char *plugin = NULL;
@@ -4289,39 +4330,6 @@ void trace_record(int argc, char **argv)
 	else if ((profile = strcmp(argv[1], "profile") == 0)) {
 		handle_init = trace_init_profile;
 		events = 1;
-	} else if (strcmp(argv[1], "restart") == 0) {
-		for (;;) {
-			int c;
-
-			c = getopt(argc-1, argv+1, "hatB:");
-			if (c == -1)
-				break;
-			switch (c) {
-			case 'h':
-				usage(argv);
-				break;
-			case 'B':
-				instance = create_instance(optarg);
-				if (!instance)
-					die("Failed to create instance");
-				add_instance(instance);
-				break;
-			case 'a':
-				add_all_instances();
-				break;
-			case 't':
-				/* Force to use top instance */
-				topt = 1;
-				instance = &top_instance;
-				break;
-			default:
-				usage(argv);
-			}
-
-		}
-		update_first_instance(instance, topt);
-		tracecmd_enable_tracing();
-		exit(0);
 	} else if (strcmp(argv[1], "reset") == 0) {
 		/* if last arg is -a, then -b and -d apply to all instances */
 		int last_specified_all = 0;
-- 
2.14.1

[PATCH 03/11] trace-cmd: Extract trace_reset() from trace_record()

[Vladislav Valtchev (VMware)]

In this patch, an entire trace_reset() function has been extracted from
trace_record() and added to the commands table in trace-cmd.c, without any
ad-hoc changes. Now trace_record() is a little bit smaller and all the code
handling the 'reset' command is in a dedicated function.

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

diff --git a/trace-cmd.c b/trace-cmd.c
index 2b9146e..7bab476 100644
--- a/trace-cmd.c
+++ b/trace-cmd.c
@@ -108,7 +108,7 @@ struct command commands[] = {
 	{"stream", trace_record},
 	{"profile", trace_record},
 	{"restart", trace_restart},
-	{"reset", trace_record},
+	{"reset", trace_reset},
 	{"stat", trace_stat},
 	{"options", trace_option},
 	{"show", trace_show},
diff --git a/trace-local.h b/trace-local.h
index 2b55aa8..49e7635 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -62,6 +62,8 @@ void trace_stop(int argc, char **argv);
 
 void trace_restart(int argc, char **argv);
 
+void trace_reset(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 e9cce12..8f8d270 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4280,6 +4280,84 @@ void trace_restart(int argc, char **argv)
 	exit(0);
 }
 
+void trace_reset(int argc, char **argv)
+{
+	int c;
+	int topt = 0;
+	struct buffer_instance *instance = &top_instance;
+
+	init_instance(instance);
+
+	/* if last arg is -a, then -b and -d apply to all instances */
+	int last_specified_all = 0;
+	struct buffer_instance *inst; /* iterator */
+
+	while ((c = getopt(argc-1, argv+1, "hab:B:td")) >= 0) {
+
+		switch (c) {
+		case 'h':
+			usage(argv);
+			break;
+		case 'b':
+		{
+			int size = atoi(optarg);
+			/* Min buffer size is 1 */
+			if (size <= 1)
+				size = 1;
+			if (last_specified_all) {
+				for_each_instance(inst) {
+					inst->buffer_size = size;
+				}
+			} else {
+				instance->buffer_size = size;
+			}
+			break;
+		}
+		case 'B':
+			last_specified_all = 0;
+			instance = create_instance(optarg);
+			if (!instance)
+				die("Failed to create instance");
+			add_instance(instance);
+			/* -d will remove keep */
+			instance->keep = 1;
+			break;
+		case 't':
+			/* Force to use top instance */
+			last_specified_all = 0;
+			topt = 1;
+			instance = &top_instance;
+			break;
+		case 'a':
+			last_specified_all = 1;
+			add_all_instances();
+			for_each_instance(instance) {
+				instance->keep = 1;
+			}
+			break;
+		case 'd':
+			if (last_specified_all) {
+				for_each_instance(inst) {
+					inst->keep = 0;
+				}
+			} else {
+				if (is_top_instance(instance))
+					die("Can not delete top level buffer");
+				instance->keep = 0;
+			}
+			break;
+		}
+	}
+	update_first_instance(instance, topt);
+	tracecmd_disable_all_tracing(1);
+	set_buffer_size();
+	clear_filters();
+	clear_triggers();
+	tracecmd_remove_instances();
+	clear_func_filters();
+	exit(0);
+}
+
 void trace_record(int argc, char **argv)
 {
 	const char *plugin = NULL;
@@ -4330,75 +4408,6 @@ void trace_record(int argc, char **argv)
 	else if ((profile = strcmp(argv[1], "profile") == 0)) {
 		handle_init = trace_init_profile;
 		events = 1;
-	} else if (strcmp(argv[1], "reset") == 0) {
-		/* if last arg is -a, then -b and -d apply to all instances */
-		int last_specified_all = 0;
-		struct buffer_instance *inst; /* iterator */
-
-		while ((c = getopt(argc-1, argv+1, "hab:B:td")) >= 0) {
-
-			switch (c) {
-			case 'h':
-				usage(argv);
-				break;
-			case 'b':
-			{
-				int size = atoi(optarg);
-				/* Min buffer size is 1 */
-				if (size <= 1)
-					size = 1;
-				if (last_specified_all) {
-					for_each_instance(inst) {
-						inst->buffer_size = size;
-					}
-				} else {
-					instance->buffer_size = size;
-				}
-				break;
-			}
-			case 'B':
-				last_specified_all = 0;
-				instance = create_instance(optarg);
-				if (!instance)
-					die("Failed to create instance");
-				add_instance(instance);
-				/* -d will remove keep */
-				instance->keep = 1;
-				break;
-			case 't':
-				/* Force to use top instance */
-				last_specified_all = 0;
-				topt = 1;
-				instance = &top_instance;
-				break;
-			case 'a':
-				last_specified_all = 1;
-				add_all_instances();
-				for_each_instance(instance) {
-					instance->keep = 1;
-				}
-				break;
-			case 'd':
-				if (last_specified_all) {
-					for_each_instance(inst) {
-						inst->keep = 0;
-					}
-				} else {
-					if (is_top_instance(instance))
-						die("Can not delete top level buffer");
-					instance->keep = 0;
-				}
-				break;
-			}
-		}
-		update_first_instance(instance, topt);
-		tracecmd_disable_all_tracing(1);
-		set_buffer_size();
-		clear_filters();
-		clear_triggers();
-		tracecmd_remove_instances();
-		clear_func_filters();
-		exit(0);
 	} else
 		usage(argv);
 
-- 
2.14.1

[PATCH 04/11] 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 | 336 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 180 insertions(+), 156 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 8f8d270..6dc828b 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4358,62 +4358,50 @@ 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 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 +4419,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 +4431,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 +4483,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 +4506,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 +4542,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 +4583,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 +4598,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 +4647,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 +4660,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 +4675,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,106 +4705,141 @@ 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;
 	}
 
+}
+
+static void init_common_record_context(struct common_record_context *ctx)
+{
+	memset(ctx, 0, sizeof(*ctx));
+	ctx->instance = &top_instance;
+	cpu_count = count_cpus();
+}
+
+void trace_record(int argc, char **argv)
+{
+	struct common_record_context ctx;
+	enum trace_type type = 0;
+
+	init_common_record_context(&ctx);
+	init_instance(ctx.instance);
+
+	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)) {
 		if (!buffer_instances)
 			die("No instances reference??");
 		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;
@@ -4825,10 +4848,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();
@@ -4836,10 +4859,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 {
@@ -4849,7 +4872,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();
@@ -4874,17 +4897,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();
@@ -4904,15 +4927,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 05/11] trace-cmd: Rename trace_profile() to trace_profile_int()

[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 trace_profile_int() 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 49e7635..83c794a 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 trace_profile_int(void);
 void trace_profile_set_merge_like_comms(void);
 
 struct tracecmd_input *
diff --git a/trace-profile.c b/trace-profile.c
index 0af27cd..777c4f2 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 trace_profile_int(void)
 {
 	struct handle_data *h;
 
diff --git a/trace-read.c b/trace-read.c
index 350a843..862455e 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();
+		trace_profile_int();
 
 	list_for_each_entry(handles, handle_list, list) {
 		free_filters(handles->event_filters);
diff --git a/trace-record.c b/trace-record.c
index 6dc828b..b75d209 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4937,7 +4937,7 @@ void trace_record(int argc, char **argv)
 		tracecmd_output_close(network_handle);
 
 	if (ctx.profile)
-		trace_profile();
+		trace_profile_int();
 
 	exit(0);
 }
-- 
2.14.1

[PATCH 06/11] 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 b75d209..2d74a68 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,14 +4385,14 @@ struct common_record_context {
 	int topt;
 	int do_child;
 	int run_command;
-
-	int extract;
-	int start;
-	int stream;
-	int profile;
-	int record;
 };
 
+#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,
@@ -4419,7 +4428,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";
@@ -4431,7 +4440,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;
@@ -4550,17 +4559,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 */
@@ -4587,7 +4596,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;
@@ -4607,7 +4616,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");
@@ -4616,7 +4625,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;
@@ -4624,7 +4633,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;
@@ -4637,7 +4646,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':
@@ -4715,10 +4724,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;
@@ -4741,15 +4750,16 @@ void trace_record(int argc, char **argv)
 	init_common_record_context(&ctx);
 	init_instance(ctx.instance);
 
-	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
@@ -4763,14 +4773,14 @@ 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)) {
 		if (!buffer_instances)
 			die("No instances reference??");
 		first_instance = buffer_instances;
@@ -4779,7 +4789,7 @@ void trace_record(int argc, char **argv)
 
 	update_first_instance(ctx.instance, ctx.topt);
 
-	if (!ctx.extract)
+	if (!IS_EXTRACT(&ctx))
 		check_doing_something();
 	check_function_plugin();
 
@@ -4799,10 +4809,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)
@@ -4810,7 +4820,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);
 
@@ -4818,7 +4828,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) {
@@ -4833,13 +4843,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;
@@ -4862,7 +4872,7 @@ void trace_record(int argc, char **argv)
 			start_threads(type, ctx.global);
 	}
 
-	if (ctx.extract) {
+	if (IS_EXTRACT(&ctx)) {
 		flush_threads();
 
 	} else {
@@ -4897,7 +4907,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.
@@ -4906,7 +4916,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
@@ -4936,7 +4946,7 @@ void trace_record(int argc, char **argv)
 	if (host)
 		tracecmd_output_close(network_handle);
 
-	if (ctx.profile)
+	if (IS_PROFILE(&ctx))
 		trace_profile_int();
 
 	exit(0);
-- 
2.14.1

[PATCH 07/11] 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 | 148 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 76 insertions(+), 72 deletions(-)

diff --git a/trace-record.c b/trace-record.c
index 2d74a68..916b768 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4742,114 +4742,92 @@ static void init_common_record_context(struct common_record_context *ctx)
 	cpu_count = count_cpus();
 }
 
-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);
-	init_instance(ctx.instance);
-
-	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)) {
 		if (!buffer_instances)
 			die("No instances reference??");
 		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;
@@ -4858,10 +4836,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();
@@ -4869,10 +4847,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 {
@@ -4882,7 +4860,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();
@@ -4907,17 +4885,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();
@@ -4937,17 +4915,43 @@ 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_int();
 
 	exit(0);
 }
+
+void trace_record(int argc, char **argv)
+{
+	struct common_record_context ctx;
+
+	init_common_record_context(&ctx);
+	init_instance(ctx.instance);
+
+	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 08/11] 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 | 102 +++++++++++++++++++++++++++++++++++++--------------------
 3 files changed, 78 insertions(+), 40 deletions(-)

diff --git a/trace-cmd.c b/trace-cmd.c
index 7bab476..7297756 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 83c794a..cb47e78 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 916b768..ae0733a 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4732,28 +4732,17 @@ static void parse_record_options(int argc,
 			    "Did you mean 'record'?");
 		ctx->run_command = 1;
 	}
-
-}
-
-static void init_common_record_context(struct common_record_context *ctx)
-{
-	memset(ctx, 0, sizeof(*ctx));
-	ctx->instance = &top_instance;
-	cpu_count = count_cpus();
 }
 
+/*
+ * 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)
 {
 	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.
@@ -4903,7 +4892,7 @@ static void record_trace(int argc, char **argv,
 	destroy_stats();
 
 	if (keep)
-		exit(0);
+		return;
 
 	update_reset_files();
 	update_reset_triggers();
@@ -4923,10 +4912,67 @@ static void record_trace(int argc, char **argv,
 
 	if (host)
 		tracecmd_output_close(network_handle);
+}
 
-	if (IS_PROFILE(ctx))
-		trace_profile_int();
+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();
+}
 
+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);
+
+	/*
+	 * If no instances were set, then enable profiling on the top instance.
+	 */
+	if (!buffer_instances)
+		top_instance.profile = 1;
+
+	record_trace(argc, argv, &ctx);
+	trace_profile_int();
 	exit(0);
 }
 
@@ -4934,24 +4980,8 @@ void trace_record(int argc, char **argv)
 {
 	struct common_record_context ctx;
 
-	init_common_record_context(&ctx);
-	init_instance(ctx.instance);
-
-	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 09/11] trace-cmd: Consolidate ARRAY_SIZE() in trace-cmd.h

[Vladislav Valtchev (VMware)]

Since ARRAY_SIZE() is a very useful macro, it makes sense to move it in a common
header file, in order to avoid several C files to re-define it.

Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
---
 plugin_blk.c | 1 -
 trace-cmd.c  | 2 --
 trace-cmd.h  | 2 ++
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/plugin_blk.c b/plugin_blk.c
index e87bebb..a1b5df0 100644
--- a/plugin_blk.c
+++ b/plugin_blk.c
@@ -30,7 +30,6 @@
 #define MINORMASK	((1U << MINORBITS) - 1)
 #define MAJOR(dev)	((unsigned int) ((dev) >> MINORBITS))
 #define MINOR(dev)	((unsigned int) ((dev) & MINORMASK))
-#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 
 struct blk_data {
 	unsigned long long	sector;
diff --git a/trace-cmd.c b/trace-cmd.c
index 7297756..6c2efa3 100644
--- a/trace-cmd.c
+++ b/trace-cmd.c
@@ -117,8 +117,6 @@ struct command commands[] = {
 	{"-h", trace_usage},
 };
 
-#define ARRAY_SIZE(_a) (sizeof(_a) / sizeof(_a[0]))
-
 int main (int argc, char **argv)
 {
 	int i;
diff --git a/trace-cmd.h b/trace-cmd.h
index 485907f..3fb6aab 100644
--- a/trace-cmd.h
+++ b/trace-cmd.h
@@ -22,6 +22,8 @@
 
 #include "event-parse.h"
 
+#define ARRAY_SIZE(_a) (sizeof(_a) / sizeof((_a)[0]))
+
 #define TRACECMD_ERR_MSK	((unsigned long)(-1) & ~((1UL << 14) - 1))
 #define TRACECMD_ISERR(ptr)	((unsigned long)(ptr) > TRACECMD_ERR_MSK)
 #define TRACECMD_ERROR(ret)	((void *)((unsigned long)(ret) | TRACECMD_ERR_MSK))
-- 
2.14.1

[PATCH 10/11] trace-cmd: Making the "die" functions noreturn

[Vladislav Valtchev (VMware)]

This patch makes all the "die" functions in trace-cmd noreturn in order we to be
able to use them in negative code paths inside non-void functions, without
getting compiler warnings and, clearly, without work-arounds like non-sense
return statements after a die().

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

diff --git a/trace-cmd.h b/trace-cmd.h
index 3fb6aab..6fd34d7 100644
--- a/trace-cmd.h
+++ b/trace-cmd.h
@@ -23,6 +23,8 @@
 #include "event-parse.h"
 
 #define ARRAY_SIZE(_a) (sizeof(_a) / sizeof((_a)[0]))
+#define __weak __attribute__((weak))
+#define __noreturn __attribute__((noreturn))
 
 #define TRACECMD_ERR_MSK	((unsigned long)(-1) & ~((1UL << 14) - 1))
 #define TRACECMD_ISERR(ptr)	((unsigned long)(ptr) > TRACECMD_ERR_MSK)
diff --git a/trace-local.h b/trace-local.h
index cb47e78..a06dc9b 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -212,9 +212,9 @@ void show_instance_file(struct buffer_instance *instance, const char *name);
 int count_cpus(void);
 
 /* No longer in event-utils.h */
-void die(const char *fmt, ...); /* Can be overriden */
+void __noreturn die(const char *fmt, ...); /* Can be overriden */
 void *malloc_or_die(unsigned int size); /* Can be overridden */
-void __die(const char *fmt, ...);
-void __vdie(const char *fmt, va_list ap);
+void __noreturn __die(const char *fmt, ...);
+void __noreturn _vdie(const char *fmt, va_list ap);
 
 #endif /* __TRACE_LOCAL_H */
diff --git a/trace-util.c b/trace-util.c
index 45fa95a..0f53e16 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -1613,7 +1613,7 @@ void tracecmd_put_tracing_file(char *name)
 	free(name);
 }
 
-void __vdie(const char *fmt, va_list ap)
+void __noreturn __vdie(const char *fmt, va_list ap)
 {
 	int ret = errno;
 
@@ -1629,7 +1629,7 @@ void __vdie(const char *fmt, va_list ap)
 	exit(ret);
 }
 
-void __die(const char *fmt, ...)
+void __noreturn __die(const char *fmt, ...)
 {
 	va_list ap;
 
@@ -1638,9 +1638,7 @@ void __die(const char *fmt, ...)
 	va_end(ap);
 }
 
-#define __weak __attribute__((weak))
-
-void __weak die(const char *fmt, ...)
+void __weak __noreturn die(const char *fmt, ...)
 {
 	va_list ap;
 
-- 
2.14.1

[PATCH 11/11] 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 ae0733a..cc75b11 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -4734,6 +4734,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.
@@ -4741,7 +4762,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
@@ -4810,17 +4831,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

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Steven Rostedt]

On Thu, 23 Nov 2017 18:33:28 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> 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().

Hi Vladislav,

I applied patches 1-3.

> 
> Signed-off-by: Vladislav Valtchev (VMware) <vladislav.valtchev@gmail.com>
> ---
> +
> +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 +4419,7 @@ void trace_record(int argc, char **argv)
> 


+
> +static void init_common_record_context(struct common_record_context *ctx)
> +{
> +	memset(ctx, 0, sizeof(*ctx));
> +	ctx->instance = &top_instance;
> +	cpu_count = count_cpus();
> +}
> +
> +void trace_record(int argc, char **argv)
> +{
> +	struct common_record_context ctx;
> +	enum trace_type type = 0;
> +
> +	init_common_record_context(&ctx);
> +	init_instance(ctx.instance);

Is there a reason that init_instance() isn't called in
init_common_record_context()?

Also, why not just do the "init_common_record_context()" in
parse_record_options()?

-- Steve

> +
> +	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.
>  	 */

Re: [PATCH 05/11] trace-cmd: Rename trace_profile() to trace_profile_int()

[Steven Rostedt]

On Thu, 23 Nov 2017 18:33:29 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> 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}.

Couple of things.

1) A change like this should always be directly before the patch that
requires it. This is patch 5, and it's not until patch 8 that this
change is required. Please keep them next to each other. This should
have been patch 7.

2) trace_profile_int() is confusing. I keep thinking it's doing
something with integers. Call it do_trace_profile().

Thanks!

-- Steve

Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Steven Rostedt]

On Thu, 23 Nov 2017 18:33:32 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:


> +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();
> +}
>  
> +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;

Why setting 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);
> +	trace_profile_int();
>  	exit(0);
>  }
>  
> @@ -4934,24 +4980,8 @@ void trace_record(int argc, char **argv)
>  {
>  	struct common_record_context ctx;
>  
> -	init_common_record_context(&ctx);
> -	init_instance(ctx.instance);
> -
> -	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);

As everything is doing both init_common_record_context() and
parse_record_options() why not just move the
init_common_record_context() into parse_record_options(). If you need
to do something special (like set events = 1 for profile) have that
done in parse_record_options() if the cmd is CMD_record. Yes, you need
to pass the CMD_* to parse_record_options() in this case.

-- Steve


>  	record_trace(argc, argv, &ctx);
> +	exit(0);
>  }

Re: [PATCH 09/11] trace-cmd: Consolidate ARRAY_SIZE() in trace-cmd.h

[Steven Rostedt]

On Thu, 23 Nov 2017 18:33:33 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> Since ARRAY_SIZE() is a very useful macro, it makes sense to move it in a common
> header file, in order to avoid several C files to re-define it.
> 

Patch 9-11 could be in their own patch series. As it looks agnostic
from the rest. I may just pull these in directly.

-- Steve

Re: [PATCH 11/11] trace-cmd: Introducing get_trace_cmd_type()

[Steven Rostedt]

On Thu, 23 Nov 2017 18:33:35 +0200
"Vladislav Valtchev (VMware)" <vladislav.valtchev@gmail.com> wrote:

> 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.
> 

Never mind. This is dependent. I pulled in patch 9 and 10 though. I'll
do some tests and push out the lastest so you can rebase on top of that.

Thanks!

-- Steve

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Vladislav Valtchev]

On Tue, 2017-11-28 at 11:48 -0500, Steven Rostedt wrote:
> 
> Is there a reason that init_instance() isn't called in
> init_common_record_context()?
> 

Hi Steven,

init_instance() has been put into init_common_record_context() later,
in patch 8, "Making start,extract,stream,profile separate funcs".


> Also, why not just do the "init_common_record_context()" in
> parse_record_options()?
> 

Because they do semantically different things: trace_* function might
want to do something before parsing the options, but after initilizing the context.
That is what already happens in trace_profile(): during the refactoring I was able
to remove a heading if (IS_PROFILE(ctx)) containing:

	handle_init = trace_init_profile;
	ctx.events = 1;

and a tail:

	/*
	 * If no instances were set, then enable profiling on the top instance.
	 */
	if (!buffer_instances)
		top_instance.profile = 1;

Since that code was profile-specific, I put it in trace_profile(),
without 'if' statements, clearly.

Vlad
 

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Steven Rostedt]

On Tue, 28 Nov 2017 20:17:46 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> Since that code was profile-specific, I put it in trace_profile(),
> without 'if' statements, clearly.

This is all about balancing. You were able to remove one if statement,
but required two function calls by all others.

-- Steve

Re: [PATCH 11/11] trace-cmd: Introducing get_trace_cmd_type()

[Steven Rostedt]

On Tue, 28 Nov 2017 12:17:49 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> Never mind. This is dependent. I pulled in patch 9 and 10 though. I'll
> do some tests and push out the lastest so you can rebase on top of that.

I did a push to master on k.org. Please rebase on top of that. It
includes patches you have already submitted.

-- Steve

Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Vladislav Valtchev]

On Tue, 2017-11-28 at 12:14 -0500, Steven Rostedt wrote:	
> As everything is doing both init_common_record_context() and
> parse_record_options() why not just move the
> init_common_record_context() into parse_record_options(). If you need
> to do something special (like set events = 1 for profile) have that
> done in parse_record_options() if the cmd is CMD_record. Yes, you need
> to pass the CMD_* to parse_record_options() in this case.
> 

Actually, I spent some effort trying to avoid exactly that:
"if" statments in "overly generic code". In my view, the point
of having several trace_* functions is to be able to write specific code
for them, without conditional branches.

I understand that, clearly, there is a trade-off because the extreme of that is to
have a different copy of a function like parse_record_options() for each command.
On the other hand, when you have an IF (cmd is PROFILE) at the beginning and an
if (cmd is PROFILE) at the end, it looks to me a good thing to move that code in the 
specific trace_profile() function. It's all about finding the right balance:
the previous extreme (before the refactoring) was to have everything in trace_record()
with much more if statements and that looked more complicated to follow, at least for me.

Clearly, I fully understand that the location of that "right balance" is pretty subjective.

Do you have a strong opinion on that?


Vlad

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Vladislav Valtchev]

On Tue, 2017-11-28 at 13:30 -0500, Steven Rostedt wrote:
> On Tue, 28 Nov 2017 20:17:46 +0200
> Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:
> 
> > Since that code was profile-specific, I put it in trace_profile(),
> > without 'if' statements, clearly.
> 
> This is all about balancing. You were able to remove one if statement,
> but required two function calls by all others.
> 

I totally agree that is all about balancing.
I wrote exactly the same thing as part of my previous e-mail (before reading this message).

Are you concerned by the cost the of function calls or by the "verbosity" in calling them?

If it's about their cost, I believe that cold code paths like these, where the program is
just parsing its command line arguments, it seems absolutely OK to me to do that.
It's a one-time thing before any kind of tracing starts. And it helps a lot contributors to
better understand the code (in my opinion).

Otherwise, I believe that having those function calls that way just makes the code simpler
for new contributors. Functions, among everything else, are also "labels" for pieces of code.
Having two different labels there (init context and parse options) makes sense to me.
But, again, I understand that is a trade-off based on my subjective value system and
way of thinking, of course.

Do you have a strong opinion?

Vlad 

Re: [PATCH 05/11] trace-cmd: Rename trace_profile() to trace_profile_int()

[Vladislav Valtchev]

On Tue, 2017-11-28 at 12:05 -0500, Steven Rostedt wrote:
> 
> Couple of things.
> 
> 1) A change like this should always be directly before the patch that
> requires it. This is patch 5, and it's not until patch 8 that this
> change is required. Please keep them next to each other. This should
> have been patch 7.
> 
> 2) trace_profile_int() is confusing. I keep thinking it's doing
> something with integers. Call it do_trace_profile().
> 

Sure, no problem.
Thanks for the review!

Re: [PATCH 11/11] trace-cmd: Introducing get_trace_cmd_type()

[Vladislav Valtchev]

On Tue, 2017-11-28 at 13:32 -0500, Steven Rostedt wrote:
> On Tue, 28 Nov 2017 12:17:49 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Never mind. This is dependent. I pulled in patch 9 and 10 though. I'll
> > do some tests and push out the lastest so you can rebase on top of that.
> 
> I did a push to master on k.org. Please rebase on top of that. It
> includes patches you have already submitted.
> 
> -- Steve

Thanks Steven!
I'm happy to have my first 7 patches merged into trace-cmd!

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Steven Rostedt]

On Tue, 28 Nov 2017 20:57:13 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> On Tue, 2017-11-28 at 13:30 -0500, Steven Rostedt wrote:
> > On Tue, 28 Nov 2017 20:17:46 +0200
> > Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:
> >   
> > > Since that code was profile-specific, I put it in trace_profile(),
> > > without 'if' statements, clearly.  
> > 
> > This is all about balancing. You were able to remove one if statement,
> > but required two function calls by all others.
> >   
> 
> I totally agree that is all about balancing.
> I wrote exactly the same thing as part of my previous e-mail (before reading this message).
> 
> Are you concerned by the cost the of function calls or by the "verbosity" in calling them?

No the cost is negligible. More about the complexity. Adding "paired"
calls can sometimes be more confusing than if logic.

> 
> Otherwise, I believe that having those function calls that way just makes the code simpler
> for new contributors. Functions, among everything else, are also "labels" for pieces of code.
> Having two different labels there (init context and parse options) makes sense to me.
> But, again, I understand that is a trade-off based on my subjective value system and
> way of thinking, of course.
> 
> Do you have a strong opinion?

Yes ;-)

I'll reply why in the other email.

-- Steve

Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Steven Rostedt]

On Tue, 28 Nov 2017 20:34:22 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> On Tue, 2017-11-28 at 12:14 -0500, Steven Rostedt wrote:	
> > As everything is doing both init_common_record_context() and
> > parse_record_options() why not just move the
> > init_common_record_context() into parse_record_options(). If you need
> > to do something special (like set events = 1 for profile) have that
> > done in parse_record_options() if the cmd is CMD_record. Yes, you need
> > to pass the CMD_* to parse_record_options() in this case.
> >   
> 
> Actually, I spent some effort trying to avoid exactly that:
> "if" statments in "overly generic code". In my view, the point
> of having several trace_* functions is to be able to write specific code
> for them, without conditional branches.

But not if they are not necessary.

> 
> I understand that, clearly, there is a trade-off because the extreme of that is to
> have a different copy of a function like parse_record_options() for each command.
> On the other hand, when you have an IF (cmd is PROFILE) at the beginning and an
> if (cmd is PROFILE) at the end, it looks to me a good thing to move that code in the 
> specific trace_profile() function. It's all about finding the right balance:

It's a single file. If this was a library function, then sure, I could
understand. But the if would be useful to see how things are different
in one place. The only reason you broke it up, was because of one user.
Are you sure that one user couldn't do it better?

If we left the if statement in, you would see the answer would be yes!

> the previous extreme (before the refactoring) was to have everything in trace_record()
> with much more if statements and that looked more complicated to follow, at least for me.

Obviously it wasn't for me ;-)

> 
> Clearly, I fully understand that the location of that "right balance" is pretty subjective.
> 
> Do you have a strong opinion on that?

Yes. Simply because we lost the fact that we can do it better.

There's a reason I asked about why the "events = 1" was needed? And
saying "it was there before" is not the right answer. You are changing
the flow of the code. These are not subtle changes. These are
legitimate changes to the flow. Even though they are only to make it
easier to understand. The algorithm is changing. Look again, and tell
me, why is "events = 1" needed? And for that matter, the setting of
handle_init. If it is needed, why?

-- Steve

Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Vladislav Valtchev]

On Tue, 2017-11-28 at 14:35 -0500, Steven Rostedt wrote:
> 
> Yes. Simply because we lost the fact that we can do it better.
> 

Hi Steven,
of course we can do better, if we make a step further than just
a mechanical refactoring. I'm used to intentionally limit myself
to do such kind of mechanical refactoring, in order to reduce to the minimum
possible the chance of introducing bugs. And it really works pretty well for me.

I intentionally did not even consider to check whether events = 1 was
needed there or not: for me that kind of simplifications should be done
*after* the mechanical refactoring, not while doing it. This way, while
searching for a bug, I can order the commits by likelihood of being the
root-cause of the bug and, when possible, I'm happy to put changes as this one
at the bottom.

Also, I learned that lesson while working in vSphere, by observing (and a few
times doing myself) "innocent" refactoring changes breaking CI and system tests
because of super-subtle side-effects.

In this concrete case, the original code was:

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

at the beginning of trace_record().
After that, it followed the big "for" loop with its big "switch".
During the refactoring, I followed this order *strictly* and I'm sure
that my change is correct exactly as much as the original code: I just
moved the things around, but the state (ctx and global variables)
continued to change (evolve) exactly in the same order as in the original
code did.

That's why, from that point-of-view, "it was there before" was the
right answer to me.


> There's a reason I asked about why the "events = 1" was needed? And
> saying "it was there before" is not the right answer. You are changing
> the flow of the code. These are not subtle changes. These are
> legitimate changes to the flow. Even though they are only to make it
> easier to understand. The algorithm is changing. Look again, and tell
> me, why is "events = 1" needed? And for that matter, the setting of
> handle_init. If it is needed, why?
> 

Of course, I understand that the level of code improvement with my algorithm
is certainly not the best we can do: it's just the most conservative approach.
I hope that you at least appreciate my effort to make "big" changes like
this one, without even a single minor bug, thanks to a strict approach: it
still seems to me the less evil thing if compared to the introduction of a
subtle bug in a corner case because of a "too smart move".

Returning back to 'events=1', yes I agree that actually parse_record_options()
never reads its value. The same is true for handle_init. Both the variables
are used in the record_trace() part. The "events" flag is checked before doing
expand_event_list() and before:

for_all_instances(ctx->instance)
	enable_events(ctx->instance);

handle_init is used by start_thread(), which is called by record_trace().

Therefore:

	handle_init = trace_init_profile;
	ctx.events = 1;

can be safely moved after parse_record_options() and, the call of
init_common_record_context() could be moved at the beginning of
parse_record_options().

Are you fine with me doing that in a patch the follows immediately
this patch 8 ?

Vlad

Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Steven Rostedt]

On Wed, 29 Nov 2017 12:03:14 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> On Tue, 2017-11-28 at 14:35 -0500, Steven Rostedt wrote:
> > 
> > Yes. Simply because we lost the fact that we can do it better.
> >   
> 
> Hi Steven,
> of course we can do better, if we make a step further than just
> a mechanical refactoring. I'm used to intentionally limit myself
> to do such kind of mechanical refactoring, in order to reduce to the minimum
> possible the chance of introducing bugs. And it really works pretty well for me.
> 
> I intentionally did not even consider to check whether events = 1 was
> needed there or not: for me that kind of simplifications should be done
> *after* the mechanical refactoring, not while doing it. This way, while
> searching for a bug, I can order the commits by likelihood of being the
> root-cause of the bug and, when possible, I'm happy to put changes as this one
> at the bottom.

I totally agree that one shouldn't think about this level of factoring
when doing only mechanical refactoring. But the issue is what's the
best mechanical refactoring you can do to let you see new changes like
this? Note, this is not a bug fix, it becomes another clean up. Having
it as an if statement within one function, makes it stick out more than
breaking up a function into two and having all users call two
functions. The two function approach will "hide" this issue. Again,
it's not a bug in the way it was done, but we miss the change because
it is more likely not to be seen.

> 
> Also, I learned that lesson while working in vSphere, by observing (and a few
> times doing myself) "innocent" refactoring changes breaking CI and system tests
> because of super-subtle side-effects.
> 
> In this concrete case, the original code was:
> 
> 	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);
> 
> at the beginning of trace_record().
> After that, it followed the big "for" loop with its big "switch".
> During the refactoring, I followed this order *strictly* and I'm sure
> that my change is correct exactly as much as the original code: I just
> moved the things around, but the state (ctx and global variables)
> continued to change (evolve) exactly in the same order as in the original
> code did.
> 
> That's why, from that point-of-view, "it was there before" was the
> right answer to me.

And I agree.
> 
> 
> > There's a reason I asked about why the "events = 1" was needed? And
> > saying "it was there before" is not the right answer. You are changing
> > the flow of the code. These are not subtle changes. These are
> > legitimate changes to the flow. Even though they are only to make it
> > easier to understand. The algorithm is changing. Look again, and tell
> > me, why is "events = 1" needed? And for that matter, the setting of
> > handle_init. If it is needed, why?
> >   
> 
> Of course, I understand that the level of code improvement with my algorithm
> is certainly not the best we can do: it's just the most conservative approach.
> I hope that you at least appreciate my effort to make "big" changes like
> this one, without even a single minor bug, thanks to a strict approach: it
> still seems to me the less evil thing if compared to the introduction of a
> subtle bug in a corner case because of a "too smart move".
> 
> Returning back to 'events=1', yes I agree that actually parse_record_options()
> never reads its value. The same is true for handle_init. Both the variables
> are used in the record_trace() part. The "events" flag is checked before doing
> expand_event_list() and before:
> 
> for_all_instances(ctx->instance)
> 	enable_events(ctx->instance);
> 
> handle_init is used by start_thread(), which is called by record_trace().
> 
> Therefore:
> 
> 	handle_init = trace_init_profile;
> 	ctx.events = 1;
> 
> can be safely moved after parse_record_options() and, the call of
> init_common_record_context() could be moved at the beginning of
> parse_record_options().
> 
> Are you fine with me doing that in a patch the follows immediately
> this patch 8 ?

Yes, but I would like init_common_record_context() to be merged into
parse_record_options() in the previous patches and we add that if
statement to make profile different :-)

I see that you see many of the parts of this elephant. Some looks like
a rope, some a fire hose, some a tree trunk, and some a wall. But when
you put it all together, you see the elephant as a whole. I'm trying to
have you see the whole elephant ;-)

The point I'm making is that the way something is refactored, should
start out not breaking the flow of logic. The original code technically
had an if statement for the profile (but it was in a switch statement).
That if statement should have stayed throughout the mechanical
refactoring. And then after the refactoring was done, we could look at
seeing if more can be cleaned up. And then we would see that the if
statement was not necessary. But by breaking up the flow into two
functions, that extra code would be hidden, and we would have
unnecessarily have two functions.

The way mechanical updates happen does matter, to help improve the
less trivial updates. That's the point I'm trying to make.

-- Steve

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Steven Rostedt]

On Tue, 28 Nov 2017 20:17:46 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> On Tue, 2017-11-28 at 11:48 -0500, Steven Rostedt wrote:
> > 
> > Is there a reason that init_instance() isn't called in
> > init_common_record_context()?
> >   
> 
> Hi Steven,
> 
> init_instance() has been put into init_common_record_context() later,
> in patch 8, "Making start,extract,stream,profile separate funcs".

I have to ask. Why isn't it done in this patch. This patch appears to
be the proper place to put it.

-- Steve

Re: [PATCH 04/11] trace-cmd: Extract parse_record_options() from trace_record()

[Vladislav Valtchev]

On Wed, 2017-11-29 at 09:53 -0500, Steven Rostedt wrote:
> On Tue, 28 Nov 2017 20:17:46 +0200
> Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:
> 
> > On Tue, 2017-11-28 at 11:48 -0500, Steven Rostedt wrote:
> > > 
> > > Is there a reason that init_instance() isn't called in
> > > init_common_record_context()?
> > >   
> > 
> > Hi Steven,
> > 
> > init_instance() has been put into init_common_record_context() later,
> > in patch 8, "Making start,extract,stream,profile separate funcs".
> 
> I have to ask. Why isn't it done in this patch. This patch appears to
> be the proper place to put it.
> 

There is no reason, actually. You're absolutely right.
It's a leftover from several iterations of fixes (originally
'instance' was not part of ctx).

Therefore, I'll move that call to init_instance() inside init_common_record_context().


Vlad