netdev - Re: [PATCH v2 bpf-next 15/15] bpftool: add `gen skeleton` BASH completions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Dec 2019 21:12:30 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Andrii Nakryiko <andriin@...com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...com, daniel@...earbox.net
Cc:     andrii.nakryiko@...il.com, kernel-team@...com
Subject: Re: [PATCH v2 bpf-next 15/15] bpftool: add `gen skeleton` BASH
 completions

2019-12-12 08:41 UTC-0800 ~ Andrii Nakryiko <andriin@...com>
> Add BASH completions for gen sub-command.
> 
> Cc: Quentin Monnet <quentin.monnet@...ronome.com>
> Acked-by: Martin KaFai Lau <kafai@...com>
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---
>  tools/bpf/bpftool/bash-completion/bpftool         | 11 +++++++++++
>  tools/bpf/bpftool/main.c                          |  2 +-
>  tools/testing/selftests/bpf/prog_tests/skeleton.c |  6 ++++--
>  tools/testing/selftests/bpf/progs/test_skeleton.c |  3 ++-
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 70493a6da206..986519cc58d1 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -716,6 +716,17 @@ _bpftool()
>                      ;;
>              esac
>              ;;
> +        gen)
> +            case $command in
> +                skeleton)
> +                    _filedir
> +		    ;;
> +                *)
> +                    [[ $prev == $object ]] && \
> +                        COMPREPLY=( $( compgen -W 'skeleton help' -- "$cur" ) )
> +                    ;;
> +            esac
> +            ;;

Hi Andrii,

Bpftool completion looks OK to me...

>          cgroup)
>              case $command in
>                  show|list|tree)
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 758b294e8a7d..1fe91c558508 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -58,7 +58,7 @@ static int do_help(int argc, char **argv)
>  		"       %s batch file FILE\n"
>  		"       %s version\n"
>  		"\n"
> -		"       OBJECT := { prog | map | cgroup | perf | net | feature | btf }\n"
> +		"       OBJECT := { prog | map | cgroup | perf | net | feature | btf | gen }\n"

... but this is part of the usage message, and ideally should be added
when you add the new feature to bpftool in patch 11. Not a major issue,
but ...

>  		"       " HELP_SPEC_OPTIONS "\n"
>  		"",
>  		bin_name, bin_name, bin_name);
> diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c
> index d65a0203e1df..94e0300f437a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c
> +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c
> @@ -39,8 +39,10 @@ void test_skeleton(void)
>  	CHECK(bss->out2 != 2, "res2", "got %lld != exp %d\n", bss->out2, 2);
>  	CHECK(bss->out3 != 3, "res3", "got %d != exp %d\n", (int)bss->out3, 3);
>  	CHECK(bss->out4 != 4, "res4", "got %lld != exp %d\n", bss->out4, 4);
> -	CHECK(bss->out5.a != 5, "res5", "got %d != exp %d\n", bss->out5.a, 5);
> -	CHECK(bss->out5.b != 6, "res6", "got %lld != exp %d\n", bss->out5.b, 6);
> +	CHECK(bss->handler_out5.a != 5, "res5", "got %d != exp %d\n",
> +	      bss->handler_out5.a, 5);
> +	CHECK(bss->handler_out5.b != 6, "res6", "got %lld != exp %d\n",
> +	      bss->handler_out5.b, 6);

... This and the code below does not seem to relate to bpftool
completion at all. And it was not present in v1. I suspect this code was
not intended to end up in this patch?

>  
>  cleanup:
>  	test_skeleton__destroy(skel);
> diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c
> index 303a841c4d1c..db4fd88f3ecb 100644
> --- a/tools/testing/selftests/bpf/progs/test_skeleton.c
> +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
> @@ -16,7 +16,6 @@ long long in4 __attribute__((aligned(64))) = 0;
>  struct s in5 = {};
>  
>  long long out2 = 0;
> -struct s out5 = {};
>  char out3 = 0;
>  long long out4 = 0;
>  int out1 = 0;
> @@ -25,6 +24,8 @@ int out1 = 0;
>  SEC("raw_tp/sys_enter")
>  int handler(const void *ctx)
>  {
> +	static volatile struct s out5;
> +
>  	out1 = in1;
>  	out2 = in2;
>  	out3 = in3;
>