[PATCH 3/3] trace-cmd: fold option parsing

[PATCH 3/3] trace-cmd: fold option parsing

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 trace-listen.c |   12 +++++-------
 trace-read.c   |   35 +++++++++++++++--------------------
 trace-record.c |   14 +++++---------
 trace-stack.c  |   32 +++++++++++++++-----------------
 4 files changed, 40 insertions(+), 53 deletions(-)

diff --git a/trace-listen.c b/trace-listen.c
index e60c96b..daaffca 100644
--- a/trace-listen.c
+++ b/trace-listen.c
@@ -647,6 +647,8 @@ static void start_daemon(void)
 		die("starting daemon");
 }
 
+#define OPT_debug	255
+
 void trace_listen(int argc, char **argv)
 {
 	char *logfile = NULL;
@@ -666,7 +668,7 @@ void trace_listen(int argc, char **argv)
 		static struct option long_options[] = {
 			{"port", required_argument, NULL, 'p'},
 			{"help", no_argument, NULL, '?'},
-			{"debug", no_argument, NULL, 0},
+			{"debug", no_argument, NULL, OPT_debug},
 			{NULL, 0, NULL, 0}
 		};
 
@@ -696,12 +698,8 @@ void trace_listen(int argc, char **argv)
 		case 'D':
 			daemon = 1;
 			break;
-		case 0:
-			switch (option_index) {
-			case 2:
-				debug = 1;
-				break;
-			}
+		case OPT_debug:
+			debug = 1;
 			break;
 		default:
 			usage(argv);
diff --git a/trace-read.c b/trace-read.c
index 932f5aa..018476d 100644
--- a/trace-read.c
+++ b/trace-read.c
@@ -812,6 +812,10 @@ static void add_functions(struct pevent *pevent, const char *file)
 	free(buf);
 }
 
+#define OPT_cpu		255
+#define OPT_events	254
+#define OPT_kallsyms	253
+
 void trace_report (int argc, char **argv)
 {
 	struct tracecmd_input *handle;
@@ -845,10 +849,10 @@ void trace_report (int argc, char **argv)
 	for (;;) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{"cpu", required_argument, NULL, 0},
-			{"events", no_argument, NULL, 0},
+			{"cpu", required_argument, NULL, OPT_cpu},
+			{"events", no_argument, NULL, OPT_events},
 			{"filter-test", no_argument, NULL, 'T'},
-			{"kallsyms", required_argument, NULL, 0},
+			{"kallsyms", required_argument, NULL, OPT_kallsyms},
 			{"help", no_argument, NULL, '?'},
 			{NULL, 0, NULL, 0}
 		};
@@ -917,23 +921,14 @@ void trace_report (int argc, char **argv)
 		case 'q':
 			silence_warnings = 1;
 			break;
-		case 0:
-			switch(option_index) {
-			case 0: /* cpu */
-				parse_cpulist(optarg);
-				break;
-			case 1: /* events */
-				print_events = 1;
-				break;
-			case 2: /* filter-test */
-				test_filters = 1;
-				break;
-			case 3: /* kallsyms */
-				functions = optarg;
-				break;
-			default:
-				usage(argv);
-			}
+		case OPT_cpu:
+			parse_cpulist(optarg);
+			break;
+		case OPT_events:
+			print_events = 1;
+			break;
+		case OPT_kallsyms:
+			functions = optarg;
 			break;
 		default:
 			usage(argv);
diff --git a/trace-record.c b/trace-record.c
index 2611bfa..1cf13b6 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -1809,6 +1809,8 @@ static void record_all_events(void)
 	listed_events = list;
 }
 
+#define OPT_date	255
+
 void trace_record (int argc, char **argv)
 {
 	const char *plugin = NULL;
@@ -1862,7 +1864,7 @@ void trace_record (int argc, char **argv)
 	for (;;) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{"date", no_argument, NULL, 0},
+			{"date", no_argument, NULL, OPT_date},
 			{"help", no_argument, NULL, '?'},
 			{NULL, 0, NULL, 0}
 		};
@@ -2002,14 +2004,8 @@ void trace_record (int argc, char **argv)
 		case 'i':
 			ignore_event_not_found = 1;
 			break;
-		case 0:
-			switch (option_index) {
-			case 0: /* date */
-				date = 1;
-				break;
-			default:
-				usage(argv);
-			}
+		case OPT_date:
+			date = 1;
 			break;
 		default:
 			usage(argv);
diff --git a/trace-stack.c b/trace-stack.c
index 3d9f392..24ba194 100644
--- a/trace-stack.c
+++ b/trace-stack.c
@@ -153,6 +153,10 @@ static void read_trace(void)
 	fclose(fp);
 }
 
+#define OPT_start	255
+#define OPT_stop	254
+#define OPT_reset	253
+
 void trace_stack (int argc, char **argv)
 {
 	enum stack_type trace_type = STACK_REPORT;
@@ -167,9 +171,9 @@ void trace_stack (int argc, char **argv)
 	for (;;) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{"start", no_argument, NULL, 0},
-			{"stop", no_argument, NULL, 0},
-			{"reset", no_argument, NULL, 0},
+			{"start", no_argument, NULL, OPT_start},
+			{"stop", no_argument, NULL, OPT_stop},
+			{"reset", no_argument, NULL, OPT_reset},
 			{"help", no_argument, NULL, '?'},
 			{NULL, 0, NULL, 0}
 		};
@@ -183,20 +187,14 @@ void trace_stack (int argc, char **argv)
 		case 'h':
 			usage(argv);
 			break;
-		case 0:
-			switch(option_index) {
-			case 0:
-				trace_type = STACK_START;
-				break;
-			case 1:
-				trace_type = STACK_STOP;
-				break;
-			case 2:
-				trace_type = STACK_RESET;
-				break;
-			default:
-				usage(argv);
-			}
+		case OPT_start:
+			trace_type = STACK_START;
+			break;
+		case OPT_stop:
+			trace_type = STACK_STOP;
+			break;
+		case OPT_reset:
+			trace_type = STACK_RESET;
 			break;
 		default:
 			usage(argv);
-- 
1.7.4

Re: [PATCH 3/3] trace-cmd: fold option parsing

[Steven Rostedt]

On Thu, 2011-03-10 at 17:58 +0800, Lai Jiangshan wrote:
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

Thanks!

>  
> +#define OPT_debug	255
> +

>  
> +#define OPT_cpu		255
> +#define OPT_events	254
> +#define OPT_kallsyms	253
> +

I wonder if it would look cleaner if we converted them all to enums:

enum {
	OPT_kallsyms	= 253,
	OPT_events	= 254,
	OPT_cpu		= 255,
};

As in the kernel, defines look ugly, and can have side-effects.

-- Steve

[PATCH 3/3 V2] trace-cmd: fold option parsing

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 trace-listen.c |   14 +++++++-------
 trace-read.c   |   37 +++++++++++++++++--------------------
 trace-record.c |   16 +++++++---------
 trace-stack.c  |   34 +++++++++++++++++-----------------
 4 files changed, 48 insertions(+), 53 deletions(-)

diff --git a/trace-listen.c b/trace-listen.c
index e60c96b..e1cf9ea 100644
--- a/trace-listen.c
+++ b/trace-listen.c
@@ -647,6 +647,10 @@ static void start_daemon(void)
 		die("starting daemon");
 }
 
+enum {
+	OPT_debug	= 128,
+};
+
 void trace_listen(int argc, char **argv)
 {
 	char *logfile = NULL;
@@ -666,7 +670,7 @@ void trace_listen(int argc, char **argv)
 		static struct option long_options[] = {
 			{"port", required_argument, NULL, 'p'},
 			{"help", no_argument, NULL, '?'},
-			{"debug", no_argument, NULL, 0},
+			{"debug", no_argument, NULL, OPT_debug},
 			{NULL, 0, NULL, 0}
 		};
 
@@ -696,12 +700,8 @@ void trace_listen(int argc, char **argv)
 		case 'D':
 			daemon = 1;
 			break;
-		case 0:
-			switch (option_index) {
-			case 2:
-				debug = 1;
-				break;
-			}
+		case OPT_debug:
+			debug = 1;
 			break;
 		default:
 			usage(argv);
diff --git a/trace-read.c b/trace-read.c
index 63e2940..dfd945e 100644
--- a/trace-read.c
+++ b/trace-read.c
@@ -815,6 +815,12 @@ static void add_functions(struct pevent *pevent, const char *file)
 	free(buf);
 }
 
+enum {
+	OPT_cpu		= 128,
+	OPT_events,
+	OPT_kallsyms,
+};
+
 void trace_report (int argc, char **argv)
 {
 	struct tracecmd_input *handle;
@@ -848,10 +854,10 @@ void trace_report (int argc, char **argv)
 	for (;;) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{"cpu", required_argument, NULL, 0},
-			{"events", no_argument, NULL, 0},
+			{"cpu", required_argument, NULL, OPT_cpu},
+			{"events", no_argument, NULL, OPT_events},
 			{"filter-test", no_argument, NULL, 'T'},
-			{"kallsyms", required_argument, NULL, 0},
+			{"kallsyms", required_argument, NULL, OPT_kallsyms},
 			{"help", no_argument, NULL, '?'},
 			{NULL, 0, NULL, 0}
 		};
@@ -920,23 +926,14 @@ void trace_report (int argc, char **argv)
 		case 'q':
 			silence_warnings = 1;
 			break;
-		case 0:
-			switch(option_index) {
-			case 0: /* cpu */
-				parse_cpulist(optarg);
-				break;
-			case 1: /* events */
-				print_events = 1;
-				break;
-			case 2: /* filter-test */
-				test_filters = 1;
-				break;
-			case 3: /* kallsyms */
-				functions = optarg;
-				break;
-			default:
-				usage(argv);
-			}
+		case OPT_cpu:
+			parse_cpulist(optarg);
+			break;
+		case OPT_events:
+			print_events = 1;
+			break;
+		case OPT_kallsyms:
+			functions = optarg;
 			break;
 		default:
 			usage(argv);
diff --git a/trace-record.c b/trace-record.c
index 2611bfa..dd1f18c 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -1809,6 +1809,10 @@ static void record_all_events(void)
 	listed_events = list;
 }
 
+enum {
+	OPT_date	= 128,
+};
+
 void trace_record (int argc, char **argv)
 {
 	const char *plugin = NULL;
@@ -1862,7 +1866,7 @@ void trace_record (int argc, char **argv)
 	for (;;) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{"date", no_argument, NULL, 0},
+			{"date", no_argument, NULL, OPT_date},
 			{"help", no_argument, NULL, '?'},
 			{NULL, 0, NULL, 0}
 		};
@@ -2002,14 +2006,8 @@ void trace_record (int argc, char **argv)
 		case 'i':
 			ignore_event_not_found = 1;
 			break;
-		case 0:
-			switch (option_index) {
-			case 0: /* date */
-				date = 1;
-				break;
-			default:
-				usage(argv);
-			}
+		case OPT_date:
+			date = 1;
 			break;
 		default:
 			usage(argv);
diff --git a/trace-stack.c b/trace-stack.c
index 3d9f392..52c30de 100644
--- a/trace-stack.c
+++ b/trace-stack.c
@@ -153,6 +153,12 @@ static void read_trace(void)
 	fclose(fp);
 }
 
+enum {
+	OPT_start	= 128,
+	OPT_stop,
+	OPT_reset,
+};
+
 void trace_stack (int argc, char **argv)
 {
 	enum stack_type trace_type = STACK_REPORT;
@@ -167,9 +173,9 @@ void trace_stack (int argc, char **argv)
 	for (;;) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{"start", no_argument, NULL, 0},
-			{"stop", no_argument, NULL, 0},
-			{"reset", no_argument, NULL, 0},
+			{"start", no_argument, NULL, OPT_start},
+			{"stop", no_argument, NULL, OPT_stop},
+			{"reset", no_argument, NULL, OPT_reset},
 			{"help", no_argument, NULL, '?'},
 			{NULL, 0, NULL, 0}
 		};
@@ -183,20 +189,14 @@ void trace_stack (int argc, char **argv)
 		case 'h':
 			usage(argv);
 			break;
-		case 0:
-			switch(option_index) {
-			case 0:
-				trace_type = STACK_START;
-				break;
-			case 1:
-				trace_type = STACK_STOP;
-				break;
-			case 2:
-				trace_type = STACK_RESET;
-				break;
-			default:
-				usage(argv);
-			}
+		case OPT_start:
+			trace_type = STACK_START;
+			break;
+		case OPT_stop:
+			trace_type = STACK_STOP;
+			break;
+		case OPT_reset:
+			trace_type = STACK_RESET;
 			break;
 		default:
 			usage(argv);
-- 
1.7.4

Re: [PATCH 3/3 V2] trace-cmd: fold option parsing

[Steven Rostedt]

On Mon, 2011-03-14 at 10:42 +0800, Lai Jiangshan wrote:
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  trace-listen.c |   14 +++++++-------
>  trace-read.c   |   37 +++++++++++++++++--------------------
>  trace-record.c |   16 +++++++---------
>  trace-stack.c  |   34 +++++++++++++++++-----------------
>  4 files changed, 48 insertions(+), 53 deletions(-)

Thanks Lai,

But I already applied your original changes, and then added a patch to
convert the defines into enums.

-- Steve

> 
> diff --git a/trace-listen.c b/trace-listen.c
> index e60c96b..e1cf9ea 100644
> --- a/trace-listen.c
> +++ b/trace-listen.c
> @@ -647,6 +647,10 @@ static void start_daemon(void)
>  		die("starting daemon");
>  }
>  
> +enum {
> +	OPT_debug	= 128,
> +};
> +
>  void trace_listen(int argc, char **argv)
>  {
>  	char *logfile = NULL;
> @@ -666,7 +670,7 @@ void trace_listen(int argc, char **argv)
>  		static struct option long_options[] = {
>  			{"port", required_argument, NULL, 'p'},
>  			{"help", no_argument, NULL, '?'},
> -			{"debug", no_argument, NULL, 0},
> +			{"debug", no_argument, NULL, OPT_debug},
>  			{NULL, 0, NULL, 0}
>  		};
>  
> @@ -696,12 +700,8 @@ void trace_listen(int argc, char **argv)
>  		case 'D':
>  			daemon = 1;
>  			break;
> -		case 0:
> -			switch (option_index) {
> -			case 2:
> -				debug = 1;
> -				break;
> -			}
> +		case OPT_debug:
> +			debug = 1;
>  			break;
>  		default:
>  			usage(argv);
> diff --git a/trace-read.c b/trace-read.c
> index 63e2940..dfd945e 100644
> --- a/trace-read.c
> +++ b/trace-read.c
> @@ -815,6 +815,12 @@ static void add_functions(struct pevent *pevent, const char *file)
>  	free(buf);
>  }
>  
> +enum {
> +	OPT_cpu		= 128,
> +	OPT_events,
> +	OPT_kallsyms,
> +};
> +
>  void trace_report (int argc, char **argv)
>  {
>  	struct tracecmd_input *handle;
> @@ -848,10 +854,10 @@ void trace_report (int argc, char **argv)
>  	for (;;) {
>  		int option_index = 0;
>  		static struct option long_options[] = {
> -			{"cpu", required_argument, NULL, 0},
> -			{"events", no_argument, NULL, 0},
> +			{"cpu", required_argument, NULL, OPT_cpu},
> +			{"events", no_argument, NULL, OPT_events},
>  			{"filter-test", no_argument, NULL, 'T'},
> -			{"kallsyms", required_argument, NULL, 0},
> +			{"kallsyms", required_argument, NULL, OPT_kallsyms},
>  			{"help", no_argument, NULL, '?'},
>  			{NULL, 0, NULL, 0}
>  		};
> @@ -920,23 +926,14 @@ void trace_report (int argc, char **argv)
>  		case 'q':
>  			silence_warnings = 1;
>  			break;
> -		case 0:
> -			switch(option_index) {
> -			case 0: /* cpu */
> -				parse_cpulist(optarg);
> -				break;
> -			case 1: /* events */
> -				print_events = 1;
> -				break;
> -			case 2: /* filter-test */
> -				test_filters = 1;
> -				break;
> -			case 3: /* kallsyms */
> -				functions = optarg;
> -				break;
> -			default:
> -				usage(argv);
> -			}
> +		case OPT_cpu:
> +			parse_cpulist(optarg);
> +			break;
> +		case OPT_events:
> +			print_events = 1;
> +			break;
> +		case OPT_kallsyms:
> +			functions = optarg;
>  			break;
>  		default:
>  			usage(argv);
> diff --git a/trace-record.c b/trace-record.c
> index 2611bfa..dd1f18c 100644
> --- a/trace-record.c
> +++ b/trace-record.c
> @@ -1809,6 +1809,10 @@ static void record_all_events(void)
>  	listed_events = list;
>  }
>  
> +enum {
> +	OPT_date	= 128,
> +};
> +
>  void trace_record (int argc, char **argv)
>  {
>  	const char *plugin = NULL;
> @@ -1862,7 +1866,7 @@ void trace_record (int argc, char **argv)
>  	for (;;) {
>  		int option_index = 0;
>  		static struct option long_options[] = {
> -			{"date", no_argument, NULL, 0},
> +			{"date", no_argument, NULL, OPT_date},
>  			{"help", no_argument, NULL, '?'},
>  			{NULL, 0, NULL, 0}
>  		};
> @@ -2002,14 +2006,8 @@ void trace_record (int argc, char **argv)
>  		case 'i':
>  			ignore_event_not_found = 1;
>  			break;
> -		case 0:
> -			switch (option_index) {
> -			case 0: /* date */
> -				date = 1;
> -				break;
> -			default:
> -				usage(argv);
> -			}
> +		case OPT_date:
> +			date = 1;
>  			break;
>  		default:
>  			usage(argv);
> diff --git a/trace-stack.c b/trace-stack.c
> index 3d9f392..52c30de 100644
> --- a/trace-stack.c
> +++ b/trace-stack.c
> @@ -153,6 +153,12 @@ static void read_trace(void)
>  	fclose(fp);
>  }
>  
> +enum {
> +	OPT_start	= 128,
> +	OPT_stop,
> +	OPT_reset,
> +};
> +
>  void trace_stack (int argc, char **argv)
>  {
>  	enum stack_type trace_type = STACK_REPORT;
> @@ -167,9 +173,9 @@ void trace_stack (int argc, char **argv)
>  	for (;;) {
>  		int option_index = 0;
>  		static struct option long_options[] = {
> -			{"start", no_argument, NULL, 0},
> -			{"stop", no_argument, NULL, 0},
> -			{"reset", no_argument, NULL, 0},
> +			{"start", no_argument, NULL, OPT_start},
> +			{"stop", no_argument, NULL, OPT_stop},
> +			{"reset", no_argument, NULL, OPT_reset},
>  			{"help", no_argument, NULL, '?'},
>  			{NULL, 0, NULL, 0}
>  		};
> @@ -183,20 +189,14 @@ void trace_stack (int argc, char **argv)
>  		case 'h':
>  			usage(argv);
>  			break;
> -		case 0:
> -			switch(option_index) {
> -			case 0:
> -				trace_type = STACK_START;
> -				break;
> -			case 1:
> -				trace_type = STACK_STOP;
> -				break;
> -			case 2:
> -				trace_type = STACK_RESET;
> -				break;
> -			default:
> -				usage(argv);
> -			}
> +		case OPT_start:
> +			trace_type = STACK_START;
> +			break;
> +		case OPT_stop:
> +			trace_type = STACK_STOP;
> +			break;
> +		case OPT_reset:
> +			trace_type = STACK_RESET;
>  			break;
>  		default:
>  			usage(argv);