diff mbox series

[2/6] main: Print specific errors for invalid arguments

Message ID 20180525045240.24196-3-amitay@ozlabs.org
State Superseded
Headers show
Series Overhaul target selection | expand

Commit Message

Amitay Isaacs May 25, 2018, 4:52 a.m. UTC 
This avoids the large usage message obscuring the actual errors from
parsing options.  Print usage only if an option is invalid.

Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 src/main.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Rashmica Gupta June 1, 2018, 12:30 a.m. UTC | #1 
On 25/05/18 14:52, Amitay Isaacs wrote:
> This avoids the large usage message obscuring the actual errors from
> parsing options.  Print usage only if an option is invalid.
>
> Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> ---
>  src/main.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/src/main.c b/src/main.c
> index 457cda2..4a1b0e0 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -226,7 +226,7 @@ static bool parse_list(const char *arg, int max, int *list, int *count)
>  static bool parse_options(int argc, char *argv[])
>  {
>  	int c;
> -	bool opt_error = true;
> +	bool opt_error = false;
>  	int p_list[MAX_PROCESSORS];
>  	int c_list[MAX_CHIPS];
>  	int t_list[MAX_THREADS];
> @@ -255,8 +255,6 @@ static bool parse_options(int argc, char *argv[])
>  
>  		switch(c) {
>  		case 'a':
> -			opt_error = false;
> -
>  			if (p_count == 0) {
>  				p_count = MAX_PROCESSORS;
>  				for (i=0; i<MAX_PROCESSORS; i++)
> @@ -277,28 +275,27 @@ static bool parse_options(int argc, char *argv[])
>  			break;
>  
>  		case 'p':
> -			if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count))
> +			if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count)) {
>  				fprintf(stderr, "Failed to parse '-p %s'\n", optarg);
> -			else
> -				opt_error = false;
> +				opt_error = true;
> +			}
>  			break;
>  
>  		case 'c':
> -			if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count))
> +			if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count)) {
>  				fprintf(stderr, "Failed to parse '-c %s'\n", optarg);
> -			else
> -				opt_error = false;
> +				opt_error = true;
> +			}
>  			break;
>  
>  		case 't':
> -			if (!parse_list(optarg, MAX_THREADS, t_list, &t_count))
> +			if (!parse_list(optarg, MAX_THREADS, t_list, &t_count)) {
>  				fprintf(stderr, "Failed to parse '-t %s'\n", optarg);
> -			else
>  				opt_error = false;
Should this also be changed to opt_error=true?

> +			}
>  			break;
>  
>  		case 'b':
> -			opt_error = false;
>  			if (strcmp(optarg, "fsi") == 0) {
>  				backend = FSI;
>  				device_node = "p9w";
> @@ -312,12 +309,13 @@ static bool parse_options(int argc, char *argv[])
>  				backend = FAKE;
>  			} else if (strcmp(optarg, "host") == 0) {
>  				backend = HOST;
> -			} else
> +			} else {
> +				fprintf(stderr, "Invalid backend '%s'\n", optarg);
>  				opt_error = true;
> +			}
>  			break;
>  
>  		case 'd':
> -			opt_error = false;
>  			device_node = optarg;
>  			break;
>  
> @@ -325,6 +323,8 @@ static bool parse_options(int argc, char *argv[])
>  			errno = 0;
>  			i2c_addr = strtoull(optarg, &endptr, 0);
>  			opt_error = (errno || *endptr != '\0');
> +			if (opt_error)
> +				fprintf(stderr, "Invalid slave address '%s'\n", optarg);
>  			break;
>  
>  		case 'D':
> @@ -337,19 +337,18 @@ static bool parse_options(int argc, char *argv[])
>  			break;
>  
>  		case 'E':
> -			opt_error = false;
>  			pdbg_expert_mode = true;
>  			break;
>  
>  		case '?':
>  		case 'h':
>  			opt_error = true;
> +			print_usage(basename(argv[0]));
>  			break;
>  		}
>  	} while (c != EOF && !opt_error);
>  
>  	if (opt_error) {
> -		print_usage(basename(argv[0]));
>  		return false;
>  	}
>
Amitay Isaacs June 1, 2018, 1:07 a.m. UTC | #2 
On Fri, 2018-06-01 at 10:30 +1000, rashmica wrote:
> 
> On 25/05/18 14:52, Amitay Isaacs wrote:
> > This avoids the large usage message obscuring the actual errors
> > from
> > parsing options.  Print usage only if an option is invalid.
> > 
> > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> > ---
> >  src/main.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/main.c b/src/main.c
> > index 457cda2..4a1b0e0 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -226,7 +226,7 @@ static bool parse_list(const char *arg, int
> > max, int *list, int *count)
> >  static bool parse_options(int argc, char *argv[])
> >  {
> >  	int c;
> > -	bool opt_error = true;
> > +	bool opt_error = false;
> >  	int p_list[MAX_PROCESSORS];
> >  	int c_list[MAX_CHIPS];
> >  	int t_list[MAX_THREADS];
> > @@ -255,8 +255,6 @@ static bool parse_options(int argc, char
> > *argv[])
> >  
> >  		switch(c) {
> >  		case 'a':
> > -			opt_error = false;
> > -
> >  			if (p_count == 0) {
> >  				p_count = MAX_PROCESSORS;
> >  				for (i=0; i<MAX_PROCESSORS; i++)
> > @@ -277,28 +275,27 @@ static bool parse_options(int argc, char
> > *argv[])
> >  			break;
> >  
> >  		case 'p':
> > -			if (!parse_list(optarg, MAX_PROCESSORS,
> > p_list, &p_count))
> > +			if (!parse_list(optarg, MAX_PROCESSORS,
> > p_list, &p_count)) {
> >  				fprintf(stderr, "Failed to parse
> > '-p %s'\n", optarg);
> > -			else
> > -				opt_error = false;
> > +				opt_error = true;
> > +			}
> >  			break;
> >  
> >  		case 'c':
> > -			if (!parse_list(optarg, MAX_CHIPS, c_list,
> > &c_count))
> > +			if (!parse_list(optarg, MAX_CHIPS, c_list,
> > &c_count)) {
> >  				fprintf(stderr, "Failed to parse
> > '-c %s'\n", optarg);
> > -			else
> > -				opt_error = false;
> > +				opt_error = true;
> > +			}
> >  			break;
> >  
> >  		case 't':
> > -			if (!parse_list(optarg, MAX_THREADS,
> > t_list, &t_count))
> > +			if (!parse_list(optarg, MAX_THREADS,
> > t_list, &t_count)) {
> >  				fprintf(stderr, "Failed to parse
> > '-t %s'\n", optarg);
> > -			else
> >  				opt_error = false;
> 
> Should this also be changed to opt_error=true?

Yeah, good catch!

> 
> > +			}
> >  			break;
> >  
> >  		case 'b':
> > -			opt_error = false;
> >  			if (strcmp(optarg, "fsi") == 0) {
> >  				backend = FSI;
> >  				device_node = "p9w";
> > @@ -312,12 +309,13 @@ static bool parse_options(int argc, char
> > *argv[])
> >  				backend = FAKE;
> >  			} else if (strcmp(optarg, "host") == 0) {
> >  				backend = HOST;
> > -			} else
> > +			} else {
> > +				fprintf(stderr, "Invalid backend
> > '%s'\n", optarg);
> >  				opt_error = true;
> > +			}
> >  			break;
> >  
> >  		case 'd':
> > -			opt_error = false;
> >  			device_node = optarg;
> >  			break;
> >  
> > @@ -325,6 +323,8 @@ static bool parse_options(int argc, char
> > *argv[])
> >  			errno = 0;
> >  			i2c_addr = strtoull(optarg, &endptr, 0);
> >  			opt_error = (errno || *endptr != '\0');
> > +			if (opt_error)
> > +				fprintf(stderr, "Invalid slave
> > address '%s'\n", optarg);
> >  			break;
> >  
> >  		case 'D':
> > @@ -337,19 +337,18 @@ static bool parse_options(int argc, char
> > *argv[])
> >  			break;
> >  
> >  		case 'E':
> > -			opt_error = false;
> >  			pdbg_expert_mode = true;
> >  			break;
> >  
> >  		case '?':
> >  		case 'h':
> >  			opt_error = true;
> > +			print_usage(basename(argv[0]));
> >  			break;
> >  		}
> >  	} while (c != EOF && !opt_error);
> >  
> >  	if (opt_error) {
> > -		print_usage(basename(argv[0]));
> >  		return false;
> >  	}
> >  
> 
> 

Amitay.
diff mbox series

Patch

diff --git a/src/main.c b/src/main.c
index 457cda2..4a1b0e0 100644
--- a/src/main.c
+++ b/src/main.c
@@ -226,7 +226,7 @@  static bool parse_list(const char *arg, int max, int *list, int *count)
 static bool parse_options(int argc, char *argv[])
 {
 	int c;
-	bool opt_error = true;
+	bool opt_error = false;
 	int p_list[MAX_PROCESSORS];
 	int c_list[MAX_CHIPS];
 	int t_list[MAX_THREADS];
@@ -255,8 +255,6 @@  static bool parse_options(int argc, char *argv[])
 
 		switch(c) {
 		case 'a':
-			opt_error = false;
-
 			if (p_count == 0) {
 				p_count = MAX_PROCESSORS;
 				for (i=0; i<MAX_PROCESSORS; i++)
@@ -277,28 +275,27 @@  static bool parse_options(int argc, char *argv[])
 			break;
 
 		case 'p':
-			if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count))
+			if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count)) {
 				fprintf(stderr, "Failed to parse '-p %s'\n", optarg);
-			else
-				opt_error = false;
+				opt_error = true;
+			}
 			break;
 
 		case 'c':
-			if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count))
+			if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count)) {
 				fprintf(stderr, "Failed to parse '-c %s'\n", optarg);
-			else
-				opt_error = false;
+				opt_error = true;
+			}
 			break;
 
 		case 't':
-			if (!parse_list(optarg, MAX_THREADS, t_list, &t_count))
+			if (!parse_list(optarg, MAX_THREADS, t_list, &t_count)) {
 				fprintf(stderr, "Failed to parse '-t %s'\n", optarg);
-			else
 				opt_error = false;
+			}
 			break;
 
 		case 'b':
-			opt_error = false;
 			if (strcmp(optarg, "fsi") == 0) {
 				backend = FSI;
 				device_node = "p9w";
@@ -312,12 +309,13 @@  static bool parse_options(int argc, char *argv[])
 				backend = FAKE;
 			} else if (strcmp(optarg, "host") == 0) {
 				backend = HOST;
-			} else
+			} else {
+				fprintf(stderr, "Invalid backend '%s'\n", optarg);
 				opt_error = true;
+			}
 			break;
 
 		case 'd':
-			opt_error = false;
 			device_node = optarg;
 			break;
 
@@ -325,6 +323,8 @@  static bool parse_options(int argc, char *argv[])
 			errno = 0;
 			i2c_addr = strtoull(optarg, &endptr, 0);
 			opt_error = (errno || *endptr != '\0');
+			if (opt_error)
+				fprintf(stderr, "Invalid slave address '%s'\n", optarg);
 			break;
 
 		case 'D':
@@ -337,19 +337,18 @@  static bool parse_options(int argc, char *argv[])
 			break;
 
 		case 'E':
-			opt_error = false;
 			pdbg_expert_mode = true;
 			break;
 
 		case '?':
 		case 'h':
 			opt_error = true;
+			print_usage(basename(argv[0]));
 			break;
 		}
 	} while (c != EOF && !opt_error);
 
 	if (opt_error) {
-		print_usage(basename(argv[0]));
 		return false;
 	}