From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753687AbdLHXqf (ORCPT ); Fri, 8 Dec 2017 18:46:35 -0500 Received: from mail-qt0-f178.google.com ([209.85.216.178]:43758 "EHLO mail-qt0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753367AbdLHXqd (ORCPT ); Fri, 8 Dec 2017 18:46:33 -0500 X-Google-Smtp-Source: AGs4zMYOTTkDe9VyYwdzJY0US/g5Uv5xWuowwA7wKhLvG45x8b2gWz8S/1EKwCyvGE4ef2feCfUqfQ== Date: Fri, 8 Dec 2017 15:46:29 -0800 From: Jakub Kicinski To: Roman Gushchin Cc: , , , , , , Quentin Monnet , David Ahern Subject: Re: [PATCH v3 net-next 4/4] bpftool: implement cgroup bpf operations Message-ID: <20171208154629.20ace049@cakuba.netronome.com> In-Reply-To: <20171208145236.12635-5-guro@fb.com> References: <20171208145236.12635-1-guro@fb.com> <20171208145236.12635-5-guro@fb.com> Organization: Netronome Systems, Ltd. MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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; > +}