Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]

[Takshak Chahande <ctakshak@fb.com>]

Alexei Starovoitov <ast@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