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: <7bac058b-ca3c-5e20-ed6e-667a6602db7e@netronome.com>
Date:   Thu, 25 Apr 2019 17:33:51 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.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

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.

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