On 6/24/19 5:57 PM, Jakub Kicinski wrote:
> On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
>> I see. The local flag would not an option in getopt_long() sense, what
>> I was thinking was about adding an "effective" keyword:
>
> Something like this, untested:
>
> --->8------------
>
> The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
> as a subcommand option. We want to move away from global
> options, anyway.
>
> We need a global variable because of nftw limitations.
> Clean this flag on every invocations in case we run in
> batch mode.
>
> NOTE the argv[1] use on the error path in do_show() looks
> like a bug on it's own.
> ---
> .../bpftool/Documentation/bpftool-cgroup.rst | 24 +++----
> tools/bpf/bpftool/Documentation/bpftool.rst | 6 +-
> tools/bpf/bpftool/bash-completion/bpftool | 17 ++---
> tools/bpf/bpftool/cgroup.c | 62 ++++++++++++-------
> tools/bpf/bpftool/main.c | 7 +--
> tools/bpf/bpftool/main.h | 3 +-
> 6 files changed, 66 insertions(+), 53 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> index 324df15bf4cc..4fde3dfad395 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> @@ -12,8 +12,7 @@ SYNOPSIS
>
> **bpftool** [*OPTIONS*] **cgroup** *COMMAND*
>
> - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | {
> **-f** | **--bpffs** }
> - | { **-e** | **--effective** } }
> + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | {
> **-f** | **--bpffs** } }
>
> *COMMANDS* :=
> { **show** | **list** | **tree** | **attach** | **detach** | **help** }
> @@ -21,8 +20,8 @@ SYNOPSIS
> CGROUP COMMANDS
> ===============
>
> -| **bpftool** **cgroup { show | list }** *CGROUP*
> -| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
> +| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
> +| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
> | **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG*
> [*ATTACH_FLAGS*]
> | **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> | **bpftool** **cgroup help**
> @@ -35,13 +34,17 @@ CGROUP COMMANDS
>
> DESCRIPTION
> ===========
> - **bpftool cgroup { show | list }** *CGROUP*
> + **bpftool cgroup { show | list }** *CGROUP* [**effective**]
> List all programs attached to the cgroup *CGROUP*.
>
> Output will start with program ID followed by attach type,
> attach flags and program name.
>
> - **bpftool cgroup tree** [*CGROUP_ROOT*]
> + If **effective** is specified retrieve effective programs that
> + will execute for events within a cgroup. This includes
> + inherited along with attached ones.
> +
> + **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
> Iterate over all cgroups in *CGROUP_ROOT* and list all
> attached programs. If *CGROUP_ROOT* is not specified,
> bpftool uses cgroup v2 mountpoint.
> @@ -50,6 +53,10 @@ DESCRIPTION
> commands: it starts with absolute cgroup path, followed by
> program ID, attach type, attach flags and program name.
>
> + If **effective** is specified retrieve effective programs that
> + will execute for events within a cgroup. This includes
> + inherited along with attached ones.
> +
> **bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> Attach program *PROG* to the cgroup *CGROUP* with attach type
> *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
> @@ -122,11 +129,6 @@ OPTIONS
> Print all logs available from libbpf, including debug-level
> information.
>
> - -e, --effective
> - Retrieve effective programs that will execute for events
> - within a cgroup. This includes inherited along with attached
> - ones.
> -
> EXAMPLES
> ========
> |
> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst
> b/tools/bpf/bpftool/Documentation/bpftool.rst
> index d2f76b55988d..6a9c52ef84a9 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> @@ -19,7 +19,7 @@ SYNOPSIS
> *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** |
> **feature** }
>
> *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
> - | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** |
> **--effective** } }
> + | { **-j** | **--json** } [{ **-p** | **--pretty** }] }
>
> *MAP-COMMANDS* :=
> { **show** | **list** | **create** | **dump** | **update** | **lookup**
> | **getnext**
> @@ -71,10 +71,6 @@ OPTIONS
> includes logs from libbpf as well as from the verifier, when
> attempting to load programs.
>
> - -e, --effective
> - Retrieve effective programs that will execute for events
> - within a cgroup. This includes inherited along with attached
> ones.
> -
> SEE ALSO
> ========
> **bpf**\ (2),
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool
> b/tools/bpf/bpftool/bash-completion/bpftool
> index c98cb99867f6..de84ae06ae4e 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -187,7 +187,7 @@ _bpftool()
>
> # Deal with options
> if [[ ${words[cword]} == -* ]]; then
> - local c='--version --json --pretty --bpffs --mapcompat --debug
> --effective'
> + local c='--version --json --pretty --bpffs --mapcompat --debug'
> COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
> return 0
> fi
> @@ -678,12 +678,15 @@ _bpftool()
> ;;
> cgroup)
> case $command in
> - show|list)
> - _filedir
> - return 0
> - ;;
> - tree)
> - _filedir
> + show|list|tree)
> + case $cword in
> + 3)
> + _filedir
> + ;;
> + 4)
> + COMPREPLY=( $( compgen -W 'effective' -- "$cur"
> ) )
> + ;;
> + esac
> return 0
> ;;
> attach|detach)
> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index 1bb2a751107a..88b80616d47b 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -28,6 +28,8 @@
> " connect6 | sendmsg4 | sendmsg6 |\n" \
> " recvmsg4 | recvmsg6 | sysctl }"
>
> +static unsigned int query_flags;
> +
> static const char * const attach_type_strings[] = {
> [BPF_CGROUP_INET_INGRESS] = "ingress",
> [BPF_CGROUP_INET_EGRESS] = "egress",
> @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum
> bpf_attach_type type)
> __u32 prog_cnt = 0;
> int ret;
>
> - ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
> - &prog_cnt);
> + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
> + NULL, &prog_cnt);
> if (ret)
> return -1;
>
> @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum
> bpf_attach_type type,
> static int do_show(int argc, char **argv)
> {
> enum bpf_attach_type type;
> + const char *path;
> int cgroup_fd;
> int ret = -1;
>
> - if (argc < 1) {
> - p_err("too few parameters for cgroup show");
> - goto exit;
> - } else if (argc > 1) {
> - p_err("too many parameters for cgroup show");
> - goto exit;
> + query_flags = 0;
> +
> + if (!REQ_ARGS(1))
> + return -1;
> + path = GET_ARG();
> +
> + while (argc) {
> + if (is_prefix(*argv, "effective")) {
> + query_flags |= BPF_F_QUERY_EFFECTIVE;
> + NEXT_ARG();
> + } else {
> + p_err("expected no more arguments, 'effective', got:
> '%s'?",
> + *argv);
> + return -1;
> + }
> }
>
> - cgroup_fd = open(argv[0], O_RDONLY);
> + cgroup_fd = open(path, O_RDONLY);
> if (cgroup_fd < 0) {
> - p_err("can't open cgroup %s", argv[1]);
> + p_err("can't open cgroup %s", path);
> goto exit;
> }
>
> @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
> char *cgroup_root;
> int ret;
>
> - switch (argc) {
> - case 0:
> + query_flags = 0;
> +
> + if (!argc) {
> cgroup_root = find_cgroup_root();
> if (!cgroup_root) {
> p_err("cgroup v2 isn't mounted");
> return -1;
> }
> - break;
> - case 1:
> - cgroup_root = argv[0];
> - break;
> - default:
> - p_err("too many parameters for cgroup tree");
> - return -1;
> + } else {
> + cgroup_root = GET_ARG();
> +
> + while (argc) {
> + if (is_prefix(*argv, "effective")) {
> + query_flags |= BPF_F_QUERY_EFFECTIVE;
> + NEXT_ARG();
> + } else {
> + p_err("expected no more arguments, 'effective',
> got: '%s'?",
> + *argv);
> + return -1;
> + }
> + }
> }
>
> -
> if (json_output)
> jsonw_start_array(json_wtr);
> else
> @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
> }
>
> fprintf(stderr,
> - "Usage: %s %s { show | list } CGROUP\n"
> - " %s %s tree [CGROUP_ROOT]\n"
> + "Usage: %s %s { show | list } CGROUP [**effective**]\n"
> + " %s %s tree [CGROUP_ROOT] [**effective**]\n"
lgtm.
Takshak, Andrey, wdyt?
