* [PATCH bpf-next 1/3] bpftool: match several programs with same tag
2019-12-10 16:05 [PATCH bpf-next 0/3] bpftool: match programs and maps by names Paul Chaignon
@ 2019-12-10 16:06 ` Paul Chaignon
2019-12-10 17:29 ` Quentin Monnet
2019-12-10 20:36 ` Jakub Kicinski
2019-12-10 16:06 ` [PATCH bpf-next 2/3] bpftool: match programs by name Paul Chaignon
2019-12-10 16:06 ` [PATCH bpf-next 3/3] bpftool: match maps " Paul Chaignon
2 siblings, 2 replies; 13+ messages in thread
From: Paul Chaignon @ 2019-12-10 16:06 UTC (permalink / raw)
To: bpf
Cc: Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
When several BPF programs have the same tag, bpftool matches only the
first (in ID order). This patch changes that behavior such that dump and
show commands return all matched programs. Commands that require a single
program (e.g., pin and attach) will error out if given a tag that matches
several. bpftool prog dump will also error out if file or visual are
given and several programs have the given tag.
In the case of the dump command, a program header is added before each
dump only if the tag matches several programs; this patch doesn't change
the output if a single program matches.
Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
---
.../bpftool/Documentation/bpftool-prog.rst | 16 +-
tools/bpf/bpftool/prog.c | 371 ++++++++++++------
2 files changed, 272 insertions(+), 115 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 7a374b3c851d..d377d0cb7923 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -53,8 +53,10 @@ DESCRIPTION
===========
**bpftool prog { show | list }** [*PROG*]
Show information about loaded programs. If *PROG* is
- specified show information only about given program, otherwise
- list all programs currently loaded on the system.
+ specified show information only about given programs,
+ otherwise list all programs currently loaded on the system.
+ In case of **tag**, *PROG* may match several programs which
+ will all be shown.
Output will start with program ID followed by program type and
zero or more named attributes (depending on kernel version).
@@ -68,11 +70,15 @@ DESCRIPTION
performed via the **kernel.bpf_stats_enabled** sysctl knob.
**bpftool prog dump xlated** *PROG* [{ **file** *FILE* | **opcodes** | **visual** | **linum** }]
- Dump eBPF instructions of the program from the kernel. By
+ Dump eBPF instructions of the programs from the kernel. By
default, eBPF will be disassembled and printed to standard
output in human-readable format. In this case, **opcodes**
controls if raw opcodes should be printed as well.
+ In case of **tag**, *PROG* may match several programs which
+ will all be dumped. However, if **file** or **visual** is
+ specified, *PROG* must match a single program.
+
If **file** is specified, the binary image will instead be
written to *FILE*.
@@ -80,15 +86,17 @@ DESCRIPTION
built instead, and eBPF instructions will be presented with
CFG in DOT format, on standard output.
- If the prog has line_info available, the source line will
+ If the programs have line_info available, the source line will
be displayed by default. If **linum** is specified,
the filename, line number and line column will also be
displayed on top of the source line.
**bpftool prog dump jited** *PROG* [{ **file** *FILE* | **opcodes** | **linum** }]
Dump jited image (host machine code) of the program.
+
If *FILE* is specified image will be written to a file,
otherwise it will be disassembled and printed to stdout.
+ *PROG* must match a single program when **file** is specified.
**opcodes** controls if raw opcodes will be printed.
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 4535c863d2cd..ca4278269e73 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -25,6 +25,11 @@
#include "main.h"
#include "xlated_dumper.h"
+enum dump_mode {
+ DUMP_JITED,
+ DUMP_XLATED,
+};
+
static const char * const attach_type_strings[] = {
[BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
[BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
@@ -77,11 +82,13 @@ static void print_boot_time(__u64 nsecs, char *buf, unsigned int size)
strftime(buf, size, "%FT%T%z", &load_tm);
}
-static int prog_fd_by_tag(unsigned char *tag)
+static int
+prog_fd_by_tag(unsigned char *tag, int *fds)
{
unsigned int id = 0;
+ int fd, nb_fds = 0;
+ void *tmp;
int err;
- int fd;
while (true) {
struct bpf_prog_info info = {};
@@ -89,36 +96,54 @@ static int prog_fd_by_tag(unsigned char *tag)
err = bpf_prog_get_next_id(id, &id);
if (err) {
- p_err("%s", strerror(errno));
- return -1;
+ if (errno != ENOENT) {
+ p_err("%s", strerror(errno));
+ goto err_close_fds;
+ }
+ return nb_fds;
}
fd = bpf_prog_get_fd_by_id(id);
if (fd < 0) {
p_err("can't get prog by id (%u): %s",
id, strerror(errno));
- return -1;
+ goto err_close_fds;
}
err = bpf_obj_get_info_by_fd(fd, &info, &len);
if (err) {
p_err("can't get prog info (%u): %s",
id, strerror(errno));
- close(fd);
- return -1;
+ goto err_close_fd;
}
- if (!memcmp(tag, info.tag, BPF_TAG_SIZE))
- return fd;
+ if (memcmp(tag, info.tag, BPF_TAG_SIZE)) {
+ close(fd);
+ continue;
+ }
- close(fd);
+ if (nb_fds > 0) {
+ tmp = realloc(fds, (nb_fds + 1) * sizeof(int));
+ if (!tmp) {
+ p_err("failed to realloc");
+ goto err_close_fd;
+ }
+ fds = tmp;
+ }
+ fds[nb_fds++] = fd;
}
+
+err_close_fd:
+ close(fd);
+err_close_fds:
+ for (nb_fds--; nb_fds >= 0; nb_fds--)
+ close(fds[nb_fds]);
+ return -1;
}
-int prog_parse_fd(int *argc, char ***argv)
+static int
+prog_parse_fds(int *argc, char ***argv, int *fds)
{
- int fd;
-
if (is_prefix(**argv, "id")) {
unsigned int id;
char *endptr;
@@ -132,10 +157,12 @@ int prog_parse_fd(int *argc, char ***argv)
}
NEXT_ARGP();
- fd = bpf_prog_get_fd_by_id(id);
- if (fd < 0)
+ fds[0] = bpf_prog_get_fd_by_id(id);
+ if (fds[0] < 0) {
p_err("get by id (%u): %s", id, strerror(errno));
- return fd;
+ return -1;
+ }
+ return 1;
} else if (is_prefix(**argv, "tag")) {
unsigned char tag[BPF_TAG_SIZE];
@@ -149,7 +176,7 @@ int prog_parse_fd(int *argc, char ***argv)
}
NEXT_ARGP();
- return prog_fd_by_tag(tag);
+ return prog_fd_by_tag(tag, fds);
} else if (is_prefix(**argv, "pinned")) {
char *path;
@@ -158,13 +185,43 @@ int prog_parse_fd(int *argc, char ***argv)
path = **argv;
NEXT_ARGP();
- return open_obj_pinned_any(path, BPF_OBJ_PROG);
+ fds[0] = open_obj_pinned_any(path, BPF_OBJ_PROG);
+ if (fds[0] < 0)
+ return -1;
+ return 1;
}
p_err("expected 'id', 'tag' or 'pinned', got: '%s'?", **argv);
return -1;
}
+int prog_parse_fd(int *argc, char ***argv)
+{
+ int *fds = NULL;
+ int nb_fds, fd;
+
+ fds = malloc(sizeof(int));
+ if (!fds) {
+ p_err("mem alloc failed");
+ return -1;
+ }
+ nb_fds = prog_parse_fds(argc, argv, fds);
+ if (nb_fds != 1) {
+ if (nb_fds > 1) {
+ p_err("several programs match this handle");
+ for (nb_fds--; nb_fds >= 0; nb_fds--)
+ close(fds[nb_fds]);
+ }
+ fd = -1;
+ goto err_free;
+ }
+
+ fd = fds[0];
+err_free:
+ free(fds);
+ return fd;
+}
+
static void show_prog_maps(int fd, u32 num_maps)
{
struct bpf_prog_info info = {};
@@ -194,11 +251,9 @@ static void show_prog_maps(int fd, u32 num_maps)
}
}
-static void print_prog_json(struct bpf_prog_info *info, int fd)
+static void
+print_prog_header_json(struct bpf_prog_info *info)
{
- char *memlock;
-
- jsonw_start_object(json_wtr);
jsonw_uint_field(json_wtr, "id", info->id);
if (info->type < ARRAY_SIZE(prog_type_name))
jsonw_string_field(json_wtr, "type",
@@ -219,7 +274,15 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
jsonw_uint_field(json_wtr, "run_time_ns", info->run_time_ns);
jsonw_uint_field(json_wtr, "run_cnt", info->run_cnt);
}
+}
+static void
+print_prog_json(struct bpf_prog_info *info, int fd)
+{
+ char *memlock;
+
+ jsonw_start_object(json_wtr);
+ print_prog_header_json(info);
print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
if (info->load_time) {
@@ -268,10 +331,9 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
jsonw_end_object(json_wtr);
}
-static void print_prog_plain(struct bpf_prog_info *info, int fd)
+static void
+print_prog_header_plain(struct bpf_prog_info *info)
{
- char *memlock;
-
printf("%u: ", info->id);
if (info->type < ARRAY_SIZE(prog_type_name))
printf("%s ", prog_type_name[info->type]);
@@ -289,6 +351,14 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
printf(" run_time_ns %lld run_cnt %lld",
info->run_time_ns, info->run_cnt);
printf("\n");
+}
+
+static void
+print_prog_plain(struct bpf_prog_info *info, int fd)
+{
+ char *memlock;
+
+ print_prog_header_plain(info);
if (info->load_time) {
char buf[32];
@@ -351,21 +421,43 @@ static int show_prog(int fd)
static int do_show(int argc, char **argv)
{
+ int fd, nb_fds, i;
+ int *fds = NULL;
__u32 id = 0;
int err;
- int fd;
if (show_pinned)
build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
if (argc == 2) {
- fd = prog_parse_fd(&argc, &argv);
- if (fd < 0)
+ fds = malloc(sizeof(int));
+ if (!fds) {
+ p_err("mem alloc failed");
return -1;
+ }
+ nb_fds = prog_parse_fds(&argc, &argv, fds);
+ if (nb_fds < 1)
+ goto err_free;
- err = show_prog(fd);
- close(fd);
- return err;
+ if (json_output && nb_fds > 1)
+ jsonw_start_array(json_wtr); /* root array */
+ for (i = 0; i < nb_fds; i++) {
+ err = show_prog(fds[i]);
+ close(fds[i]);
+ if (err) {
+ for (i++; i < nb_fds; i++)
+ close(fds[i]);
+ goto err_free;
+ }
+ }
+ if (json_output && nb_fds > 1)
+ jsonw_end_array(json_wtr); /* root array */
+
+ return 0;
+
+err_free:
+ free(fds);
+ return -1;
}
if (argc)
@@ -408,101 +500,32 @@ static int do_show(int argc, char **argv)
return err;
}
-static int do_dump(int argc, char **argv)
+static int
+prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
+ char *filepath, bool opcodes, bool visual, bool linum)
{
- struct bpf_prog_info_linear *info_linear;
struct bpf_prog_linfo *prog_linfo = NULL;
- enum {DUMP_JITED, DUMP_XLATED} mode;
const char *disasm_opt = NULL;
- struct bpf_prog_info *info;
struct dump_data dd = {};
void *func_info = NULL;
struct btf *btf = NULL;
- char *filepath = NULL;
- bool opcodes = false;
- bool visual = false;
char func_sig[1024];
unsigned char *buf;
- bool linum = false;
__u32 member_len;
- __u64 arrays;
ssize_t n;
int fd;
- if (is_prefix(*argv, "jited")) {
- if (disasm_init())
- return -1;
- mode = DUMP_JITED;
- } else if (is_prefix(*argv, "xlated")) {
- mode = DUMP_XLATED;
- } else {
- p_err("expected 'xlated' or 'jited', got: %s", *argv);
- return -1;
- }
- NEXT_ARG();
-
- if (argc < 2)
- usage();
-
- fd = prog_parse_fd(&argc, &argv);
- if (fd < 0)
- return -1;
-
- if (is_prefix(*argv, "file")) {
- NEXT_ARG();
- if (!argc) {
- p_err("expected file path");
- return -1;
- }
-
- filepath = *argv;
- NEXT_ARG();
- } else if (is_prefix(*argv, "opcodes")) {
- opcodes = true;
- NEXT_ARG();
- } else if (is_prefix(*argv, "visual")) {
- visual = true;
- NEXT_ARG();
- } else if (is_prefix(*argv, "linum")) {
- linum = true;
- NEXT_ARG();
- }
-
- if (argc) {
- usage();
- return -1;
- }
-
- if (mode == DUMP_JITED)
- arrays = 1UL << BPF_PROG_INFO_JITED_INSNS;
- else
- arrays = 1UL << BPF_PROG_INFO_XLATED_INSNS;
-
- arrays |= 1UL << BPF_PROG_INFO_JITED_KSYMS;
- arrays |= 1UL << BPF_PROG_INFO_JITED_FUNC_LENS;
- arrays |= 1UL << BPF_PROG_INFO_FUNC_INFO;
- arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
- arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
-
- info_linear = bpf_program__get_prog_info_linear(fd, arrays);
- close(fd);
- if (IS_ERR_OR_NULL(info_linear)) {
- p_err("can't get prog info: %s", strerror(errno));
- return -1;
- }
-
- info = &info_linear->info;
if (mode == DUMP_JITED) {
if (info->jited_prog_len == 0) {
p_info("no instructions returned");
- goto err_free;
+ return -1;
}
buf = (unsigned char *)(info->jited_prog_insns);
member_len = info->jited_prog_len;
} else { /* DUMP_XLATED */
if (info->xlated_prog_len == 0) {
p_err("error retrieving insn dump: kernel.kptr_restrict set?");
- goto err_free;
+ return -1;
}
buf = (unsigned char *)info->xlated_prog_insns;
member_len = info->xlated_prog_len;
@@ -510,7 +533,7 @@ static int do_dump(int argc, char **argv)
if (info->btf_id && btf__get_from_id(info->btf_id, &btf)) {
p_err("failed to get btf");
- goto err_free;
+ return -1;
}
func_info = (void *)info->func_info;
@@ -526,7 +549,7 @@ static int do_dump(int argc, char **argv)
if (fd < 0) {
p_err("can't open file %s: %s", filepath,
strerror(errno));
- goto err_free;
+ return -1;
}
n = write(fd, buf, member_len);
@@ -534,7 +557,7 @@ static int do_dump(int argc, char **argv)
if (n != member_len) {
p_err("error writing output file: %s",
n < 0 ? strerror(errno) : "short write");
- goto err_free;
+ return -1;
}
if (json_output)
@@ -548,7 +571,7 @@ static int do_dump(int argc, char **argv)
info->netns_ino,
&disasm_opt);
if (!name)
- goto err_free;
+ return -1;
}
if (info->nr_jited_func_lens && info->jited_func_lens) {
@@ -643,11 +666,137 @@ static int do_dump(int argc, char **argv)
kernel_syms_destroy(&dd);
}
- free(info_linear);
+ return 0;
+}
+
+static int
+do_dump(int argc, char **argv)
+{
+ struct bpf_prog_info_linear *info_linear;
+ char *filepath = NULL;
+ bool opcodes = false;
+ bool visual = false;
+ enum dump_mode mode;
+ bool linum = false;
+ int *fds = NULL;
+ int nb_fds, i;
+ __u64 arrays;
+
+ if (is_prefix(*argv, "jited")) {
+ if (disasm_init())
+ return -1;
+ mode = DUMP_JITED;
+ } else if (is_prefix(*argv, "xlated")) {
+ mode = DUMP_XLATED;
+ } else {
+ p_err("expected 'xlated' or 'jited', got: %s", *argv);
+ return -1;
+ }
+ NEXT_ARG();
+
+ if (argc < 2)
+ usage();
+
+ fds = malloc(sizeof(int));
+ if (!fds) {
+ p_err("mem alloc failed");
+ return -1;
+ }
+ nb_fds = prog_parse_fds(&argc, &argv, fds);
+ if (nb_fds < 1)
+ goto err_free;
+
+ if (is_prefix(*argv, "file")) {
+ NEXT_ARG();
+ if (!argc) {
+ p_err("expected file path");
+ goto err_close;
+ }
+ if (nb_fds > 1) {
+ p_err("several programs matched");
+ goto err_close;
+ }
+
+ filepath = *argv;
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "opcodes")) {
+ opcodes = true;
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "visual")) {
+ if (nb_fds > 1) {
+ p_err("several programs matched");
+ goto err_close;
+ }
+
+ visual = true;
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "linum")) {
+ linum = true;
+ NEXT_ARG();
+ }
+
+ if (argc) {
+ usage();
+ goto err_close;
+ }
+
+ if (mode == DUMP_JITED)
+ arrays = 1UL << BPF_PROG_INFO_JITED_INSNS;
+ else
+ arrays = 1UL << BPF_PROG_INFO_XLATED_INSNS;
+
+ arrays |= 1UL << BPF_PROG_INFO_JITED_KSYMS;
+ arrays |= 1UL << BPF_PROG_INFO_JITED_FUNC_LENS;
+ arrays |= 1UL << BPF_PROG_INFO_FUNC_INFO;
+ arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
+ arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
+
+ if (json_output && nb_fds > 1)
+ jsonw_start_array(json_wtr); /* root array */
+ for (i = 0; i < nb_fds; i++) {
+ info_linear = bpf_program__get_prog_info_linear(fds[i], arrays);
+ close(fds[i]);
+ if (IS_ERR_OR_NULL(info_linear)) {
+ p_err("can't get prog info: %s", strerror(errno));
+ for (i++; i < nb_fds; i++)
+ close(fds[i]);
+ goto err_free;
+ }
+
+ if (json_output && nb_fds > 1) {
+ jsonw_start_object(json_wtr); /* prog object */
+ print_prog_header_json(&info_linear->info);
+ jsonw_name(json_wtr, "insns");
+ } else if (nb_fds > 1) {
+ print_prog_header_plain(&info_linear->info);
+ }
+
+ if (prog_dump(&info_linear->info, mode, filepath, opcodes,
+ visual, linum)) {
+ free(info_linear);
+ for (i++; i < nb_fds; i++)
+ close(fds[i]);
+ goto err_free;
+ }
+
+ if (json_output && nb_fds > 1)
+ jsonw_end_object(json_wtr); /* prog object */
+ else if (i != nb_fds - 1 && nb_fds > 1)
+ printf("\n");
+
+ free(info_linear);
+ }
+ if (json_output && nb_fds > 1)
+ jsonw_end_array(json_wtr); /* root array */
+
+ free(fds);
return 0;
+err_close:
+ for (i = 0; i < nb_fds; i++)
+ close(fds[i]);
err_free:
- free(info_linear);
+ free(fds);
return -1;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 1/3] bpftool: match several programs with same tag
2019-12-10 16:06 ` [PATCH bpf-next 1/3] bpftool: match several programs with same tag Paul Chaignon
@ 2019-12-10 17:29 ` Quentin Monnet
2019-12-13 18:10 ` Paul Chaignon
2019-12-10 20:36 ` Jakub Kicinski
1 sibling, 1 reply; 13+ messages in thread
From: Quentin Monnet @ 2019-12-10 17:29 UTC (permalink / raw)
To: Paul Chaignon, bpf
Cc: paul.chaignon, netdev, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko
Hi Paul,
2019-12-10 17:06 UTC+0100 ~ Paul Chaignon <paul.chaignon@orange.com>
> When several BPF programs have the same tag, bpftool matches only the
> first (in ID order). This patch changes that behavior such that dump and
> show commands return all matched programs. Commands that require a single
> program (e.g., pin and attach) will error out if given a tag that matches
> several. bpftool prog dump will also error out if file or visual are
> given and several programs have the given tag.
>
> In the case of the dump command, a program header is added before each
> dump only if the tag matches several programs; this patch doesn't change
> the output if a single program matches.
>
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> ---
> .../bpftool/Documentation/bpftool-prog.rst | 16 +-
> tools/bpf/bpftool/prog.c | 371 ++++++++++++------
> 2 files changed, 272 insertions(+), 115 deletions(-)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 4535c863d2cd..ca4278269e73 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -25,6 +25,11 @@
> #include "main.h"
> #include "xlated_dumper.h"
>
> +enum dump_mode {
> + DUMP_JITED,
> + DUMP_XLATED,
> +};
> +
> static const char * const attach_type_strings[] = {
> [BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
> [BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
> @@ -77,11 +82,13 @@ static void print_boot_time(__u64 nsecs, char *buf, unsigned int size)
> strftime(buf, size, "%FT%T%z", &load_tm);
> }
>
> -static int prog_fd_by_tag(unsigned char *tag)
> +static int
> +prog_fd_by_tag(unsigned char *tag, int *fds)
Nit: No line break necessary if it fits on one line.
(Sorry for misleading you on that in an earlier discussion :/)
> {
> unsigned int id = 0;
> + int fd, nb_fds = 0;
> + void *tmp;
> int err;
> - int fd;
>
> while (true) {
> struct bpf_prog_info info = {};
[...]
> @@ -351,21 +421,43 @@ static int show_prog(int fd)
>
> static int do_show(int argc, char **argv)
> {
> + int fd, nb_fds, i;
> + int *fds = NULL;
> __u32 id = 0;
> int err;
> - int fd;
>
> if (show_pinned)
> build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
>
> if (argc == 2) {
> - fd = prog_parse_fd(&argc, &argv);
> - if (fd < 0)
> + fds = malloc(sizeof(int));
> + if (!fds) {
> + p_err("mem alloc failed");
> return -1;
> + }
> + nb_fds = prog_parse_fds(&argc, &argv, fds);
> + if (nb_fds < 1)
> + goto err_free;
>
> - err = show_prog(fd);
> - close(fd);
> - return err;
> + if (json_output && nb_fds > 1)
> + jsonw_start_array(json_wtr); /* root array */
> + for (i = 0; i < nb_fds; i++) {
> + err = show_prog(fds[i]);
> + close(fds[i]);
> + if (err) {
> + for (i++; i < nb_fds; i++)
> + close(fds[i]);
> + goto err_free;
Alternatively, we could keep trying to list the remaining programs. For
example, if the system has a long list of BPF programs running and one
of them is removed while printing the list, we would still have the rest
of the list.
If we went this way, maybe just set err to non-zero if no program at all
could be printed?
> + }
> + }
> + if (json_output && nb_fds > 1)
> + jsonw_end_array(json_wtr); /* root array */
> +
> + return 0;
> +
> +err_free:
> + free(fds);
> + return -1;
> }
>
> if (argc)
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 1/3] bpftool: match several programs with same tag
2019-12-10 17:29 ` Quentin Monnet
@ 2019-12-13 18:10 ` Paul Chaignon
0 siblings, 0 replies; 13+ messages in thread
From: Paul Chaignon @ 2019-12-13 18:10 UTC (permalink / raw)
To: Quentin Monnet
Cc: bpf, paul.chaignon, netdev, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko
On Tue, Dec 10, 2019 at 05:29:18PM +0000, Quentin Monnet wrote:
> Hi Paul,
>
> 2019-12-10 17:06 UTC+0100 ~ Paul Chaignon <paul.chaignon@orange.com>
> > When several BPF programs have the same tag, bpftool matches only the
> > first (in ID order). This patch changes that behavior such that dump and
> > show commands return all matched programs. Commands that require a single
> > program (e.g., pin and attach) will error out if given a tag that matches
> > several. bpftool prog dump will also error out if file or visual are
> > given and several programs have the given tag.
> >
> > In the case of the dump command, a program header is added before each
> > dump only if the tag matches several programs; this patch doesn't change
> > the output if a single program matches.
> >
> > Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> > ---
> > .../bpftool/Documentation/bpftool-prog.rst | 16 +-
> > tools/bpf/bpftool/prog.c | 371 ++++++++++++------
> > 2 files changed, 272 insertions(+), 115 deletions(-)
> >
[...]
> > @@ -351,21 +421,43 @@ static int show_prog(int fd)
> >
> > static int do_show(int argc, char **argv)
> > {
> > + int fd, nb_fds, i;
> > + int *fds = NULL;
> > __u32 id = 0;
> > int err;
> > - int fd;
> >
> > if (show_pinned)
> > build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
> >
> > if (argc == 2) {
> > - fd = prog_parse_fd(&argc, &argv);
> > - if (fd < 0)
> > + fds = malloc(sizeof(int));
> > + if (!fds) {
> > + p_err("mem alloc failed");
> > return -1;
> > + }
> > + nb_fds = prog_parse_fds(&argc, &argv, fds);
> > + if (nb_fds < 1)
> > + goto err_free;
> >
> > - err = show_prog(fd);
> > - close(fd);
> > - return err;
> > + if (json_output && nb_fds > 1)
> > + jsonw_start_array(json_wtr); /* root array */
> > + for (i = 0; i < nb_fds; i++) {
> > + err = show_prog(fds[i]);
> > + close(fds[i]);
> > + if (err) {
> > + for (i++; i < nb_fds; i++)
> > + close(fds[i]);
> > + goto err_free;
>
> Alternatively, we could keep trying to list the remaining programs. For
> example, if the system has a long list of BPF programs running and one
> of them is removed while printing the list, we would still have the rest
> of the list.
As discussed off the list, since the kernel keeps a refcount for programs,
a program won't be removed while printing the list, not as long as we hold
an fd to it. Unless there's another common case of failure, I'm going to
keep the current behavior in the v2 and break out of the loop on errors
(though, with the appropriate JSON array closing).
>
> If we went this way, maybe just set err to non-zero if no program at all
> could be printed?
>
> > + }
> > + }
> > + if (json_output && nb_fds > 1)
> > + jsonw_end_array(json_wtr); /* root array */
> > +
> > + return 0;
> > +
> > +err_free:
> > + free(fds);
> > + return -1;
> > }
> >
> > if (argc)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/3] bpftool: match several programs with same tag
2019-12-10 16:06 ` [PATCH bpf-next 1/3] bpftool: match several programs with same tag Paul Chaignon
2019-12-10 17:29 ` Quentin Monnet
@ 2019-12-10 20:36 ` Jakub Kicinski
2019-12-13 12:39 ` Paul Chaignon
1 sibling, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2019-12-10 20:36 UTC (permalink / raw)
To: Paul Chaignon
Cc: bpf, Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
On Tue, 10 Dec 2019 17:06:25 +0100, Paul Chaignon wrote:
> When several BPF programs have the same tag, bpftool matches only the
> first (in ID order). This patch changes that behavior such that dump and
> show commands return all matched programs. Commands that require a single
> program (e.g., pin and attach) will error out if given a tag that matches
> several. bpftool prog dump will also error out if file or visual are
> given and several programs have the given tag.
>
> In the case of the dump command, a program header is added before each
> dump only if the tag matches several programs; this patch doesn't change
> the output if a single program matches.
How does this work? Could you add examples to the commit message?
This header idea doesn't seem correct, aren't id and other per-instance
fields only printed once?
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> - close(fd);
> + if (nb_fds > 0) {
> + tmp = realloc(fds, (nb_fds + 1) * sizeof(int));
> + if (!tmp) {
> + p_err("failed to realloc");
> + goto err_close_fd;
> + }
> + fds = tmp;
How does this work? the new array is never returned to the caller, and
the caller will most likely access freed memory, no?
> + }
> + fds[nb_fds++] = fd;
> }
> +
> +err_close_fd:
> + close(fd);
> +err_close_fds:
> + for (nb_fds--; nb_fds >= 0; nb_fds--)
> + close(fds[nb_fds]);
> + return -1;
> }
> +int prog_parse_fd(int *argc, char ***argv)
> +{
> + int *fds = NULL;
> + int nb_fds, fd;
> +
> + fds = malloc(sizeof(int));
> + if (!fds) {
> + p_err("mem alloc failed");
> + return -1;
> + }
> + nb_fds = prog_parse_fds(argc, argv, fds);
> + if (nb_fds != 1) {
> + if (nb_fds > 1) {
> + p_err("several programs match this handle");
> + for (nb_fds--; nb_fds >= 0; nb_fds--)
nit: since you checked nb_fds is positive, while (nb_fds--) ?
> + close(fds[nb_fds]);
> + }
> + fd = -1;
> + goto err_free;
> + }
> +
> + fd = fds[0];
> +err_free:
nit: we tried to call the labels exit_xyz if the code is used on both
error and success path, but maybe that pattern got lost over time.
> + free(fds);
> + return fd;
> +}
> +
> static void show_prog_maps(int fd, u32 num_maps)
> {
> struct bpf_prog_info info = {};
> static int do_show(int argc, char **argv)
> {
> + int fd, nb_fds, i;
> + int *fds = NULL;
> __u32 id = 0;
> int err;
> - int fd;
>
> if (show_pinned)
> build_pinned_obj_table(&prog_table, BPF_OBJ_PROG);
>
> if (argc == 2) {
> - fd = prog_parse_fd(&argc, &argv);
> - if (fd < 0)
> + fds = malloc(sizeof(int));
> + if (!fds) {
> + p_err("mem alloc failed");
> return -1;
> + }
> + nb_fds = prog_parse_fds(&argc, &argv, fds);
> + if (nb_fds < 1)
> + goto err_free;
>
> - err = show_prog(fd);
> - close(fd);
> - return err;
> + if (json_output && nb_fds > 1)
> + jsonw_start_array(json_wtr); /* root array */
> + for (i = 0; i < nb_fds; i++) {
> + err = show_prog(fds[i]);
> + close(fds[i]);
> + if (err) {
> + for (i++; i < nb_fds; i++)
> + close(fds[i]);
> + goto err_free;
I'm 90% sure JSON arrays close/end themselves on exit 🤔
> + }
> + }
> + if (json_output && nb_fds > 1)
> + jsonw_end_array(json_wtr); /* root array */
> +
> + return 0;
> +
> +err_free:
> + free(fds);
> + return -1;
Perhaps move the argc == 2 code to a separate function?
> }
>
> if (argc)
> @@ -408,101 +500,32 @@ static int do_show(int argc, char **argv)
> return err;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 1/3] bpftool: match several programs with same tag
2019-12-10 20:36 ` Jakub Kicinski
@ 2019-12-13 12:39 ` Paul Chaignon
0 siblings, 0 replies; 13+ messages in thread
From: Paul Chaignon @ 2019-12-13 12:39 UTC (permalink / raw)
To: Jakub Kicinski
Cc: bpf, Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
On Tue, Dec 10, 2019 at 12:36:25PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 17:06:25 +0100, Paul Chaignon wrote:
> > When several BPF programs have the same tag, bpftool matches only the
> > first (in ID order). This patch changes that behavior such that dump and
> > show commands return all matched programs. Commands that require a single
> > program (e.g., pin and attach) will error out if given a tag that matches
> > several. bpftool prog dump will also error out if file or visual are
> > given and several programs have the given tag.
> >
> > In the case of the dump command, a program header is added before each
> > dump only if the tag matches several programs; this patch doesn't change
> > the output if a single program matches.
>
> How does this work? Could you add examples to the commit message?
>
> This header idea doesn't seem correct, aren't id and other per-instance
> fields only printed once?
Sorry, that was unclear. What I call the header here is the first line
from the prog show output (in the case of plain output). So the output
when multiple programs match looks as follows. When a single program
matches, the first line (with the ID, type, name, tag and license) is
omitted.
$ ./bpftool prog dump xlated tag 6deef7357e7b4530
3: cgroup_skb tag 6deef7357e7b4530 gpl
0: (bf) r6 = r1
[...]
7: (95) exit
4: cgroup_skb tag 6deef7357e7b4530 gpl
0: (bf) r6 = r1
[...]
7: (95) exit
>
> > Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
>
> > - close(fd);
> > + if (nb_fds > 0) {
> > + tmp = realloc(fds, (nb_fds + 1) * sizeof(int));
> > + if (!tmp) {
> > + p_err("failed to realloc");
> > + goto err_close_fd;
> > + }
> > + fds = tmp;
>
> How does this work? the new array is never returned to the caller, and
> the caller will most likely access freed memory, no?
Oh, this is bad. Yes, fds should actually be "int **" and this line
should be "*fds = tmp;". I'll fix it in v2.
[...]
> > + close(fds[nb_fds]);
> > + }
> > + fd = -1;
> > + goto err_free;
> > + }
> > +
> > + fd = fds[0];
> > +err_free:
>
> nit: we tried to call the labels exit_xyz if the code is used on both
> error and success path, but maybe that pattern got lost over time.
Seems lost in prog.c but still valid across bpftool. I'll make the
change.
[...]
Paul
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 2/3] bpftool: match programs by name
2019-12-10 16:05 [PATCH bpf-next 0/3] bpftool: match programs and maps by names Paul Chaignon
2019-12-10 16:06 ` [PATCH bpf-next 1/3] bpftool: match several programs with same tag Paul Chaignon
@ 2019-12-10 16:06 ` Paul Chaignon
2019-12-10 17:29 ` Quentin Monnet
2019-12-10 21:04 ` Jakub Kicinski
2019-12-10 16:06 ` [PATCH bpf-next 3/3] bpftool: match maps " Paul Chaignon
2 siblings, 2 replies; 13+ messages in thread
From: Paul Chaignon @ 2019-12-10 16:06 UTC (permalink / raw)
To: bpf
Cc: Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
When working with frequently modified BPF programs, both the ID and the
tag may change. bpftool currently doesn't provide a "stable" way to match
such programs.
This patch implements lookup by name for programs. The show and dump
commands will return all programs with the given name, whereas other
commands will error out if several programs have the same name.
Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
---
.../bpf/bpftool/Documentation/bpftool-map.rst | 2 +-
.../bpftool/Documentation/bpftool-prog.rst | 12 +++++-----
tools/bpf/bpftool/bash-completion/bpftool | 22 ++++++++++++-----
tools/bpf/bpftool/main.h | 2 +-
tools/bpf/bpftool/prog.c | 24 +++++++++++++++----
5 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 1c0f7146aab0..90d848b5b7d3 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -41,7 +41,7 @@ MAP COMMANDS
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
| *DATA* := { [**hex**] *BYTES* }
-| *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+| *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
| *VALUE* := { *DATA* | *MAP* | *PROG* }
| *UPDATE_FLAGS* := { **any** | **exist** | **noexist** }
| *TYPE* := { **hash** | **array** | **prog_array** | **perf_event_array** | **percpu_hash**
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index d377d0cb7923..64ddf8a4c518 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -33,7 +33,7 @@ PROG COMMANDS
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
-| *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+| *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
| *TYPE* := {
| **socket** | **kprobe** | **kretprobe** | **classifier** | **action** |
| **tracepoint** | **raw_tracepoint** | **xdp** | **perf_event** | **cgroup/skb** |
@@ -55,8 +55,8 @@ DESCRIPTION
Show information about loaded programs. If *PROG* is
specified show information only about given programs,
otherwise list all programs currently loaded on the system.
- In case of **tag**, *PROG* may match several programs which
- will all be shown.
+ In case of **tag** or **name**, *PROG* may match several
+ programs which will all be shown.
Output will start with program ID followed by program type and
zero or more named attributes (depending on kernel version).
@@ -75,9 +75,9 @@ DESCRIPTION
output in human-readable format. In this case, **opcodes**
controls if raw opcodes should be printed as well.
- In case of **tag**, *PROG* may match several programs which
- will all be dumped. However, if **file** or **visual** is
- specified, *PROG* must match a single program.
+ In case of **tag** or **name**, *PROG* may match several
+ programs which will all be dumped. However, if **file** or
+ **visual** is specified, *PROG* must match a single program.
If **file** is specified, the binary image will instead be
written to *FILE*.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 70493a6da206..05b5be4a6ef9 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -71,6 +71,12 @@ _bpftool_get_prog_tags()
command sed -n 's/.*"tag": "\(.*\)",$/\1/p' )" -- "$cur" ) )
}
+_bpftool_get_prog_names()
+{
+ COMPREPLY+=( $( compgen -W "$( bpftool -jp prog 2>&1 | \
+ command sed -n 's/.*"name": "\(.*\)",$/\1/p' )" -- "$cur" ) )
+}
+
_bpftool_get_btf_ids()
{
COMPREPLY+=( $( compgen -W "$( bpftool -jp btf 2>&1 | \
@@ -201,6 +207,10 @@ _bpftool()
_bpftool_get_prog_tags
return 0
;;
+ name)
+ _bpftool_get_prog_names
+ return 0
+ ;;
dev)
_sysfs_get_netdevs
return 0
@@ -263,7 +273,7 @@ _bpftool()
;;
esac
- local PROG_TYPE='id pinned tag'
+ local PROG_TYPE='id pinned tag name'
local MAP_TYPE='id pinned'
case $command in
show|list)
@@ -559,7 +569,7 @@ _bpftool()
return 0
;;
prog_array)
- local PROG_TYPE='id pinned tag'
+ local PROG_TYPE='id pinned tag name'
COMPREPLY+=( $( compgen -W "$PROG_TYPE" \
-- "$cur" ) )
return 0
@@ -644,7 +654,7 @@ _bpftool()
esac
;;
btf)
- local PROG_TYPE='id pinned tag'
+ local PROG_TYPE='id pinned tag name'
local MAP_TYPE='id pinned'
case $command in
dump)
@@ -735,7 +745,7 @@ _bpftool()
connect6 sendmsg4 sendmsg6 recvmsg4 recvmsg6 sysctl \
getsockopt setsockopt'
local ATTACH_FLAGS='multi override'
- local PROG_TYPE='id pinned tag'
+ local PROG_TYPE='id pinned tag name'
case $prev in
$command)
_filedir
@@ -760,7 +770,7 @@ _bpftool()
elif [[ "$command" == "attach" ]]; then
# We have an attach type on the command line,
# but it is not the previous word, or
- # "id|pinned|tag" (we already checked for
+ # "id|pinned|tag|name" (we already checked for
# that). This should only leave the case when
# we need attach flags for "attach" commamnd.
_bpftool_one_of_list "$ATTACH_FLAGS"
@@ -786,7 +796,7 @@ _bpftool()
esac
;;
net)
- local PROG_TYPE='id pinned tag'
+ local PROG_TYPE='id pinned tag name'
local ATTACH_TYPES='xdp xdpgeneric xdpdrv xdpoffload'
case $command in
show|list)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 2899095f8254..a7ead7bb9447 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -42,7 +42,7 @@
#define BPF_TAG_FMT "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
#define HELP_SPEC_PROGRAM \
- "PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }"
+ "PROG := { id PROG_ID | pinned FILE | tag PROG_TAG | name PROG_NAME }"
#define HELP_SPEC_OPTIONS \
"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n" \
"\t {-m|--mapcompat} | {-n|--nomount} }"
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index ca4278269e73..8d3eedf7734d 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -83,7 +83,7 @@ static void print_boot_time(__u64 nsecs, char *buf, unsigned int size)
}
static int
-prog_fd_by_tag(unsigned char *tag, int *fds)
+prog_fd_by_nametag(char *nametag, int *fds, bool tag)
{
unsigned int id = 0;
int fd, nb_fds = 0;
@@ -117,7 +117,8 @@ prog_fd_by_tag(unsigned char *tag, int *fds)
goto err_close_fd;
}
- if (memcmp(tag, info.tag, BPF_TAG_SIZE)) {
+ if ((tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) ||
+ (!tag && strncmp(nametag, info.name, BPF_OBJ_NAME_LEN))) {
close(fd);
continue;
}
@@ -164,7 +165,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
}
return 1;
} else if (is_prefix(**argv, "tag")) {
- unsigned char tag[BPF_TAG_SIZE];
+ char tag[BPF_TAG_SIZE];
NEXT_ARGP();
@@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
}
NEXT_ARGP();
- return prog_fd_by_tag(tag, fds);
+ return prog_fd_by_nametag(tag, fds, true);
+ } else if (is_prefix(**argv, "name")) {
+ char *name;
+
+ NEXT_ARGP();
+
+ name = **argv;
+ if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
+ p_err("can't parse name");
+ return -1;
+ }
+ NEXT_ARGP();
+
+ return prog_fd_by_nametag(name, fds, false);
} else if (is_prefix(**argv, "pinned")) {
char *path;
@@ -191,7 +205,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
return 1;
}
- p_err("expected 'id', 'tag' or 'pinned', got: '%s'?", **argv);
+ p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
return -1;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpftool: match programs by name
2019-12-10 16:06 ` [PATCH bpf-next 2/3] bpftool: match programs by name Paul Chaignon
@ 2019-12-10 17:29 ` Quentin Monnet
2019-12-10 21:04 ` Jakub Kicinski
1 sibling, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-12-10 17:29 UTC (permalink / raw)
To: Paul Chaignon, bpf
Cc: paul.chaignon, netdev, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko
2019-12-10 17:06 UTC+0100 ~ Paul Chaignon <paul.chaignon@orange.com>
> When working with frequently modified BPF programs, both the ID and the
> tag may change. bpftool currently doesn't provide a "stable" way to match
> such programs.
>
> This patch implements lookup by name for programs. The show and dump
> commands will return all programs with the given name, whereas other
> commands will error out if several programs have the same name.
>
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Thanks!
Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 2/3] bpftool: match programs by name
2019-12-10 16:06 ` [PATCH bpf-next 2/3] bpftool: match programs by name Paul Chaignon
2019-12-10 17:29 ` Quentin Monnet
@ 2019-12-10 21:04 ` Jakub Kicinski
2019-12-13 12:40 ` Paul Chaignon
1 sibling, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2019-12-10 21:04 UTC (permalink / raw)
To: Paul Chaignon
Cc: bpf, Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
On Tue, 10 Dec 2019 17:06:42 +0100, Paul Chaignon wrote:
> When working with frequently modified BPF programs, both the ID and the
> tag may change. bpftool currently doesn't provide a "stable" way to match
> such programs.
>
> This patch implements lookup by name for programs. The show and dump
> commands will return all programs with the given name, whereas other
> commands will error out if several programs have the same name.
>
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> @@ -164,7 +165,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> }
> return 1;
> } else if (is_prefix(**argv, "tag")) {
> - unsigned char tag[BPF_TAG_SIZE];
> + char tag[BPF_TAG_SIZE];
Perhaps better to change the argument to prog_fd_by_nametag() to void *?
>
> NEXT_ARGP();
>
> @@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> }
> NEXT_ARGP();
>
> - return prog_fd_by_tag(tag, fds);
> + return prog_fd_by_nametag(tag, fds, true);
> + } else if (is_prefix(**argv, "name")) {
> + char *name;
> +
> + NEXT_ARGP();
> +
> + name = **argv;
> + if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
Is this needed? strncmp will simply never match, is it preferred to
hard error?
> + p_err("can't parse name");
> + return -1;
> + }
> + NEXT_ARGP();
> +
> + return prog_fd_by_nametag(name, fds, false);
> } else if (is_prefix(**argv, "pinned")) {
> char *path;
>
> @@ -191,7 +205,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> return 1;
> }
>
> - p_err("expected 'id', 'tag' or 'pinned', got: '%s'?", **argv);
> + p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
> return -1;
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpftool: match programs by name
2019-12-10 21:04 ` Jakub Kicinski
@ 2019-12-13 12:40 ` Paul Chaignon
2019-12-13 17:56 ` Jakub Kicinski
0 siblings, 1 reply; 13+ messages in thread
From: Paul Chaignon @ 2019-12-13 12:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: bpf, Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
On Tue, Dec 10, 2019 at 01:04:13PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 17:06:42 +0100, Paul Chaignon wrote:
> > When working with frequently modified BPF programs, both the ID and the
> > tag may change. bpftool currently doesn't provide a "stable" way to match
> > such programs.
> >
> > This patch implements lookup by name for programs. The show and dump
> > commands will return all programs with the given name, whereas other
> > commands will error out if several programs have the same name.
> >
> > Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
>
> > @@ -164,7 +165,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> > }
> > return 1;
> > } else if (is_prefix(**argv, "tag")) {
> > - unsigned char tag[BPF_TAG_SIZE];
> > + char tag[BPF_TAG_SIZE];
>
> Perhaps better to change the argument to prog_fd_by_nametag() to void *?
>
> >
> > NEXT_ARGP();
> >
> > @@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> > }
> > NEXT_ARGP();
> >
> > - return prog_fd_by_tag(tag, fds);
> > + return prog_fd_by_nametag(tag, fds, true);
> > + } else if (is_prefix(**argv, "name")) {
> > + char *name;
> > +
> > + NEXT_ARGP();
> > +
> > + name = **argv;
> > + if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
>
> Is this needed? strncmp will simply never match, is it preferred to
> hard error?
I tried to follow the fail-early pattern of lookups by tag above. I do
like that there's a different error message for a longer than expected
name. Since libbpf silently truncates names, typing a longer name is
not uncommon.
[...]
Paul
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 2/3] bpftool: match programs by name
2019-12-13 12:40 ` Paul Chaignon
@ 2019-12-13 17:56 ` Jakub Kicinski
0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2019-12-13 17:56 UTC (permalink / raw)
To: Paul Chaignon
Cc: bpf, Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
On Fri, 13 Dec 2019 13:40:38 +0100, Paul Chaignon wrote:
> > > @@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> > > }
> > > NEXT_ARGP();
> > >
> > > - return prog_fd_by_tag(tag, fds);
> > > + return prog_fd_by_nametag(tag, fds, true);
> > > + } else if (is_prefix(**argv, "name")) {
> > > + char *name;
> > > +
> > > + NEXT_ARGP();
> > > +
> > > + name = **argv;
> > > + if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
> >
> > Is this needed? strncmp will simply never match, is it preferred to
> > hard error?
>
> I tried to follow the fail-early pattern of lookups by tag above.
Right although tag does a scanf and if we didn't scan all letters
we'd use uninit memory.
> I do like that there's a different error message for a longer than
> expected name. Since libbpf silently truncates names, typing a
> longer name is not uncommon.
Ugh, I didn't realize libbpf truncates names. Okay, let's keep the
error for now so we can switch to truncation if users complain.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 3/3] bpftool: match maps by name
2019-12-10 16:05 [PATCH bpf-next 0/3] bpftool: match programs and maps by names Paul Chaignon
2019-12-10 16:06 ` [PATCH bpf-next 1/3] bpftool: match several programs with same tag Paul Chaignon
2019-12-10 16:06 ` [PATCH bpf-next 2/3] bpftool: match programs by name Paul Chaignon
@ 2019-12-10 16:06 ` Paul Chaignon
2019-12-10 17:29 ` Quentin Monnet
2 siblings, 1 reply; 13+ messages in thread
From: Paul Chaignon @ 2019-12-10 16:06 UTC (permalink / raw)
To: bpf
Cc: Quentin Monnet, paul.chaignon, netdev, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko
This patch implements lookup by name for maps and changes the behavior of
lookups by tag to be consistent with prog subcommands. Similarly to
program subcommands, the show and dump commands will return all maps with
the given name (or tag), whereas other commands will error out if several
maps have the same name (resp. tag).
When a map has BTF info, it is dumped in JSON with available BTF info.
This patch requires that all matched maps have BTF info before switching
the output format to JSON.
Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
---
.../bpf/bpftool/Documentation/bpftool-map.rst | 10 +-
tools/bpf/bpftool/bash-completion/bpftool | 131 ++++++-
tools/bpf/bpftool/main.h | 2 +-
tools/bpf/bpftool/map.c | 366 +++++++++++++++---
4 files changed, 432 insertions(+), 77 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 90d848b5b7d3..cdeae8ae90ba 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -39,7 +39,7 @@ MAP COMMANDS
| **bpftool** **map freeze** *MAP*
| **bpftool** **map help**
|
-| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
+| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* | **name** *MAP_NAME* }
| *DATA* := { [**hex**] *BYTES* }
| *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
| *VALUE* := { *DATA* | *MAP* | *PROG* }
@@ -55,8 +55,9 @@ DESCRIPTION
===========
**bpftool map { show | list }** [*MAP*]
Show information about loaded maps. If *MAP* is specified
- show information only about given map, otherwise list all
- maps currently loaded on the system.
+ show information only about given maps, otherwise list all
+ maps currently loaded on the system. In case of **name**,
+ *MAP* may match several maps which will all be shown.
Output will start with map ID followed by map type and
zero or more named attributes (depending on kernel version).
@@ -66,7 +67,8 @@ DESCRIPTION
as *FILE*.
**bpftool map dump** *MAP*
- Dump all entries in a given *MAP*.
+ Dump all entries in a given *MAP*. In case of **name**,
+ *MAP* may match several maps which will all be dumped.
**bpftool map update** *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*]
Update map entry for a given *KEY*.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 05b5be4a6ef9..21c676a1eeb1 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -59,6 +59,21 @@ _bpftool_get_map_ids_for_type()
command sed -n 's/.*"id": \(.*\),$/\1/p' )" -- "$cur" ) )
}
+_bpftool_get_map_names()
+{
+ COMPREPLY+=( $( compgen -W "$( bpftool -jp map 2>&1 | \
+ command sed -n 's/.*"name": \(.*\),$/\1/p' )" -- "$cur" ) )
+}
+
+# Takes map type and adds matching map names to the list of suggestions.
+_bpftool_get_map_names_for_type()
+{
+ local type="$1"
+ COMPREPLY+=( $( compgen -W "$( bpftool -jp map 2>&1 | \
+ command grep -C2 "$type" | \
+ command sed -n 's/.*"name": \(.*\),$/\1/p' )" -- "$cur" ) )
+}
+
_bpftool_get_prog_ids()
{
COMPREPLY+=( $( compgen -W "$( bpftool -jp prog 2>&1 | \
@@ -186,6 +201,52 @@ _bpftool_map_update_get_id()
esac
}
+_bpftool_map_update_get_name()
+{
+ local command="$1"
+
+ # Is it the map to update, or a map to insert into the map to update?
+ # Search for "value" keyword.
+ local idx value
+ for (( idx=7; idx < ${#words[@]}-1; idx++ )); do
+ if [[ ${words[idx]} == "value" ]]; then
+ value=1
+ break
+ fi
+ done
+ if [[ $value -eq 0 ]]; then
+ case "$command" in
+ push)
+ _bpftool_get_map_names_for_type stack
+ ;;
+ enqueue)
+ _bpftool_get_map_names_for_type queue
+ ;;
+ *)
+ _bpftool_get_map_names
+ ;;
+ esac
+ return 0
+ fi
+
+ # Name to complete is for a value. It can be either prog name or map name. This
+ # depends on the type of the map to update.
+ local type=$(_bpftool_map_guess_map_type)
+ case $type in
+ array_of_maps|hash_of_maps)
+ _bpftool_get_map_names
+ return 0
+ ;;
+ prog_array)
+ _bpftool_get_prog_names
+ return 0
+ ;;
+ *)
+ return 0
+ ;;
+ esac
+}
+
_bpftool()
{
local cur prev words objword
@@ -207,10 +268,6 @@ _bpftool()
_bpftool_get_prog_tags
return 0
;;
- name)
- _bpftool_get_prog_names
- return 0
- ;;
dev)
_sysfs_get_netdevs
return 0
@@ -261,7 +318,8 @@ _bpftool()
# Completion depends on object and command in use
case $object in
prog)
- # Complete id, only for subcommands that use prog (but no map) ids
+ # Complete id and name, only for subcommands that use prog (but no
+ # map) ids/names.
case $command in
show|list|dump|pin)
case $prev in
@@ -269,12 +327,16 @@ _bpftool()
_bpftool_get_prog_ids
return 0
;;
+ name)
+ _bpftool_get_prog_names
+ return 0
+ ;;
esac
;;
esac
local PROG_TYPE='id pinned tag name'
- local MAP_TYPE='id pinned'
+ local MAP_TYPE='id pinned name'
case $command in
show|list)
[[ $prev != "$command" ]] && return 0
@@ -325,6 +387,9 @@ _bpftool()
id)
_bpftool_get_prog_ids
;;
+ name)
+ _bpftool_get_map_names
+ ;;
pinned)
_filedir
;;
@@ -345,6 +410,9 @@ _bpftool()
id)
_bpftool_get_map_ids
;;
+ name)
+ _bpftool_get_map_names
+ ;;
pinned)
_filedir
;;
@@ -409,6 +477,10 @@ _bpftool()
_bpftool_get_map_ids
return 0
;;
+ name)
+ _bpftool_get_map_names
+ return 0
+ ;;
pinned|pinmaps)
_filedir
return 0
@@ -457,7 +529,7 @@ _bpftool()
esac
;;
map)
- local MAP_TYPE='id pinned'
+ local MAP_TYPE='id pinned name'
case $command in
show|list|dump|peek|pop|dequeue|freeze)
case $prev in
@@ -483,6 +555,24 @@ _bpftool()
esac
return 0
;;
+ name)
+ case "$command" in
+ peek)
+ _bpftool_get_map_names_for_type stack
+ _bpftool_get_map_names_for_type queue
+ ;;
+ pop)
+ _bpftool_get_map_names_for_type stack
+ ;;
+ dequeue)
+ _bpftool_get_map_names_for_type queue
+ ;;
+ *)
+ _bpftool_get_map_names
+ ;;
+ esac
+ return 0
+ ;;
*)
return 0
;;
@@ -530,6 +620,10 @@ _bpftool()
_bpftool_get_map_ids
return 0
;;
+ name)
+ _bpftool_get_map_names
+ return 0
+ ;;
key)
COMPREPLY+=( $( compgen -W 'hex' -- "$cur" ) )
;;
@@ -555,6 +649,10 @@ _bpftool()
_bpftool_map_update_get_id $command
return 0
;;
+ name)
+ _bpftool_map_update_get_name $command
+ return 0
+ ;;
key)
COMPREPLY+=( $( compgen -W 'hex' -- "$cur" ) )
;;
@@ -563,7 +661,7 @@ _bpftool()
# map, depending on the type of the map to update.
case "$(_bpftool_map_guess_map_type)" in
array_of_maps|hash_of_maps)
- local MAP_TYPE='id pinned'
+ local MAP_TYPE='id pinned name'
COMPREPLY+=( $( compgen -W "$MAP_TYPE" \
-- "$cur" ) )
return 0
@@ -631,6 +729,10 @@ _bpftool()
_bpftool_get_map_ids_for_type perf_event_array
return 0
;;
+ name)
+ _bpftool_get_map_names_for_type perf_event_array
+ return 0
+ ;;
cpu)
return 0
;;
@@ -655,7 +757,7 @@ _bpftool()
;;
btf)
local PROG_TYPE='id pinned tag name'
- local MAP_TYPE='id pinned'
+ local MAP_TYPE='id pinned name'
case $command in
dump)
case $prev in
@@ -686,6 +788,17 @@ _bpftool()
esac
return 0
;;
+ name)
+ case $pprev in
+ prog)
+ _bpftool_get_prog_names
+ ;;
+ map)
+ _bpftool_get_map_names
+ ;;
+ esac
+ return 0
+ ;;
format)
COMPREPLY=( $( compgen -W "c raw" -- "$cur" ) )
;;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index a7ead7bb9447..81890f4c8cca 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -47,7 +47,7 @@
"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n" \
"\t {-m|--mapcompat} | {-n|--nomount} }"
#define HELP_SPEC_MAP \
- "MAP := { id MAP_ID | pinned FILE }"
+ "MAP := { id MAP_ID | pinned FILE | name MAP_NAME }"
static const char * const prog_type_name[] = {
[BPF_PROG_TYPE_UNSPEC] = "unspec",
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index de61d73b9030..f0e0be08ba21 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -91,10 +91,68 @@ static void *alloc_value(struct bpf_map_info *info)
return malloc(info->value_size);
}
-int map_parse_fd(int *argc, char ***argv)
+static int
+map_fd_by_name(char *name, int *fds)
{
- int fd;
+ unsigned int id = 0;
+ int fd, nb_fds = 0;
+ void *tmp;
+ int err;
+
+ while (true) {
+ struct bpf_map_info info = {};
+ __u32 len = sizeof(info);
+
+ err = bpf_map_get_next_id(id, &id);
+ if (err) {
+ if (errno != ENOENT) {
+ p_err("%s", strerror(errno));
+ goto err_close_fds;
+ }
+ return nb_fds;
+ }
+
+ fd = bpf_map_get_fd_by_id(id);
+ if (fd < 0) {
+ p_err("can't get map by id (%u): %s",
+ id, strerror(errno));
+ goto err_close_fds;
+ }
+
+ err = bpf_obj_get_info_by_fd(fd, &info, &len);
+ if (err) {
+ p_err("can't get map info (%u): %s",
+ id, strerror(errno));
+ goto err_close_fd;
+ }
+
+ if (strncmp(name, info.name, BPF_OBJ_NAME_LEN)) {
+ close(fd);
+ continue;
+ }
+
+ if (nb_fds > 0) {
+ tmp = realloc(fds, (nb_fds + 1) * sizeof(int));
+ if (!tmp) {
+ p_err("failed to realloc");
+ goto err_close_fd;
+ }
+ fds = tmp;
+ }
+ fds[nb_fds++] = fd;
+ }
+err_close_fd:
+ close(fd);
+err_close_fds:
+ for (nb_fds--; nb_fds >= 0; nb_fds--)
+ close(fds[nb_fds]);
+ return -1;
+}
+
+static int
+map_parse_fds(int *argc, char ***argv, int *fds)
+{
if (is_prefix(**argv, "id")) {
unsigned int id;
char *endptr;
@@ -108,10 +166,25 @@ int map_parse_fd(int *argc, char ***argv)
}
NEXT_ARGP();
- fd = bpf_map_get_fd_by_id(id);
- if (fd < 0)
+ fds[0] = bpf_map_get_fd_by_id(id);
+ if (fds[0] < 0) {
p_err("get map by id (%u): %s", id, strerror(errno));
- return fd;
+ return -1;
+ }
+ return 1;
+ } else if (is_prefix(**argv, "name")) {
+ char *name;
+
+ NEXT_ARGP();
+
+ name = **argv;
+ if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
+ p_err("can't parse name");
+ return -1;
+ }
+ NEXT_ARGP();
+
+ return map_fd_by_name(name, fds);
} else if (is_prefix(**argv, "pinned")) {
char *path;
@@ -120,13 +193,43 @@ int map_parse_fd(int *argc, char ***argv)
path = **argv;
NEXT_ARGP();
- return open_obj_pinned_any(path, BPF_OBJ_MAP);
+ fds[0] = open_obj_pinned_any(path, BPF_OBJ_MAP);
+ if (fds[0] < 0)
+ return -1;
+ return 1;
}
- p_err("expected 'id' or 'pinned', got: '%s'?", **argv);
+ p_err("expected 'id', 'name' or 'pinned', got: '%s'?", **argv);
return -1;
}
+int map_parse_fd(int *argc, char ***argv)
+{
+ int *fds = NULL;
+ int nb_fds, fd;
+
+ fds = malloc(sizeof(int));
+ if (!fds) {
+ p_err("mem alloc failed");
+ return -1;
+ }
+ nb_fds = map_parse_fds(argc, argv, fds);
+ if (nb_fds != 1) {
+ if (nb_fds > 1) {
+ p_err("several maps match this handle");
+ for (nb_fds--; nb_fds >= 0; nb_fds--)
+ close(fds[nb_fds]);
+ }
+ fd = -1;
+ goto err_free;
+ }
+
+ fd = fds[0];
+err_free:
+ free(fds);
+ return fd;
+}
+
int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len)
{
int err;
@@ -479,6 +582,22 @@ static int parse_elem(char **argv, struct bpf_map_info *info,
return -1;
}
+static void
+show_map_header_json(struct bpf_map_info *info, json_writer_t *wtr)
+{
+ jsonw_uint_field(wtr, "id", info->id);
+ if (info->type < ARRAY_SIZE(map_type_name))
+ jsonw_string_field(wtr, "type", map_type_name[info->type]);
+ else
+ jsonw_uint_field(wtr, "type", info->type);
+
+ if (*info->name)
+ jsonw_string_field(wtr, "name", info->name);
+
+ jsonw_name(wtr, "flags");
+ jsonw_printf(wtr, "%d", info->map_flags);
+}
+
static int show_map_close_json(int fd, struct bpf_map_info *info)
{
char *memlock, *frozen_str;
@@ -489,18 +608,7 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
jsonw_start_object(json_wtr);
- jsonw_uint_field(json_wtr, "id", info->id);
- if (info->type < ARRAY_SIZE(map_type_name))
- jsonw_string_field(json_wtr, "type",
- map_type_name[info->type]);
- else
- jsonw_uint_field(json_wtr, "type", info->type);
-
- if (*info->name)
- jsonw_string_field(json_wtr, "name", info->name);
-
- jsonw_name(json_wtr, "flags");
- jsonw_printf(json_wtr, "%d", info->map_flags);
+ show_map_header_json(info, json_wtr);
print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
@@ -561,14 +669,9 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
return 0;
}
-static int show_map_close_plain(int fd, struct bpf_map_info *info)
+static void
+show_map_header_plain(struct bpf_map_info *info)
{
- char *memlock, *frozen_str;
- int frozen = 0;
-
- memlock = get_fdinfo(fd, "memlock");
- frozen_str = get_fdinfo(fd, "frozen");
-
printf("%u: ", info->id);
if (info->type < ARRAY_SIZE(map_type_name))
printf("%s ", map_type_name[info->type]);
@@ -581,6 +684,18 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
printf("flags 0x%x", info->map_flags);
print_dev_plain(info->ifindex, info->netns_dev, info->netns_ino);
printf("\n");
+}
+
+static int
+show_map_close_plain(int fd, struct bpf_map_info *info)
+{
+ char *memlock, *frozen_str;
+ int frozen = 0;
+
+ memlock = get_fdinfo(fd, "memlock");
+ frozen_str = get_fdinfo(fd, "frozen");
+
+ show_map_header_plain(info);
printf("\tkey %uB value %uB max_entries %u",
info->key_size, info->value_size, info->max_entries);
@@ -646,6 +761,8 @@ static int do_show(int argc, char **argv)
{
struct bpf_map_info info = {};
__u32 len = sizeof(info);
+ int *fds = NULL;
+ int nb_fds, i;
__u32 id = 0;
int err;
int fd;
@@ -654,14 +771,42 @@ static int do_show(int argc, char **argv)
build_pinned_obj_table(&map_table, BPF_OBJ_MAP);
if (argc == 2) {
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
- if (fd < 0)
+ fds = malloc(sizeof(int));
+ if (!fds) {
+ p_err("mem alloc failed");
return -1;
+ }
+ nb_fds = map_parse_fds(&argc, &argv, fds);
+ if (nb_fds < 1)
+ goto err_free;
+
+ if (json_output && nb_fds > 1)
+ jsonw_start_array(json_wtr); /* root array */
+ for (i = 0; i < nb_fds; i++) {
+ err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
+ if (err) {
+ p_err("can't get map info: %s",
+ strerror(errno));
+ for (; i < nb_fds; i++)
+ close(fds[i]);
+ goto err_free;
+ }
- if (json_output)
- return show_map_close_json(fd, &info);
- else
- return show_map_close_plain(fd, &info);
+ if (json_output)
+ show_map_close_json(fds[i], &info);
+ else
+ show_map_close_plain(fds[i], &info);
+
+ close(fds[i]);
+ }
+ if (json_output && nb_fds > 1)
+ jsonw_end_array(json_wtr); /* root array */
+
+ return 0;
+
+err_free:
+ free(fds);
+ return -1;
}
if (argc)
@@ -765,23 +910,55 @@ static int dump_map_elem(int fd, void *key, void *value,
return 0;
}
-static int do_dump(int argc, char **argv)
+static int
+maps_have_btf(int *fds, int nb_fds)
+{
+ struct bpf_map_info info = {};
+ __u32 len = sizeof(info);
+ struct btf *btf = NULL;
+ int err, i;
+
+ for (i = 0; i < nb_fds; i++) {
+ err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
+ if (err) {
+ p_err("can't get map info: %s", strerror(errno));
+ goto err_close;
+ }
+
+ err = btf__get_from_id(info.btf_id, &btf);
+ if (err) {
+ p_err("failed to get btf");
+ goto err_close;
+ }
+
+ if (!btf)
+ return 0;
+ }
+
+ return 1;
+
+err_close:
+ for (; i < nb_fds; i++)
+ close(fds[i]);
+ return -1;
+}
+
+static int
+map_dump(int fd, json_writer_t *wtr, bool enable_btf, bool show_header)
{
struct bpf_map_info info = {};
void *key, *value, *prev_key;
unsigned int num_elems = 0;
__u32 len = sizeof(info);
- json_writer_t *btf_wtr;
struct btf *btf = NULL;
int err;
- int fd;
- if (argc != 2)
- usage();
-
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
- if (fd < 0)
- return -1;
+ err = bpf_obj_get_info_by_fd(fd, &info, &len);
+ if (err) {
+ p_err("can't get map info: %s", strerror(errno));
+ close(fd);
+ return err;
+ }
key = malloc(info.key_size);
value = alloc_value(&info);
@@ -793,25 +970,27 @@ static int do_dump(int argc, char **argv)
prev_key = NULL;
- err = btf__get_from_id(info.btf_id, &btf);
- if (err) {
- p_err("failed to get btf");
- goto exit_free;
+ if (enable_btf) {
+ err = btf__get_from_id(info.btf_id, &btf);
+ if (err || !btf) {
+ /* enable_btf is true only if we've already checked
+ * that all maps have BTF information.
+ */
+ p_err("failed to get btf");
+ goto exit_free;
+ }
}
- if (json_output)
- jsonw_start_array(json_wtr);
- else
- if (btf) {
- btf_wtr = get_btf_writer();
- if (!btf_wtr) {
- p_info("failed to create json writer for btf. falling back to plain output");
- btf__free(btf);
- btf = NULL;
- } else {
- jsonw_start_array(btf_wtr);
- }
+ if (wtr) {
+ if (show_header) {
+ jsonw_start_object(wtr); /* map object */
+ show_map_header_json(&info, wtr);
+ jsonw_name(wtr, "elements");
}
+ jsonw_start_array(wtr); /* elements */
+ } else if (show_header) {
+ show_map_header_plain(&info);
+ }
if (info.type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
info.value_size != 8)
@@ -824,15 +1003,14 @@ static int do_dump(int argc, char **argv)
err = 0;
break;
}
- num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
+ num_elems += dump_map_elem(fd, key, value, &info, btf, wtr);
prev_key = key;
}
- if (json_output)
- jsonw_end_array(json_wtr);
- else if (btf) {
- jsonw_end_array(btf_wtr);
- jsonw_destroy(&btf_wtr);
+ if (wtr) {
+ jsonw_end_array(wtr); /* elements */
+ if (show_header)
+ jsonw_end_object(wtr); /* map object */
} else {
printf("Found %u element%s\n", num_elems,
num_elems != 1 ? "s" : "");
@@ -847,6 +1025,68 @@ static int do_dump(int argc, char **argv)
return err;
}
+static int
+do_dump(int argc, char **argv)
+{
+ json_writer_t *wtr = NULL, *btf_wtr = NULL;
+ int nb_fds, i, btf = 0;
+ int *fds = NULL;
+
+ if (argc != 2)
+ usage();
+
+ fds = malloc(sizeof(int));
+ if (!fds) {
+ p_err("mem alloc failed");
+ return -1;
+ }
+ nb_fds = map_parse_fds(&argc, &argv, fds);
+ if (nb_fds < 1)
+ goto err_free;
+
+ if (json_output) {
+ wtr = json_wtr;
+ } else {
+ btf = maps_have_btf(fds, nb_fds);
+ if (btf < 0)
+ goto err_free;
+ if (btf) {
+ btf_wtr = get_btf_writer();
+ if (btf_wtr) {
+ wtr = btf_wtr;
+ } else {
+ p_info("failed to create json writer for btf. falling back to plain output");
+ btf = 0;
+ }
+ }
+ }
+
+ if (wtr && nb_fds > 1)
+ jsonw_start_array(wtr); /* root array */
+ for (i = 0; i < nb_fds; i++) {
+ if (map_dump(fds[i], wtr, btf, nb_fds > 1)) {
+ for (; i < nb_fds; i++)
+ close(fds[i]);
+ goto err_destroy;
+ }
+ if (!wtr && i != nb_fds - 1)
+ printf("\n");
+ }
+ if (wtr && nb_fds > 1)
+ jsonw_end_array(wtr); /* root array */
+
+ if (btf)
+ jsonw_destroy(&btf_wtr);
+ return 0;
+
+err_destroy:
+ if (btf)
+ jsonw_destroy(&btf_wtr);
+err_free:
+ free(fds);
+ return -1;
+}
+
static int alloc_key_value(struct bpf_map_info *info, void **key, void **value)
{
*key = NULL;
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH bpf-next 3/3] bpftool: match maps by name
2019-12-10 16:06 ` [PATCH bpf-next 3/3] bpftool: match maps " Paul Chaignon
@ 2019-12-10 17:29 ` Quentin Monnet
0 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-12-10 17:29 UTC (permalink / raw)
To: Paul Chaignon, bpf
Cc: paul.chaignon, netdev, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko
2019-12-10 17:06 UTC+0100 ~ Paul Chaignon <paul.chaignon@orange.com>
> This patch implements lookup by name for maps and changes the behavior of
> lookups by tag to be consistent with prog subcommands. Similarly to
> program subcommands, the show and dump commands will return all maps with
> the given name (or tag), whereas other commands will error out if several
> maps have the same name (resp. tag).
>
> When a map has BTF info, it is dumped in JSON with available BTF info.
> This patch requires that all matched maps have BTF info before switching
> the output format to JSON.
>
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> ---
> .../bpf/bpftool/Documentation/bpftool-map.rst | 10 +-
> tools/bpf/bpftool/bash-completion/bpftool | 131 ++++++-
> tools/bpf/bpftool/main.h | 2 +-
> tools/bpf/bpftool/map.c | 366 +++++++++++++++---
> 4 files changed, 432 insertions(+), 77 deletions(-)
>
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 05b5be4a6ef9..21c676a1eeb1 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
Nice work on the completion, thanks!
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index de61d73b9030..f0e0be08ba21 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
[...]
> @@ -654,14 +771,42 @@ static int do_show(int argc, char **argv)
> build_pinned_obj_table(&map_table, BPF_OBJ_MAP);
>
> if (argc == 2) {
> - fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
> - if (fd < 0)
> + fds = malloc(sizeof(int));
> + if (!fds) {
> + p_err("mem alloc failed");
> return -1;
> + }
> + nb_fds = map_parse_fds(&argc, &argv, fds);
> + if (nb_fds < 1)
> + goto err_free;
> +
> + if (json_output && nb_fds > 1)
> + jsonw_start_array(json_wtr); /* root array */
> + for (i = 0; i < nb_fds; i++) {
> + err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> + if (err) {
> + p_err("can't get map info: %s",
> + strerror(errno));
> + for (; i < nb_fds; i++)
> + close(fds[i]);
> + goto err_free;
Same remarks as on patch 1, we may want to keep listing the maps even if
we get a failure for one of them?
> + }
>
> - if (json_output)
> - return show_map_close_json(fd, &info);
> - else
> - return show_map_close_plain(fd, &info);
> + if (json_output)
> + show_map_close_json(fds[i], &info);
> + else
> + show_map_close_plain(fds[i], &info);
> +
> + close(fds[i]);
> + }
> + if (json_output && nb_fds > 1)
> + jsonw_end_array(json_wtr); /* root array */
> +
> + return 0;
> +
> +err_free:
> + free(fds);
> + return -1;
> }
>
> if (argc)
The rest of the code looks good to me, thanks a lot for working on this!
Quentin
^ permalink raw reply [flat|nested] 13+ messages in thread