On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote: > 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. > > Syntax: > $ bpftool cgroup list CGROUP > $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS] > $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG > > Signed-off-by: Roman Gushchin <g...@fb.com>
Looks good, a few very minor nits/questions below. > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c > new file mode 100644 > index 000000000000..88d67f74313f > --- /dev/null > +++ b/tools/bpf/bpftool/cgroup.c > @@ -0,0 +1,305 @@ > +/* > + * Copyright (C) 2017 Facebook > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + * > + * Author: Roman Gushchin <g...@fb.com> > + */ > + > +#include <fcntl.h> > +#include <stdlib.h> > +#include <string.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <unistd.h> > + > +#include <bpf.h> > + > +#include "main.h" > + > +static const char * const attach_type_strings[] = { > + [BPF_CGROUP_INET_INGRESS] = "ingress", > + [BPF_CGROUP_INET_EGRESS] = "egress", > + [BPF_CGROUP_INET_SOCK_CREATE] = "sock_create", > + [BPF_CGROUP_SOCK_OPS] = "sock_ops", > + [BPF_CGROUP_DEVICE] = "device", > + [__MAX_BPF_ATTACH_TYPE] = NULL, > +}; > + > +static enum bpf_attach_type parse_attach_type(const char *str) > +{ > + enum bpf_attach_type type; > + > + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) { > + if (attach_type_strings[type] && > + strcmp(str, attach_type_strings[type]) == 0) Here and for matching flags you use straight strcmp(), not our locally defined is_prefix(), is this intentional? is_prefix() allows abbreviations, like in iproute2 commands. E.g. this would work: # bpftool cg att /sys/fs/cgroup/test.slice/ dev id 1 allow_multi > + return type; > + } > + > + return __MAX_BPF_ATTACH_TYPE; > +} > +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) > +{ > + __u32 attach_flags; > + __u32 prog_ids[1024] = {0}; > + __u32 prog_cnt, iter; > + char *attach_flags_str; > + int ret; nit: could you reorder the variables so they're listed longest to shortest (reverse christmas tree)? > + prog_cnt = ARRAY_SIZE(prog_ids); > + ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids, > + &prog_cnt); > + if (ret) > + return ret; > + > + if (prog_cnt == 0) > + return 0; > + > + switch (attach_flags) { > + case BPF_F_ALLOW_MULTI: > + attach_flags_str = "allow_multi"; > + break; > + case BPF_F_ALLOW_OVERRIDE: > + attach_flags_str = "allow_override"; > + break; > + case 0: > + attach_flags_str = ""; > + break; > + default: > + attach_flags_str = "unknown"; nit: would it make sense to perhaps print flags in hex format in this case? > + } > + > + for (iter = 0; iter < prog_cnt; iter++) > + list_bpf_prog(prog_ids[iter], attach_type_strings[type], > + attach_flags_str); > + > + return 0; > +} > +static int do_attach(int argc, char **argv) > +{ > + int cgroup_fd, prog_fd; > + enum bpf_attach_type attach_type; > + int attach_flags = 0; > + int i; > + int ret = -1; > + > + if (argc < 4) { > + p_err("too few parameters for cgroup attach\n"); > + goto exit; > + } > + > + cgroup_fd = open(argv[0], O_RDONLY); > + if (cgroup_fd < 0) { > + p_err("can't open cgroup %s\n", argv[1]); > + goto exit; > + } > + > + attach_type = parse_attach_type(argv[1]); > + if (attach_type == __MAX_BPF_ATTACH_TYPE) { > + p_err("invalid attach type\n"); > + goto exit_cgroup; > + } > + > + argc -= 2; > + argv = &argv[2]; > + prog_fd = prog_parse_fd(&argc, &argv); > + if (prog_fd < 0) > + goto exit_cgroup; > + > + for (i = 0; i < argc; i++) { > + if (strcmp(argv[i], "allow_multi") == 0) { > + attach_flags |= BPF_F_ALLOW_MULTI; > + } else if (strcmp(argv[i], "allow_override") == 0) { > + attach_flags |= BPF_F_ALLOW_OVERRIDE; This is the other potential place for is_prefix() I referred to. That reminds me - would you care to also update Quentin's bash completions (tools/bpf/bpftool/bash-completion/bpftool)? They are _very_ nice to have in day to day use! > + } else { > + p_err("unknown option: %s\n", argv[i]); > + goto exit_cgroup; > + } > + } > + > + if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, attach_flags)) { > + p_err("failed to attach program"); > + 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; > +} Otherwise looks really nice, thanks for working on it!