| 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
| ||
|
Date: Thu, 25 Apr 2019 09:44:30 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Quentin Monnet <quentin.monnet@...ronome.com>
Cc: Kernel Team <kernel-team@...com>,
Networking <netdev@...r.kernel.org>, bpf@...r.kernel.org,
Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Yonghong Song <yhs@...com>, Song Liu <songliubraving@...com>,
Martin Lau <kafai@...com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Andrii Nakryiko <andriin@...com>
Subject: Re: [PATCH v2 bpf-next 3/3] bpftool: add bash completions for btf command
On Thu, Apr 25, 2019 at 9:33 AM Quentin Monnet
<quentin.monnet@...ronome.com> wrote:
>
> 2019-04-25 09:14 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> > On Thu, Apr 25, 2019 at 4:15 AM Quentin Monnet
> > <quentin.monnet@...ronome.com> wrote:
> >>
> >> 2019-04-24 22:03 UTC-0700 ~ <andrii.nakryiko@...il.com>
> >>> From: Andrii Nakryiko <andriin@...com>
> >>>
> >>> Add full support for btf command in bash-completion script.
> >>>
> >>> Cc: Quentin Monnet <quentin.monnet@...ronome.com>
> >>> Cc: Yonghong Song <yhs@...com>
> >>> Cc: Daniel Borkmann <daniel@...earbox.net>
> >>> Cc: Alexei Starovoitov <ast@...com>
> >>> Signed-off-by: Andrii Nakryiko <andriin@...com>
> >>> ---
> >>> tools/bpf/bpftool/bash-completion/bpftool | 46 +++++++++++++++++++++++
> >>> 1 file changed, 46 insertions(+)
> >>>
> >>> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> >>> index 9f3ffe1e26ab..030f81bdec6a 100644
> >>> --- a/tools/bpf/bpftool/bash-completion/bpftool
> >>> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> >>> @@ -217,6 +217,7 @@ _bpftool()
> >>> done
> >>> cur=${words[cword]}
> >>> prev=${words[cword - 1]}
> >>> + pprev=${words[cword - 2]}
> >>>
> >>> local object=${words[1]} command=${words[2]}
> >>>
> >>> @@ -607,6 +608,51 @@ _bpftool()
> >>> ;;
> >>> esac
> >>> ;;
> >>> + btf)
> >>> + local PROG_TYPE='id pinned tag'
> >>> + local MAP_TYPE='id pinned'
> >>> + case $command in
> >>> + dump)
> >>> + case $prev in
> >>> + $command)
> >>> + COMPREPLY+=( $( compgen -W "id map prog file" -- \
> >>> + "$cur" ) )
> >>> + return 0
> >>> + ;;
> >>> + prog)
> >>> + COMPREPLY=( $( compgen -W "$PROG_TYPE" -- "$cur" ) )
> >>> + return 0
> >>> + ;;
> >>> + map)
> >>> + COMPREPLY=( $( compgen -W "$MAP_TYPE" -- "$cur" ) )
> >>> + return 0
> >>> + ;;
> >>> + id)
> >>> + case $pprev in
> >>> + prog)
> >>> + _bpftool_get_prog_ids
> >>> + ;;
> >>> + map)
> >>> + _bpftool_get_map_ids
> >>> + ;;
> >>> + esac
> >>> + return 0
> >>> + ;;
> >>> + *)
> >>> + if _bpftool_search_list 'map'; then
> >>> + COMPREPLY+=( $( compgen -W 'key value kv all' -- \
> >>> + "$cur" ) )
> >>
> >> Hi Andrii,
> >>
> >> This COMPREPLY will suggest completion for "key|value|kv|all", even if
> >> one of those words has been used on the command line before already (I
> >> do not believe this is expected?). What about the following instead?
> >>
> >> _bpftool_one_of_list 'key value kv all'
> >
> > I'll make it more precise and correct. key|value|kv|all should go only
> > after `bpftool btf dump map id <id> <here>`, I'll do match based on
> > number of workds, similar to how it's done in prog attach/detach
> > handling.
>
> I think that bit was mostly correct. What you have is “if we're after
> map and it's not prog/map/id, always complete with key/value/kv/all”.
>
> What I'm proposing is “[in the same context], complete just once with
> only one of key/value/kv/all”. Like this:
>
> *)
> if _bpftool_search_list 'map'; then
> _bpftool_one_of_list 'key value kv all'
> fi
>
> With that you probably do not have to change it for word numbers.
> Although if you wish to go for them, it's fine too.
I still changed it, because in my follow up patches I'll add extra
options (e.g., to output BTF as compilable C), at which point this
script will start suggesting key|value|kv|all in the wrong place:
bpftool btf dump map id 111 compilable <cursor-is-here-now>
Now it will suggest `key value kv all`, but it's incorrect, so I want
to make sure those for go only after `map (id|pinned) <value> <here>`.
>
> >
> >
> >>
> >>> + fi
> >>> + return 0
> >>> + ;;
> >>
> >> Nit: It seems that the last bloc (the case when $prev matches on "*") is
> >> not correctly indented, it should be aligned with $command/prog/map/id?
> >
> > Copy/pasted from `prog dump`, I wasn't sure if that was intentional,
> > will fix both.
> >
> >>
> >> Other than this the completion seems good to me.
> >>
> >> But note that I did not receive all of your patches, only the cover
> >> letters (for v1 and v2 of the set) and this one (because I'm directly
> >> CC-ed?). It looks like the other patches for both v1 and v2 did not make
> >> it to the mailing lists or to patchwork, so you might want to double
> >> check and resend if necessary.
> >
> > Hm.. yeah, you are right, not sure what happened there. Neither bfp@
> > and netdev@ didn't get it, while other recipients did. I'll send out
> > v3 and will see if that still happens.
>
> Cool, thanks!
> Quentin
Powered by blists - more mailing lists