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

[Roman Gushchin <guro@fb.com>]

On Fri, Dec 08, 2017 at 03:39:43PM +0000, Quentin Monnet wrote:
> 2017-12-08 14:12 UTC+0000 ~ 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.
> 
> Do they? As far as I can tell the only other bpftool command that uses
> flags is the `bpftool map update`, and it does specify the possible
> values for UPDATE_FLAGS (and document them) in the man page.

You are right about UPDATE_FLAGS, but at the same time we do
not describe bpf program attributes in prog show:
  **bpftool prog show** [*PROG*]
	  Show information about loaded programs.  If *PROG* is
	  specified show information only about given program, otherwise
	  list all programs currently loaded on the system.

	  Output will start with program ID followed by program type and
	  zero or more named attributes (depending on kernel version).

I think, that actually ATTACH_TYPE is similar to PROGRAM_TYPE because
it will likely be extended in the following kernel versions.
So we should probably support specifying it in a numeric form too.

ATTACH_FLAGS are probably less volatile and will unlikely be extended often,
so we can describe them in docs and add a note about the kernel version
next time when a new flag will be added.

Anyway, I don't see any big problem in documenting current ATTACH_FLAG
and ATTACH_TYPE sets, if we think that it's a good way forward.

Thanks!