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!

Reply via email to