| Message ID | 20180601063050.19286-2-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Overhaul target selection | expand |
Thanks Amitay. A couple more comments below: > + while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) { > + char *a, *b, *saveptr2 = NULL; > + int from, to; These should be unsigned to avoid somewhat contrived errors like this: ./pdbg -p 2147483649,4294967295 getscom 0xf000f Segmentation fault > + > + > + a = strtok_r(tok, "-", &saveptr2); > + if (a == NULL) { > + return false; > + } else { > + from = atoi(a); > + if (from >= max) { > + return false; > + } > + } > + > + b = strtok_r(NULL, "-", &saveptr2); > + if (b == NULL) { > + to = from; > + } else { > + to = atoi(b); I think it would be preferable to detect errors here. At the moment the following works: ./pdbg -p 0-0xa ... but: ./pdbg -p 1-0xa ... doesn't. Which is not a problem in that I don't think people will want to specify things in hex (altough perhaps they do and is there any cost to supporting it?), but an error would be less confusing than guessing the users intent. > + if (to >= max) { > + return false; > + } > + } > + > + if (from > to) > + return false; Should we add an assert(to < max) here? I can see at present that the above code bails if to/from is >= max but future changes could alter that and lead to an overflow in the below. > + for (i=from; i<=to; i++) Also very minor style nit-pick - most of the exsiting code has whitespace between operators (ie. for (i = from; i <= to; i++)), so it would be good if we could maintain that. Thanks! - Alistair > + list[i] = 1; > + > + tmp = NULL; > + }; > + > + n = 0; > + for (i=0; i<max; i++) { > + if (list[i] == 1) > + n++; > + } > + > + *count = n; > + return true; > +} > + > static bool parse_options(int argc, char *argv[]) > { > int c; > bool opt_error = true; > - static int current_processor = INT_MAX, current_chip = INT_MAX, current_thread = INT_MAX; > + int p_list[MAX_PROCESSORS]; > + int c_list[MAX_CHIPS]; > + int t_list[MAX_THREADS]; > + int p_count = 0, c_count = 0, t_count = 0; > + int i, j, k; > struct option long_opts[] = { > {"all", no_argument, NULL, 'a'}, > {"backend", required_argument, NULL, 'b'}, > @@ -195,50 +256,45 @@ static bool parse_options(int argc, char *argv[]) > switch(c) { > case 'a': > opt_error = false; > - for (current_processor = 0; current_processor < MAX_PROCESSORS; current_processor++) { > - processorsel[current_processor] = &chipsel[current_processor][0]; > - for (current_chip = 0; current_chip < MAX_CHIPS; current_chip++) { > - chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0]; > - for (current_thread = 0; current_thread < MAX_THREADS; current_thread++) > - threadsel[current_processor][current_chip][current_thread] = 1; > - } > + > + if (p_count == 0) { > + p_count = MAX_PROCESSORS; > + for (i=0; i<MAX_PROCESSORS; i++) > + p_list[i] = 1; > + } > + > + if (c_count == 0) { > + c_count = MAX_CHIPS; > + for (i=0; i<MAX_CHIPS; i++) > + c_list[i] = 1; > + } > + > + if (t_count == 0) { > + t_count = MAX_THREADS; > + for (i=0; i<MAX_THREADS; i++) > + t_list[i] = 1; > } > break; > > case 'p': > - errno = 0; > - current_processor = strtoul(optarg, &endptr, 0); > - opt_error = (errno || *endptr != '\0'); > - if (!opt_error) { > - if (current_processor >= MAX_PROCESSORS) > - opt_error = true; > - else > - processorsel[current_processor] = &chipsel[current_processor][0]; > - } > + if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count)) > + fprintf(stderr, "Failed to parse '-p %s'\n", optarg); > + else > + opt_error = false; > break; > > case 'c': > - errno = 0; > - current_chip = strtoul(optarg, &endptr, 0); > - opt_error = (errno || *endptr != '\0'); > - if (!opt_error) { > - if (current_chip >= MAX_CHIPS) > - opt_error = true; > - else > - chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0]; > - } > + if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count)) > + fprintf(stderr, "Failed to parse '-c %s'\n", optarg); > + else > + opt_error = false; > break; > > case 't': > - errno = 0; > - current_thread = strtoul(optarg, &endptr, 0); > - opt_error = (errno || *endptr != '\0'); > - if (!opt_error) { > - if (current_thread >= MAX_THREADS) > - opt_error = true; > - else > - threadsel[current_processor][current_chip][current_thread] = 1; > - } > + 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': > @@ -292,10 +348,45 @@ static bool parse_options(int argc, char *argv[]) > } > } while (c != EOF && !opt_error); > > - if (opt_error) > + if (opt_error) { > print_usage(basename(argv[0])); > + return false; > + } > + > + if ((c_count > 0 || t_count > 0) && p_count == 0) { > + fprintf(stderr, "No processor(s) selected\n"); > + fprintf(stderr, "Use -p or -a to select processor(s)\n"); > + return false; > + } > + > + if (t_count > 0 && c_count == 0) { > + fprintf(stderr, "No chip(s) selected\n"); > + fprintf(stderr, "Use -c or -a to select chip(s)\n"); > + return false; > + } > + > + for (i=0; i<MAX_PROCESSORS; i++) { > + if (p_list[i] == 0) > + continue; > + > + processorsel[i] = &chipsel[i][0]; > + > + for (j=0; j<MAX_CHIPS; j++) { > + if (c_list[j] == 0) > + continue; > + > + chipsel[i][j] = &threadsel[i][j][0]; > + > + for (k=0; k<MAX_THREADS; k++) { > + if (t_list[k] == 0) > + continue; > + > + threadsel[i][j][k] = 1; > + } > + } > + } > > - return !opt_error; > + return true; > } > > void target_select(struct pdbg_target *target) >
On Mon, 2018-06-04 at 14:21 +1000, Alistair Popple wrote: > Thanks Amitay. A couple more comments below: > > > + while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) { > > + char *a, *b, *saveptr2 = NULL; > > + int from, to; > > These should be unsigned to avoid somewhat contrived errors like > this: > > ./pdbg -p 2147483649,4294967295 getscom 0xf000f > Segmentation fault Tests, tests, we need tests. :-) > > > + > > + > > + a = strtok_r(tok, "-", &saveptr2); > > + if (a == NULL) { > > + return false; > > + } else { > > + from = atoi(a); > > + if (from >= max) { > > + return false; > > + } > > + } > > + > > + b = strtok_r(NULL, "-", &saveptr2); > > + if (b == NULL) { > > + to = from; > > + } else { > > + to = atoi(b); > > I think it would be preferable to detect errors here. At the moment > the following works: > > ./pdbg -p 0-0xa ... > > but: > > ./pdbg -p 1-0xa ... > > doesn't. Which is not a problem in that I don't think people will > want to > specify things in hex (altough perhaps they do and is there any cost > to > supporting it?), but an error would be less confusing than guessing > the users > intent. Will switch to using stroul instead of atoi. > > > + if (to >= max) { > > + return false; > > + } > > + } > > + > > + if (from > to) > > + return false; > > Should we add an assert(to < max) here? I can see at present that the > above code > bails if to/from is >= max but future changes could alter that and > lead to an > overflow in the below. I think we should never assert on user input. Assert is good to catch programming errors. For invalid user-input, we should print appropriate error message. > > > + for (i=from; i<=to; i++) > > Also very minor style nit-pick - most of the exsiting code has > whitespace > between operators (ie. for (i = from; i <= to; i++)), so it would be > good if we > could maintain that. Thanks! Sure. > > - Alistair > > > + list[i] = 1; > > + > > + tmp = NULL; > > + }; > > + > > + n = 0; > > + for (i=0; i<max; i++) { > > + if (list[i] == 1) > > + n++; > > + } > > + > > + *count = n; > > + return true; > > +} > > + > > static bool parse_options(int argc, char *argv[]) > > { > > int c; > > bool opt_error = true; > > - static int current_processor = INT_MAX, current_chip = > > INT_MAX, current_thread = INT_MAX; > > + int p_list[MAX_PROCESSORS]; > > + int c_list[MAX_CHIPS]; > > + int t_list[MAX_THREADS]; > > + int p_count = 0, c_count = 0, t_count = 0; > > + int i, j, k; > > struct option long_opts[] = { > > {"all", no_argument, > > NULL, 'a'}, > > {"backend", required_argument, > > NULL, 'b'}, > > @@ -195,50 +256,45 @@ static bool parse_options(int argc, char > > *argv[]) > > switch(c) { > > case 'a': > > opt_error = false; > > - for (current_processor = 0; > > current_processor < MAX_PROCESSORS; current_processor++) { > > - processorsel[current_processor] = > > &chipsel[current_processor][0]; > > - for (current_chip = 0; > > current_chip < MAX_CHIPS; current_chip++) { > > - chipsel[current_processor] > > [current_chip] = &threadsel[current_processor][current_chip][0]; > > - for (current_thread = 0; > > current_thread < MAX_THREADS; current_thread++) > > - threadsel[current_ > > processor][current_chip][current_thread] = 1; > > - } > > + > > + if (p_count == 0) { > > + p_count = MAX_PROCESSORS; > > + for (i=0; i<MAX_PROCESSORS; i++) > > + p_list[i] = 1; > > + } > > + > > + if (c_count == 0) { > > + c_count = MAX_CHIPS; > > + for (i=0; i<MAX_CHIPS; i++) > > + c_list[i] = 1; > > + } > > + > > + if (t_count == 0) { > > + t_count = MAX_THREADS; > > + for (i=0; i<MAX_THREADS; i++) > > + t_list[i] = 1; > > } > > break; > > > > case 'p': > > - errno = 0; > > - current_processor = strtoul(optarg, > > &endptr, 0); > > - opt_error = (errno || *endptr != '\0'); > > - if (!opt_error) { > > - if (current_processor >= > > MAX_PROCESSORS) > > - opt_error = true; > > - else > > - processorsel[current_proce > > ssor] = &chipsel[current_processor][0]; > > - } > > + if (!parse_list(optarg, MAX_PROCESSORS, > > p_list, &p_count)) > > + fprintf(stderr, "Failed to parse > > '-p %s'\n", optarg); > > + else > > + opt_error = false; > > break; > > > > case 'c': > > - errno = 0; > > - current_chip = strtoul(optarg, &endptr, > > 0); > > - opt_error = (errno || *endptr != '\0'); > > - if (!opt_error) { > > - if (current_chip >= MAX_CHIPS) > > - opt_error = true; > > - else > > - chipsel[current_processor] > > [current_chip] = &threadsel[current_processor][current_chip][0]; > > - } > > + if (!parse_list(optarg, MAX_CHIPS, c_list, > > &c_count)) > > + fprintf(stderr, "Failed to parse > > '-c %s'\n", optarg); > > + else > > + opt_error = false; > > break; > > > > case 't': > > - errno = 0; > > - current_thread = strtoul(optarg, &endptr, > > 0); > > - opt_error = (errno || *endptr != '\0'); > > - if (!opt_error) { > > - if (current_thread >= MAX_THREADS) > > - opt_error = true; > > - else > > - threadsel[current_processo > > r][current_chip][current_thread] = 1; > > - } > > + 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': > > @@ -292,10 +348,45 @@ static bool parse_options(int argc, char > > *argv[]) > > } > > } while (c != EOF && !opt_error); > > > > - if (opt_error) > > + if (opt_error) { > > print_usage(basename(argv[0])); > > + return false; > > + } > > + > > + if ((c_count > 0 || t_count > 0) && p_count == 0) { > > + fprintf(stderr, "No processor(s) selected\n"); > > + fprintf(stderr, "Use -p or -a to select > > processor(s)\n"); > > + return false; > > + } > > + > > + if (t_count > 0 && c_count == 0) { > > + fprintf(stderr, "No chip(s) selected\n"); > > + fprintf(stderr, "Use -c or -a to select > > chip(s)\n"); > > + return false; > > + } > > + > > + for (i=0; i<MAX_PROCESSORS; i++) { > > + if (p_list[i] == 0) > > + continue; > > + > > + processorsel[i] = &chipsel[i][0]; > > + > > + for (j=0; j<MAX_CHIPS; j++) { > > + if (c_list[j] == 0) > > + continue; > > + > > + chipsel[i][j] = &threadsel[i][j][0]; > > + > > + for (k=0; k<MAX_THREADS; k++) { > > + if (t_list[k] == 0) > > + continue; > > + > > + threadsel[i][j][k] = 1; > > + } > > + } > > + } > > > > - return !opt_error; > > + return true; > > } > > > > void target_select(struct pdbg_target *target) > > > > Amitay.
diff --git a/src/main.c b/src/main.c index 2200a01..457cda2 100644 --- a/src/main.c +++ b/src/main.c @@ -131,9 +131,9 @@ static void print_usage(char *pname) printf("Usage: %s [options] command ...\n\n", pname); printf(" Options:\n"); - printf("\t-p, --processor=processor-id\n"); - printf("\t-c, --chip=core-id\n"); - printf("\t-t, --thread=thread\n"); + printf("\t-p, --processor=<0-%d>|<range>|<list>\n", MAX_PROCESSORS); + printf("\t-c, --chip=<0-%d>|<range>|<list>\n", MAX_CHIPS); + printf("\t-t, --thread=<0-%d>|<range>|<list>\n", MAX_THREADS); printf("\t-a, --all\n"); printf("\t\tRun command on all possible processors/chips/threads (default)\n"); printf("\t-b, --backend=backend\n"); @@ -166,11 +166,72 @@ static void print_usage(char *pname) } } +/* Parse argument of the form 0-5,7,9-11,15,17 */ +static bool parse_list(const char *arg, int max, int *list, int *count) +{ + char str[strlen(arg)+1]; + char *tok, *tmp, *saveptr = NULL; + int i, n; + + strcpy(str, arg); + + for (i=0; i<max; i++) { + list[i] = 0; + } + + tmp = str; + while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) { + char *a, *b, *saveptr2 = NULL; + int from, to; + + a = strtok_r(tok, "-", &saveptr2); + if (a == NULL) { + return false; + } else { + from = atoi(a); + if (from >= max) { + return false; + } + } + + b = strtok_r(NULL, "-", &saveptr2); + if (b == NULL) { + to = from; + } else { + to = atoi(b); + if (to >= max) { + return false; + } + } + + if (from > to) + return false; + + for (i=from; i<=to; i++) + list[i] = 1; + + tmp = NULL; + }; + + n = 0; + for (i=0; i<max; i++) { + if (list[i] == 1) + n++; + } + + *count = n; + return true; +} + static bool parse_options(int argc, char *argv[]) { int c; bool opt_error = true; - static int current_processor = INT_MAX, current_chip = INT_MAX, current_thread = INT_MAX; + int p_list[MAX_PROCESSORS]; + int c_list[MAX_CHIPS]; + int t_list[MAX_THREADS]; + int p_count = 0, c_count = 0, t_count = 0; + int i, j, k; struct option long_opts[] = { {"all", no_argument, NULL, 'a'}, {"backend", required_argument, NULL, 'b'}, @@ -195,50 +256,45 @@ static bool parse_options(int argc, char *argv[]) switch(c) { case 'a': opt_error = false; - for (current_processor = 0; current_processor < MAX_PROCESSORS; current_processor++) { - processorsel[current_processor] = &chipsel[current_processor][0]; - for (current_chip = 0; current_chip < MAX_CHIPS; current_chip++) { - chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0]; - for (current_thread = 0; current_thread < MAX_THREADS; current_thread++) - threadsel[current_processor][current_chip][current_thread] = 1; - } + + if (p_count == 0) { + p_count = MAX_PROCESSORS; + for (i=0; i<MAX_PROCESSORS; i++) + p_list[i] = 1; + } + + if (c_count == 0) { + c_count = MAX_CHIPS; + for (i=0; i<MAX_CHIPS; i++) + c_list[i] = 1; + } + + if (t_count == 0) { + t_count = MAX_THREADS; + for (i=0; i<MAX_THREADS; i++) + t_list[i] = 1; } break; case 'p': - errno = 0; - current_processor = strtoul(optarg, &endptr, 0); - opt_error = (errno || *endptr != '\0'); - if (!opt_error) { - if (current_processor >= MAX_PROCESSORS) - opt_error = true; - else - processorsel[current_processor] = &chipsel[current_processor][0]; - } + if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count)) + fprintf(stderr, "Failed to parse '-p %s'\n", optarg); + else + opt_error = false; break; case 'c': - errno = 0; - current_chip = strtoul(optarg, &endptr, 0); - opt_error = (errno || *endptr != '\0'); - if (!opt_error) { - if (current_chip >= MAX_CHIPS) - opt_error = true; - else - chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0]; - } + if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count)) + fprintf(stderr, "Failed to parse '-c %s'\n", optarg); + else + opt_error = false; break; case 't': - errno = 0; - current_thread = strtoul(optarg, &endptr, 0); - opt_error = (errno || *endptr != '\0'); - if (!opt_error) { - if (current_thread >= MAX_THREADS) - opt_error = true; - else - threadsel[current_processor][current_chip][current_thread] = 1; - } + 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': @@ -292,10 +348,45 @@ static bool parse_options(int argc, char *argv[]) } } while (c != EOF && !opt_error); - if (opt_error) + if (opt_error) { print_usage(basename(argv[0])); + return false; + } + + if ((c_count > 0 || t_count > 0) && p_count == 0) { + fprintf(stderr, "No processor(s) selected\n"); + fprintf(stderr, "Use -p or -a to select processor(s)\n"); + return false; + } + + if (t_count > 0 && c_count == 0) { + fprintf(stderr, "No chip(s) selected\n"); + fprintf(stderr, "Use -c or -a to select chip(s)\n"); + return false; + } + + for (i=0; i<MAX_PROCESSORS; i++) { + if (p_list[i] == 0) + continue; + + processorsel[i] = &chipsel[i][0]; + + for (j=0; j<MAX_CHIPS; j++) { + if (c_list[j] == 0) + continue; + + chipsel[i][j] = &threadsel[i][j][0]; + + for (k=0; k<MAX_THREADS; k++) { + if (t_list[k] == 0) + continue; + + threadsel[i][j][k] = 1; + } + } + } - return !opt_error; + return true; } void target_select(struct pdbg_target *target)
Support explicit multiple target selection using ranges and lists. For options -p/-c/-t, support the following valid arguments: 3 0-7 1,2,3 0-5,7,9-11,17,19 For loss of sanity, make sense of the following valid arguments: 3,3,3,3,3,3 1-6,2-5 1,2,3,0-7 Conjunction of -p/-c/-t with -a also works and it's insensitive to order of options specified. -a -c 1,2 processors 0-max; chips 1-2; threads 0-max -p 0 -c 0 -a processors 0; chips 0; threads 0-max -a -c 1 -t 1 processors 0-max; chips 1; threads 1 Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/main.c | 171 ++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 131 insertions(+), 40 deletions(-)