[Pdbg] [PATCH v2 1/7] main: Overhaul target selection

[Alistair Popple]

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)
>