Alexei Starovoitov <a...@fb.com> wrote on Mon [2019-Jun-24 18:23:28 -0700]:
> 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?
ya, looks fine to me.
--Takshak
