Message ID | 20180524055017.8801-10-amitay@ozlabs.org |
---|---|
State | Changes Requested |
Headers | show |
Series | Some more cleanups | expand |
Thanks Amitay, fixing this has been on the list for a while. A couple of comments below though. On Thursday, 24 May 2018 3:50:15 PM AEST Amitay Isaacs wrote: > +/* 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 n = 0, i, j; > + > + strcpy(str, arg); > + > + 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[n] = i; > + n++; What is preventing a buffer overrun here? I suspect there is one here because with this patch applied I hit a segfault running the following command: pdbg -p0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 getscom 0xf000f Obviously this is a little contrived but it's important we deal gracefully with frustrated users who really really want this to run only on p0 :-) > + } > + > + tmp = NULL; > + }; > + > + if (n > 0) { > + qsort(list, n, sizeof(int), int_cmp); > + j = 0; > + for (i=1; i<n; i++) { > + if (list[i] != list[j]) { > + list[++j] = list[i]; In general I feel this function could do with a few more brief comments to aid understanding. For example at first glance it may not be immediately obvious to everyone what the above is doing (removing duplicates from a list). There also seems to be a few corner cases when I tested with the following fake device-tree which adds a couple of extra pib nodes: /dts-v1/; / { #address-cells = <0x1>; #size-cells = <0x0>; fsi@0 { #address-cells = <0x2>; #size-cells = <0x1>; compatible = "ibm,fake-fsi"; reg = <0x0 0x0 0x0>; index = <0x0>; status = "mustexist"; pib@0 { compatible = "ibm,fake-pib"; reg = <0x0 0x0 0x0>; index = <0x0>; }; pib@1 { compatible = "ibm,fake-pib"; reg = <0x0 0x1 0x0>; index = <0x1>; }; pib@2 { compatible = "ibm,fake-pib"; reg = <0x0 0x1 0x0>; index = <0x2>; }; }; }; For example this works as expected: $ ./pdbg -p0 getscom 0xf000f fake_pib_read(0x000f000f, 0xdeadbeef) p0:0xf000f = 0x00000000deadbeef But this doesn't (it appears to return the result for p0 instead of p1): $ ./pdbg -p1 getscom 0xf000f fake_pib_read(0x000f000f, 0xdeadbeef) p0:0xf000f = 0x00000000deadbeef Even though this works: $ ./pdbg -p0-1 getscom 0xf000f fake_pib_read(0x000f000f, 0xdeadbeef) p0:0xf000f = 0x00000000deadbeef fake_pib_read(0x000f000f, 0xdeadbeef) p1:0xf000f = 0x00000000deadbeef But this doesn't: $ ./pdbg -p1-2 getscom 0xf000f fake_pib_read(0x000f000f, 0xdeadbeef) p0:0xf000f = 0x00000000deadbeef fake_pib_read(0x000f000f, 0xdeadbeef) p1:0xf000f = 0x00000000deadbeef So it would be nice if we could get the above cases fixed (and even better write some tests! - although that is something that has been on my TODO list for ages). However the concept is great and really improves a big usability issue so keen to see it fixed up so we can merge it. When you do repost it please post it as a seperate patch as I have already merged the rest of the patches in this series. Thanks! - Alistair > + } > + } > + n = j+1; > + } > + > + *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 cur_p, cur_c, cur_t; > + int i, j, k; > struct option long_opts[] = { > {"all", no_argument, NULL, 'a'}, > {"backend", required_argument, NULL, 'b'}, > @@ -195,50 +274,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] = i; > + } > + > + if (c_count == 0) { > + c_count = MAX_CHIPS; > + for (i=0; i<MAX_CHIPS; i++) > + c_list[i] = i; > + } > + > + if (t_count == 0) { > + t_count = MAX_THREADS; > + for (i=0; i<MAX_THREADS; i++) > + t_list[i] = i; > } > 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 +366,37 @@ static bool parse_options(int argc, char *argv[]) > } > } while (c != EOF && !opt_error); > > - if (opt_error) > + if (opt_error) { > print_usage(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<p_count; i++) { > + cur_p = p_list[i]; > + processorsel[i] = &chipsel[cur_p][0]; > + for (j=0; j<c_count; j++) { > + cur_c = c_list[j]; > + chipsel[cur_p][cur_c] = &threadsel[cur_p][cur_c][0]; > + for (k=0; k<t_count; k++) { > + cur_t = t_list[k]; > + threadsel[cur_p][cur_c][cur_t] = 1; > + } > + } > + } > > - return !opt_error; > + return true; > } > > void target_select(struct pdbg_target *target) >
diff --git a/src/main.c b/src/main.c index 7540b9b..1a01bf1 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,90 @@ static void print_usage(char *pname) } } +static int int_cmp(const void *a, const void *b) +{ + int a_int = *(const int *)a; + int b_int = *(const int *)b; + + if (a_int < b_int) + return -1; + else if (a_int > b_int) + return 1; + else + return 0; +} + +/* 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 n = 0, i, j; + + strcpy(str, arg); + + 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[n] = i; + n++; + } + + tmp = NULL; + }; + + if (n > 0) { + qsort(list, n, sizeof(int), int_cmp); + j = 0; + for (i=1; i<n; i++) { + if (list[i] != list[j]) { + list[++j] = list[i]; + } + } + n = j+1; + } + + *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 cur_p, cur_c, cur_t; + int i, j, k; struct option long_opts[] = { {"all", no_argument, NULL, 'a'}, {"backend", required_argument, NULL, 'b'}, @@ -195,50 +274,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] = i; + } + + if (c_count == 0) { + c_count = MAX_CHIPS; + for (i=0; i<MAX_CHIPS; i++) + c_list[i] = i; + } + + if (t_count == 0) { + t_count = MAX_THREADS; + for (i=0; i<MAX_THREADS; i++) + t_list[i] = i; } 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 +366,37 @@ static bool parse_options(int argc, char *argv[]) } } while (c != EOF && !opt_error); - if (opt_error) + if (opt_error) { print_usage(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<p_count; i++) { + cur_p = p_list[i]; + processorsel[i] = &chipsel[cur_p][0]; + for (j=0; j<c_count; j++) { + cur_c = c_list[j]; + chipsel[cur_p][cur_c] = &threadsel[cur_p][cur_c][0]; + for (k=0; k<t_count; k++) { + cur_t = t_list[k]; + threadsel[cur_p][cur_c][cur_t] = 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 | 181 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 141 insertions(+), 40 deletions(-)