From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Roman Gushchin <guro@fb.com>
Cc: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<kernel-team@fb.com>, <ast@kernel.org>, <daniel@iogearbox.net>,
<kafai@fb.com>, Quentin Monnet <quentin.monnet@netronome.com>,
David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH v3 net-next 4/4] bpftool: implement cgroup bpf operations
Date: Fri, 8 Dec 2017 15:46:29 -0800 [thread overview]
Message-ID: <20171208154629.20ace049@cakuba.netronome.com> (raw)
In-Reply-To: <20171208145236.12635-5-guro@fb.com>
On Fri, 8 Dec 2017 14:52:36 +0000, Roman Gushchin wrote:
> +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> +{
> + __u32 prog_ids[1024] = {0};
> + char *attach_flags_str;
> + __u32 prog_cnt, iter;
> + __u32 attach_flags;
> + char buf[16];
> + int ret;
> +
...
> + case BPF_F_ALLOW_OVERRIDE:
> + attach_flags_str = "allow_override";
> + break;
> + case 0:
> + attach_flags_str = "";
> + break;
> + default:
> + snprintf(buf, sizeof(buf), "unknown(%x)", attach_flags);
> + attach_flags_str = buf;
nit: theoretically speaking strlen("unknown()") == 9 + 8 + 1 > sizeof(buf)
so if we ever use all flags this may get truncated. I personally also
like using %x without 0x in front, but I restrained myself in bpftool
to avoid potential confusion (unknown(10) could be 10 or 16). Map flags
do put the prefix in, perhaps it would be good to keep those consistent?
> + }
> +
> + 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;
nit: I was hoping you'd fix the order of variables in all functions..
> + 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;
I don't feel about this strongly but as I said I was trying to follow
iproute2's conventions, and it allows aliasing. So if you type "ip a"
it will give you the first thing that starts with a, not necessarily
alphabetically, more likely in order of usefulness or order in which
things were added. IOW if "allow_" selects "allow_mutli" that's what I
would actually expect it to do..
Maybe others disagree?
> + } 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;
> +}
> +
> +static int do_detach(int argc, char **argv)
> +{
> + int prog_fd, cgroup_fd;
> + enum bpf_attach_type attach_type;
> + int ret = -1;
nit: order here too..
> + if (argc < 4) {
> + p_err("too few parameters for cgroup detach\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");
> + goto exit_cgroup;
> + }
> +
> + argc -= 2;
> + argv = &argv[2];
> + prog_fd = prog_parse_fd(&argc, &argv);
> + if (prog_fd < 0)
> + goto exit_cgroup;
> +
> + if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
> + p_err("failed to detach 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;
> +}
next prev parent reply other threads:[~2017-12-08 23:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-08 14:52 [PATCH v3 net-next 0/4] bpftool: cgroup bpf operations Roman Gushchin
2017-12-08 14:52 ` [PATCH v3 net-next 1/4] libbpf: add ability to guess program type based on section name Roman Gushchin
2017-12-08 14:52 ` [PATCH v3 net-next 2/4] libbpf: prefer global symbols as bpf program name source Roman Gushchin
2017-12-08 14:52 ` [PATCH v3 net-next 3/4] bpftool: implement prog load command Roman Gushchin
2017-12-08 14:52 ` [PATCH v3 net-next 4/4] bpftool: implement cgroup bpf operations Roman Gushchin
2017-12-08 23:46 ` Jakub Kicinski [this message]
2017-12-09 16:28 ` David Ahern
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171208154629.20ace049@cakuba.netronome.com \
--to=jakub.kicinski@netronome.com \
--cc=ast@kernel.org \
--cc=daniel@iogearbox.net \
--cc=dsahern@gmail.com \
--cc=guro@fb.com \
--cc=kafai@fb.com \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=quentin.monnet@netronome.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).