[Pdbg] [PATCH v2 8/9] main: Overhaul target selection logic

[Balbir Singh]

On Wed, May 23, 2018 at 2:31 PM, Amitay Isaacs <amitay at ozlabs.org> wrote:
> This should now fix the logic for -a working in conjunction with
> -p/-c/-t to select all threads.
>
>  -a            select all threads
>  -a -p 0       select all threads on processor 0
>  -p 0 -a       select all threads on processor 0
>  -p 0 -c 1 -a  select all threads on processor 0, chip 1
>
> Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> ---
>  src/main.c | 89 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/src/main.c b/src/main.c
> index 65bf6c5..71f82fe 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -135,7 +135,7 @@ static void print_usage(char *pname)
>         printf("\t-c, --chip=<0-%d>\n", MAX_CHIPS-1);
>         printf("\t-t, --thread=<0-%d>\n", MAX_THREADS-1);
>         printf("\t-a, --all\n");
> -       printf("\t\tRun command on all possible processors/chips/threads (default)\n");
> +       printf("\t\tRun command on all possible processors/chips/threads\n");
>         printf("\t-b, --backend=backend\n");
>         printf("\t\tfsi:\tAn experimental backend that uses\n");
>         printf("\t\t\tbit-banging to access the host processor\n");
> @@ -170,7 +170,9 @@ 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;
> +       static int start_p = INT_MAX, start_c = INT_MAX, start_t = INT_MAX;
> +       static int end_p = INT_MAX, end_c = INT_MAX, end_t = INT_MAX;

This is a bit confusing, why is start_p INT_MAX, effectively we need a
range, why does the range
start at INT_MAX and not 0, we need to reset it to 0 down below?

> +       static int cur_p, cur_c, cur_t;
>         struct option long_opts[] = {
>                 {"all",                 no_argument,            NULL,   'a'},
>                 {"backend",             required_argument,      NULL,   'b'},
> @@ -195,49 +197,53 @@ 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 (start_p == INT_MAX) {
> +                               start_p = 0;
> +                               end_p = MAX_PROCESSORS-1;
> +                       }
> +                       if (start_c == INT_MAX) {
> +                               start_c = 0;
> +                               end_c = MAX_CHIPS-1;
> +                       }
> +                       if (start_t == INT_MAX) {
> +                               start_t = 0;
> +                               end_t = MAX_THREADS-1;

Why not make these the starting default values above?

>                         }
>                         break;
>
>                 case 'p':
>                         errno = 0;
> -                       current_processor = strtoul(optarg, &endptr, 0);
> +                       cur_p = strtoul(optarg, &endptr, 0);
>                         opt_error = (errno || *endptr != '\0');
>                         if (!opt_error) {
> -                               if (current_processor >= MAX_PROCESSORS)
> +                               if (cur_p >= MAX_PROCESSORS)
>                                         opt_error = true;
>                                 else
> -                                       processorsel[current_processor] = &chipsel[current_processor][0];
> +                                       start_p = end_p = cur_p;
>                         }
>                         break;
>
>                 case 'c':
>                         errno = 0;
> -                       current_chip = strtoul(optarg, &endptr, 0);
> +                       cur_c = strtoul(optarg, &endptr, 0);
>                         opt_error = (errno || *endptr != '\0');
>                         if (!opt_error) {
> -                               if (current_chip >= MAX_CHIPS)
> +                               if (cur_c >= MAX_CHIPS)
>                                         opt_error = true;
>                                 else
> -                                       chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
> +                                       start_c = end_c = cur_c;
>                         }
>                         break;
>
>                 case 't':
>                         errno = 0;
> -                       current_thread = strtoul(optarg, &endptr, 0);
> +                       cur_t = strtoul(optarg, &endptr, 0);
>                         opt_error = (errno || *endptr != '\0');
>                         if (!opt_error) {
> -                               if (current_thread >= MAX_THREADS)
> +                               if (cur_t >= MAX_THREADS)
>                                         opt_error = true;
>                                 else
> -                                       threadsel[current_processor][current_chip][current_thread] = 1;
> +                                       start_t = end_t = cur_t;
>                         }
>                         break;
>
> @@ -292,10 +298,53 @@ static bool parse_options(int argc, char *argv[])
>                 }
>         } while (c != EOF && !opt_error);
>
> -       if (opt_error)
> +       if (opt_error) {
>                 print_usage(argv[0]);
> +               return opt_error;
> +       }
> +
> +       if (start_c != INT_MAX && start_p == INT_MAX) {
> +               printf("No processor(s) selected\n");
> +               printf("Use -p to select processor(s)\n");

These should all go to stderr? I think this can be simplified and needs to
be revisited. I think this patch also subtly breaks other things, in
the past we could do

pdbg -c0 -t 0 -t 1, now thats broken, IIUC

Balbir