diff mbox series

[v3,09/11] main: Overhaul target selection

Message ID 20180524055017.8801-10-amitay@ozlabs.org
State Changes Requested
Headers show
Series Some more cleanups | expand

Commit Message

Amitay Isaacs May 24, 2018, 5:50 a.m. UTC
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(-)

Comments

Alistair Popple May 24, 2018, 12:25 p.m. UTC | #1
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 mbox series

Patch

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)