netdev - Re: [PATCH v2 bpf-next 3/3] bpftool: add bash completions for btf command

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]
Message-ID: <CAEf4BzZF7zWGP24H7YJzCD3LLy4HVmsyKL-f2+1_N1+wq=ejQw@mail.gmail.com>
Date:   Thu, 25 Apr 2019 09:14:28 -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 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.


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

>
> Best,
> Quentin
>
> > +                    esac
> > +                    ;;
> > +                *)
> > +                    [[ $prev == $object ]] && \
> > +                        COMPREPLY=( $( compgen -W 'dump help' -- "$cur" ) )
> > +                    ;;
> > +            esac
> > +            ;;
> >          cgroup)
> >              case $command in
> >                  show|list)
> >
>