Message ID | 20180525045240.24196-3-amitay@ozlabs.org |
---|---|
State | Superseded |
Headers | show |
Series | Overhaul target selection | expand |
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; > } >
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 --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; }
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(-)