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

[Alexei Starovoitov <ast@fb.com>]

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?