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. Thanks! :)
