Date: Fri, 8 Dec 2017 15:46:29 -0800
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
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
Content-Length: 3912
Lines: 157

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;
> +}