[bpf-next,V2,3/3] selftests/bpf: test_progs indicate to shell on non-actions

When a user selects a non-existing test the summary is printed with
indication 0 for all info types, and shell "success" (EXIT_SUCCESS) is
indicated. This can be understood by a human end-user, but for shell
scripting is it useful to indicate a shell failure (EXIT_FAILURE).

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 tools/testing/selftests/bpf/test_progs.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

On Wed, Jul 1, 2020 at 1:19 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> When a user selects a non-existing test the summary is printed with
> indication 0 for all info types, and shell "success" (EXIT_SUCCESS) is
> indicated. This can be understood by a human end-user, but for shell
> scripting is it useful to indicate a shell failure (EXIT_FAILURE).
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 3345cd977c10..75cf5b13cbd6 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -706,11 +706,8 @@ int main(int argc, char **argv)
>                 goto out;
>         }
>
> -       if (env.list_test_names) {
> -               if (env.succ_cnt == 0)
> -                       env.fail_cnt = 1;
> +       if (env.list_test_names)
>                 goto out;
> -       }
>
>         fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
>                 env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
> @@ -723,5 +720,9 @@ int main(int argc, char **argv)
>         free_str_set(&env.subtest_selector.whitelist);
>         free(env.subtest_selector.num_set);
>
> +       /* Return EXIT_FAILURE when options resulted in no actions */
> +       if (!env.succ_cnt && !env.fail_cnt && !env.skip_cnt)
> +               env.fail_cnt = 1;
> +

Heh, just suggested something like this in the previous patch. I think
this change should go first in patch series and not churn on
env.list_test_names above.

I'd also rewrite it as (no need to muck around with fail_cnt, less
negation for integers):

if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
    return EXIT_FAILURE;

>         return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
>  }
>
>

On Wed, 1 Jul 2020 13:54:41 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Wed, Jul 1, 2020 at 1:19 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> >
> > When a user selects a non-existing test the summary is printed with
> > indication 0 for all info types, and shell "success" (EXIT_SUCCESS) is
> > indicated. This can be understood by a human end-user, but for shell
> > scripting is it useful to indicate a shell failure (EXIT_FAILURE).
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> >  tools/testing/selftests/bpf/test_progs.c |    9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 3345cd977c10..75cf5b13cbd6 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -706,11 +706,8 @@ int main(int argc, char **argv)
> >                 goto out;
> >         }
> >
> > -       if (env.list_test_names) {
> > -               if (env.succ_cnt == 0)
> > -                       env.fail_cnt = 1;
> > +       if (env.list_test_names)
> >                 goto out;
> > -       }
> >
> >         fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> >                 env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
> > @@ -723,5 +720,9 @@ int main(int argc, char **argv)
> >         free_str_set(&env.subtest_selector.whitelist);
> >         free(env.subtest_selector.num_set);
> >
> > +       /* Return EXIT_FAILURE when options resulted in no actions */
> > +       if (!env.succ_cnt && !env.fail_cnt && !env.skip_cnt)
> > +               env.fail_cnt = 1;
> > +  
> 
> Heh, just suggested something like this in the previous patch. I think
> this change should go first in patch series and not churn on
> env.list_test_names above.
> 
> I'd also rewrite it as (no need to muck around with fail_cnt, less
> negation for integers):
> 
> if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
>     return EXIT_FAILURE;

All good suggestions, I'll respin.