Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations

[Roman Gushchin <guro@fb.com>]

On Thu, Nov 30, 2017 at 07:04:54PM -0800, Jakub Kicinski wrote:
> Hi Roman!
> 
> On Thu, 30 Nov 2017 13:42:57 +0000, Roman Gushchin wrote:
> > This patchset adds basic cgroup bpf operations to bpftool.
> > 
> > Right now there is no convenient way to perform these operations.
> > The /samples/bpf/load_sock_ops.c implements attach/detacg operations,
> > but only for BPF_CGROUP_SOCK_OPS programs. Bps (part of bcc) implements
> > bpf introspection, but lacks any cgroup-related specific.
> > 
> > I find having a tool to perform these basic operations in the kernel tree
> > very useful, as it can be used in the corresponding bpf documentation
> > without creating additional dependencies. And bpftool seems to be
> > a right tool to extend with such functionality.
> 
> Could you place your code in a new file and add a new "object level"?
> I.e. 
> bpftool cgroup list 
> bpftool cgroup attach ...
> bpftool cgroup help
> etc?  Note that you probably want the list to be first, so if someone
> types "bpftool cg" it runs list by default.
> 
> Does it make sense to support pinned files and specifying programs by
> id?  I used the "id"/"pinned" keywords so that users can choose to use
> either.  Perhaps you should at least prefix the file to with "file"?
> So:
> $ bpftool cgattach file ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress
> $ bpftool cgattach id 19 /sys/fs/cgroup/user.slice/ ingress
> $ bpftool cgattach pin /bpf/prog /sys/fs/cgroup/user.slice/ ingress
> Would this make sense?
> 
> Smaller nits on the coding style:
>  - please try to run checkpatch, perhaps you did, but some people
>    forget tools are in the kernel tree :)
>  - please keep includes in alphabetical order;
>  - please keep variable declarations in functions ordered longest to
>    shortest, if that's impossible because of dependency between
>    initializers - move the initializers to the code.
> 
> Please also don't forget to update/create new man page.

Ok, I'll try to address these comments in v2.

Thank you!