Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin <guro@fb.com>]

On Fri, Dec 08, 2017 at 10:34:16AM +0000, Quentin Monnet wrote:
> 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> > This patch adds basic cgroup bpf operations to bpftool:
> > cgroup list, attach and detach commands.
> > 
> > Usage is described in the corresponding man pages,
> > and examples are provided.
[...]
> > +MAP COMMANDS
> > +=============
> > +
> > +|	**bpftool** **cgroup list** *CGROUP*
> > +|	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> > +|	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> > +|	**bpftool** **cgroup help**
> > +|
> > +|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> 
> Could you please give the different possible values for ATTACH_TYPE and
> ATTACH_FLAGS, and provide some documentation for the flags?

I intentionally didn't include the list of possible values, as it depends
on the exact kernel version, and other bpftool docs are carefully avoiding
specifying such things.

It would be nice to have a way to ask the kernel about provided bpf program types,
attach types, etc; but I'm not sure that hardcoding it in bpftool docs is
a good idea.

> 
> > +
> > +DESCRIPTION
> > +===========
> > +	**bpftool cgroup list** *CGROUP*
> > +		  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 attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> > +		  Attach program *PROG* to the cgroup *CGROUP* with attach type
> > +		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
[...]
> > +
> > +	attach_type = parse_attach_type(argv[1]);
> > +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> > +		p_err("invalid attach type");
> > +		goto exit_cgroup;
> > +	}
> > +
> > +	argc -= 2;
> > +	argv = &argv[2];
> > +	prog_fd = prog_parse_fd(&argc, &argv);
> > +	if (prog_fd < 0)
> > +		goto exit_cgroup;
> > +
> > +	if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
> > +		p_err("failed to attach program");
> 
> Failed to *detach* instead of “attach”.

Fixed.

> 
> > +		goto exit_prog;
> > +	}
> > +
> > +	if (json_output)
> > +		jsonw_null(json_wtr);
> > +
> > +	ret = 0;
> > +
> > +exit_prog:
> > +	close(prog_fd);
> > +exit_cgroup:
> > +	close(cgroup_fd);
> > +exit:
> > +	return ret;
> > +}
> 
> […]
> 
> Very nice work on this v2, thanks a lot!
> Quentin

Thank you for reviewing!