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

[Amitay Isaacs]

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

Reality is a nice place to visit, but I sure would not want to live there.