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!
