* [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
@ 2019-07-04 8:56 Quentin Monnet
2019-07-05 5:49 ` Y Song
0 siblings, 1 reply; 7+ messages in thread
From: Quentin Monnet @ 2019-07-04 8:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, oss-drivers, Quentin Monnet
Add a new "bpftool prog run" subcommand to run a loaded program on input
data (and possibly with input context) passed by the user.
Print output data (and output context if relevant) into a file or into
the console. Print return value and duration for the test run into the
console.
A "repeat" argument can be passed to run the program several times in a
row.
The command does not perform any kind of verification based on program
type (Is this program type allowed to use an input context?) or on data
consistency (Can I work with empty input data?), this is left to the
kernel.
Example invocation:
# perl -e 'print "\x0" x 14' | ./bpftool prog run \
pinned /sys/fs/bpf/sample_ret0 \
data_in - data_out - repeat 5
0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
Return value: 0, duration (average): 260ns
When one of data_in or ctx_in is "-", bpftool reads from standard input,
in binary format. Other formats (JSON, hexdump) might be supported (via
an optional command line keyword like "data_fmt_in") in the future if
relevant, but this would require doing more parsing in bpftool.
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
.../bpftool/Documentation/bpftool-prog.rst | 34 ++
tools/bpf/bpftool/bash-completion/bpftool | 28 +-
tools/bpf/bpftool/main.c | 29 ++
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 348 +++++++++++++++++-
tools/include/linux/sizes.h | 48 +++
6 files changed, 485 insertions(+), 3 deletions(-)
create mode 100644 tools/include/linux/sizes.h
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 1df637f85f94..7a374b3c851d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -29,6 +29,7 @@ PROG COMMANDS
| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog tracelog**
+| **bpftool** **prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -146,6 +147,39 @@ DESCRIPTION
streaming data from BPF programs to user space, one can use
perf events (see also **bpftool-map**\ (8)).
+ **bpftool prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
+ Run BPF program *PROG* in the kernel testing infrastructure
+ for BPF, meaning that the program works on the data and
+ context provided by the user, and not on actual packets or
+ monitored functions etc. Return value and duration for the
+ test run are printed out to the console.
+
+ Input data is read from the *FILE* passed with **data_in**.
+ If this *FILE* is "**-**", input data is read from standard
+ input. Input context, if any, is read from *FILE* passed with
+ **ctx_in**. Again, "**-**" can be used to read from standard
+ input, but only if standard input is not already in use for
+ input data. If a *FILE* is passed with **data_out**, output
+ data is written to that file. Similarly, output context is
+ written to the *FILE* passed with **ctx_out**. For both
+ output flows, "**-**" can be used to print to the standard
+ output (as plain text, or JSON if relevant option was
+ passed). If output keywords are omitted, output data and
+ context are discarded. Keywords **data_size_out** and
+ **ctx_size_out** are used to pass the size (in bytes) for the
+ output buffers to the kernel, although the default of 32 kB
+ should be more than enough for most cases.
+
+ Keyword **repeat** is used to indicate the number of
+ consecutive runs to perform. Note that output data and
+ context printed to files correspond to the last of those
+ runs. The duration printed out at the end of the runs is an
+ average over all runs performed by the command.
+
+ Not all program types support test run. Among those which do,
+ not all of them can take the **ctx_in**/**ctx_out**
+ arguments. bpftool does not perform checks on program types.
+
**bpftool prog help**
Print short help message.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ba37095e1f62..965a8658cca3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -408,10 +408,34 @@ _bpftool()
tracelog)
return 0
;;
+ run)
+ if [[ ${#words[@]} -lt 5 ]]; then
+ _filedir
+ return 0
+ fi
+ case $prev in
+ id)
+ _bpftool_get_prog_ids
+ return 0
+ ;;
+ data_in|data_out|ctx_in|ctx_out)
+ _filedir
+ return 0
+ ;;
+ repeat|data_size_out|ctx_size_out)
+ return 0
+ ;;
+ *)
+ _bpftool_once_attr 'data_in data_out data_size_out \
+ ctx_in ctx_out ctx_size_out repeat'
+ return 0
+ ;;
+ esac
+ ;;
*)
[[ $prev == $object ]] && \
- COMPREPLY=( $( compgen -W 'dump help pin attach detach load \
- show list tracelog' -- "$cur" ) )
+ COMPREPLY=( $( compgen -W 'dump help pin attach detach \
+ load show list tracelog run' -- "$cur" ) )
;;
esac
;;
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 4879f6395c7e..e916ff25697f 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -117,6 +117,35 @@ bool is_prefix(const char *pfx, const char *str)
return !memcmp(str, pfx, strlen(pfx));
}
+/* Last argument MUST be NULL pointer */
+int detect_common_prefix(const char *arg, ...)
+{
+ unsigned int count = 0;
+ const char *ref;
+ char msg[256];
+ va_list ap;
+
+ snprintf(msg, sizeof(msg), "ambiguous prefix: '%s' could be '", arg);
+ va_start(ap, arg);
+ while ((ref = va_arg(ap, const char *))) {
+ if (!is_prefix(arg, ref))
+ continue;
+ count++;
+ if (count > 1)
+ strncat(msg, "' or '", sizeof(msg) - strlen(msg) - 1);
+ strncat(msg, ref, sizeof(msg) - strlen(msg) - 1);
+ }
+ va_end(ap);
+ strncat(msg, "'", sizeof(msg) - strlen(msg) - 1);
+
+ if (count >= 2) {
+ p_err(msg);
+ return -1;
+ }
+
+ return 0;
+}
+
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
{
unsigned char *data = arg;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 9c5d9c80f71e..3ef0d9051e10 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -101,6 +101,7 @@ void p_err(const char *fmt, ...);
void p_info(const char *fmt, ...);
bool is_prefix(const char *pfx, const char *str);
+int detect_common_prefix(const char *arg, ...);
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
void usage(void) __noreturn;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9b0db5d14e31..8dcbaa0a8ab1 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -15,6 +15,7 @@
#include <sys/stat.h>
#include <linux/err.h>
+#include <linux/sizes.h>
#include <bpf.h>
#include <btf.h>
@@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
return 0;
}
+static int check_single_stdin(char *file_in, char *other_file_in)
+{
+ if (file_in && other_file_in &&
+ !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
+ p_err("cannot use standard input for both data_in and ctx_in");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int get_run_data(const char *fname, void **data_ptr, unsigned int *size)
+{
+ size_t block_size = 256;
+ size_t buf_size = block_size;
+ size_t nb_read = 0;
+ void *tmp;
+ FILE *f;
+
+ if (!fname) {
+ *data_ptr = NULL;
+ *size = 0;
+ return 0;
+ }
+
+ if (!strcmp(fname, "-"))
+ f = stdin;
+ else
+ f = fopen(fname, "r");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ *data_ptr = malloc(block_size);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_fclose;
+ }
+
+ while ((nb_read += fread(*data_ptr + nb_read, 1, block_size, f))) {
+ if (feof(f))
+ break;
+ if (ferror(f)) {
+ p_err("failed to read data_in/ctx_in from %s: %s",
+ fname, strerror(errno));
+ goto err_free;
+ }
+ if (nb_read > buf_size - block_size) {
+ if (buf_size == UINT32_MAX) {
+ p_err("data_in/ctx_in is too long (max: %d)",
+ UINT32_MAX);
+ goto err_free;
+ }
+ /* No space for fread()-ing next chunk; realloc() */
+ buf_size *= 2;
+ tmp = realloc(*data_ptr, buf_size);
+ if (!tmp) {
+ p_err("failed to reallocate data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_free;
+ }
+ *data_ptr = tmp;
+ }
+ }
+ if (f != stdin)
+ fclose(f);
+
+ *size = nb_read;
+ return 0;
+
+err_free:
+ free(*data_ptr);
+ *data_ptr = NULL;
+err_fclose:
+ if (f != stdin)
+ fclose(f);
+ return -1;
+}
+
+static void hex_print(void *data, unsigned int size, FILE *f)
+{
+ size_t i, j;
+ char c;
+
+ for (i = 0; i < size; i += 16) {
+ /* Row offset */
+ fprintf(f, "%07zx\t", i);
+
+ /* Hexadecimal values */
+ for (j = i; j < i + 16 && j < size; j++)
+ fprintf(f, "%02x%s", *(uint8_t *)(data + j),
+ j % 2 ? " " : "");
+ for (; j < i + 16; j++)
+ fprintf(f, " %s", j % 2 ? " " : "");
+
+ /* ASCII values (if relevant), '.' otherwise */
+ fprintf(f, "| ");
+ for (j = i; j < i + 16 && j < size; j++) {
+ c = *(char *)(data + j);
+ if (c < ' ' || c > '~')
+ c = '.';
+ fprintf(f, "%c%s", c, j == i + 7 ? " " : "");
+ }
+
+ fprintf(f, "\n");
+ }
+}
+
+static int
+print_run_output(void *data, unsigned int size, const char *fname,
+ const char *json_key)
+{
+ size_t nb_written;
+ FILE *f;
+
+ if (!fname)
+ return 0;
+
+ if (!strcmp(fname, "-")) {
+ f = stdout;
+ if (json_output) {
+ jsonw_name(json_wtr, json_key);
+ print_data_json(data, size);
+ } else {
+ hex_print(data, size, f);
+ }
+ return 0;
+ }
+
+ f = fopen(fname, "w");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ nb_written = fwrite(data, 1, size, f);
+ fclose(f);
+ if (nb_written != size) {
+ p_err("failed to write output data/ctx: %s", strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int alloc_run_data(void **data_ptr, unsigned int size_out)
+{
+ *data_ptr = calloc(size_out, 1);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for output data/ctx: %s",
+ strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int do_run(int argc, char **argv)
+{
+ char *data_fname_in = NULL, *data_fname_out = NULL;
+ char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
+ struct bpf_prog_test_run_attr test_attr = {0};
+ const unsigned int default_size = SZ_32K;
+ void *data_in = NULL, *data_out = NULL;
+ void *ctx_in = NULL, *ctx_out = NULL;
+ unsigned int repeat = 1;
+ int fd, err;
+
+ if (!REQ_ARGS(4))
+ return -1;
+
+ fd = prog_parse_fd(&argc, &argv);
+ if (fd < 0)
+ return -1;
+
+ while (argc) {
+ if (detect_common_prefix(*argv, "data_in", "data_out",
+ "data_size_out", NULL))
+ return -1;
+ if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
+ "ctx_size_out", NULL))
+ return -1;
+
+ if (is_prefix(*argv, "data_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_in = GET_ARG();
+ if (check_single_stdin(data_fname_in, ctx_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "data_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "data_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.data_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output data size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "ctx_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_in = GET_ARG();
+ if (check_single_stdin(ctx_fname_in, data_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "ctx_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "ctx_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.ctx_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output context size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "repeat")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ repeat = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as repeat number",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'data_in', 'data_out', 'data_size_out', 'ctx_in', 'ctx_out', 'ctx_size_out' or 'repeat', got: '%s'?",
+ *argv);
+ return -1;
+ }
+ }
+
+ err = get_run_data(data_fname_in, &data_in, &test_attr.data_size_in);
+ if (err)
+ return -1;
+
+ if (data_in) {
+ if (!test_attr.data_size_out)
+ test_attr.data_size_out = default_size;
+ err = alloc_run_data(&data_out, test_attr.data_size_out);
+ if (err)
+ goto free_data_in;
+ }
+
+ err = get_run_data(ctx_fname_in, &ctx_in, &test_attr.ctx_size_in);
+ if (err)
+ goto free_data_out;
+
+ if (ctx_in) {
+ if (!test_attr.ctx_size_out)
+ test_attr.ctx_size_out = default_size;
+ err = alloc_run_data(&ctx_out, test_attr.ctx_size_out);
+ if (err)
+ goto free_ctx_in;
+ }
+
+ test_attr.prog_fd = fd;
+ test_attr.repeat = repeat;
+ test_attr.data_in = data_in;
+ test_attr.data_out = data_out;
+ test_attr.ctx_in = ctx_in;
+ test_attr.ctx_out = ctx_out;
+
+ err = bpf_prog_test_run_xattr(&test_attr);
+ if (err) {
+ p_err("failed to run program: %s", strerror(errno));
+ goto free_ctx_out;
+ }
+
+ err = 0;
+
+ if (json_output)
+ jsonw_start_object(json_wtr); /* root */
+
+ /* Do not exit on errors occurring when printing output data/context,
+ * we still want to print return value and duration for program run.
+ */
+ if (test_attr.data_size_out)
+ err += print_run_output(test_attr.data_out,
+ test_attr.data_size_out,
+ data_fname_out, "data_out");
+ if (test_attr.ctx_size_out)
+ err += print_run_output(test_attr.ctx_out,
+ test_attr.ctx_size_out,
+ ctx_fname_out, "ctx_out");
+
+ if (json_output) {
+ jsonw_uint_field(json_wtr, "retval", test_attr.retval);
+ jsonw_uint_field(json_wtr, "duration", test_attr.duration);
+ jsonw_end_object(json_wtr); /* root */
+ } else {
+ fprintf(stdout, "Return value: %u, duration%s: %uns\n",
+ test_attr.retval,
+ repeat > 1 ? " (average)" : "", test_attr.duration);
+ }
+
+free_ctx_out:
+ free(ctx_out);
+free_ctx_in:
+ free(ctx_in);
+free_data_out:
+ free(data_out);
+free_data_in:
+ free(data_in);
+
+ return err;
+}
+
static int load_with_options(int argc, char **argv, bool first_prog_only)
{
struct bpf_object_load_attr load_attr = { 0 };
@@ -1058,6 +1397,11 @@ static int do_help(int argc, char **argv)
" [pinmaps MAP_DIR]\n"
" %s %s attach PROG ATTACH_TYPE [MAP]\n"
" %s %s detach PROG ATTACH_TYPE [MAP]\n"
+ " %s %s run PROG \\\n"
+ " data_in FILE \\\n"
+ " [data_out FILE [data_size_out L]] \\\n"
+ " [ctx_in FILE [ctx_out FILE [ctx_size_out M]]] \\\n"
+ " [repeat N]\n"
" %s %s tracelog\n"
" %s %s help\n"
"\n"
@@ -1079,7 +1423,8 @@ static int do_help(int argc, char **argv)
"",
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
- bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+ bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+ bin_name, argv[-2]);
return 0;
}
@@ -1095,6 +1440,7 @@ static const struct cmd cmds[] = {
{ "attach", do_attach },
{ "detach", do_detach },
{ "tracelog", do_tracelog },
+ { "run", do_run },
{ 0 }
};
diff --git a/tools/include/linux/sizes.h b/tools/include/linux/sizes.h
new file mode 100644
index 000000000000..1cbb4c4d016e
--- /dev/null
+++ b/tools/include/linux/sizes.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * include/linux/sizes.h
+ */
+#ifndef __LINUX_SIZES_H__
+#define __LINUX_SIZES_H__
+
+#include <linux/const.h>
+
+#define SZ_1 0x00000001
+#define SZ_2 0x00000002
+#define SZ_4 0x00000004
+#define SZ_8 0x00000008
+#define SZ_16 0x00000010
+#define SZ_32 0x00000020
+#define SZ_64 0x00000040
+#define SZ_128 0x00000080
+#define SZ_256 0x00000100
+#define SZ_512 0x00000200
+
+#define SZ_1K 0x00000400
+#define SZ_2K 0x00000800
+#define SZ_4K 0x00001000
+#define SZ_8K 0x00002000
+#define SZ_16K 0x00004000
+#define SZ_32K 0x00008000
+#define SZ_64K 0x00010000
+#define SZ_128K 0x00020000
+#define SZ_256K 0x00040000
+#define SZ_512K 0x00080000
+
+#define SZ_1M 0x00100000
+#define SZ_2M 0x00200000
+#define SZ_4M 0x00400000
+#define SZ_8M 0x00800000
+#define SZ_16M 0x01000000
+#define SZ_32M 0x02000000
+#define SZ_64M 0x04000000
+#define SZ_128M 0x08000000
+#define SZ_256M 0x10000000
+#define SZ_512M 0x20000000
+
+#define SZ_1G 0x40000000
+#define SZ_2G 0x80000000
+
+#define SZ_4G _AC(0x100000000, ULL)
+
+#endif /* __LINUX_SIZES_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
2019-07-04 8:56 [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs Quentin Monnet
@ 2019-07-05 5:49 ` Y Song
2019-07-05 8:21 ` Quentin Monnet
0 siblings, 1 reply; 7+ messages in thread
From: Y Song @ 2019-07-05 5:49 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> Add a new "bpftool prog run" subcommand to run a loaded program on input
> data (and possibly with input context) passed by the user.
>
> Print output data (and output context if relevant) into a file or into
> the console. Print return value and duration for the test run into the
> console.
>
> A "repeat" argument can be passed to run the program several times in a
> row.
>
> The command does not perform any kind of verification based on program
> type (Is this program type allowed to use an input context?) or on data
> consistency (Can I work with empty input data?), this is left to the
> kernel.
>
> Example invocation:
>
> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
> pinned /sys/fs/bpf/sample_ret0 \
> data_in - data_out - repeat 5
> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
> Return value: 0, duration (average): 260ns
>
> When one of data_in or ctx_in is "-", bpftool reads from standard input,
> in binary format. Other formats (JSON, hexdump) might be supported (via
> an optional command line keyword like "data_fmt_in") in the future if
> relevant, but this would require doing more parsing in bpftool.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> .../bpftool/Documentation/bpftool-prog.rst | 34 ++
> tools/bpf/bpftool/bash-completion/bpftool | 28 +-
> tools/bpf/bpftool/main.c | 29 ++
> tools/bpf/bpftool/main.h | 1 +
> tools/bpf/bpftool/prog.c | 348 +++++++++++++++++-
> tools/include/linux/sizes.h | 48 +++
> 6 files changed, 485 insertions(+), 3 deletions(-)
> create mode 100644 tools/include/linux/sizes.h
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index 1df637f85f94..7a374b3c851d 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -29,6 +29,7 @@ PROG COMMANDS
> | **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> | **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> | **bpftool** **prog tracelog**
> +| **bpftool** **prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
> | **bpftool** **prog help**
> |
> | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> @@ -146,6 +147,39 @@ DESCRIPTION
> streaming data from BPF programs to user space, one can use
> perf events (see also **bpftool-map**\ (8)).
>
> + **bpftool prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
> + Run BPF program *PROG* in the kernel testing infrastructure
> + for BPF, meaning that the program works on the data and
> + context provided by the user, and not on actual packets or
> + monitored functions etc. Return value and duration for the
> + test run are printed out to the console.
> +
> + Input data is read from the *FILE* passed with **data_in**.
> + If this *FILE* is "**-**", input data is read from standard
> + input. Input context, if any, is read from *FILE* passed with
> + **ctx_in**. Again, "**-**" can be used to read from standard
> + input, but only if standard input is not already in use for
> + input data. If a *FILE* is passed with **data_out**, output
> + data is written to that file. Similarly, output context is
> + written to the *FILE* passed with **ctx_out**. For both
> + output flows, "**-**" can be used to print to the standard
> + output (as plain text, or JSON if relevant option was
> + passed). If output keywords are omitted, output data and
> + context are discarded. Keywords **data_size_out** and
> + **ctx_size_out** are used to pass the size (in bytes) for the
> + output buffers to the kernel, although the default of 32 kB
> + should be more than enough for most cases.
> +
> + Keyword **repeat** is used to indicate the number of
> + consecutive runs to perform. Note that output data and
> + context printed to files correspond to the last of those
> + runs. The duration printed out at the end of the runs is an
> + average over all runs performed by the command.
> +
> + Not all program types support test run. Among those which do,
> + not all of them can take the **ctx_in**/**ctx_out**
> + arguments. bpftool does not perform checks on program types.
> +
> **bpftool prog help**
> Print short help message.
>
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index ba37095e1f62..965a8658cca3 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -408,10 +408,34 @@ _bpftool()
> tracelog)
> return 0
> ;;
> + run)
> + if [[ ${#words[@]} -lt 5 ]]; then
> + _filedir
> + return 0
> + fi
> + case $prev in
> + id)
> + _bpftool_get_prog_ids
> + return 0
> + ;;
> + data_in|data_out|ctx_in|ctx_out)
> + _filedir
> + return 0
> + ;;
> + repeat|data_size_out|ctx_size_out)
> + return 0
> + ;;
> + *)
> + _bpftool_once_attr 'data_in data_out data_size_out \
> + ctx_in ctx_out ctx_size_out repeat'
> + return 0
> + ;;
> + esac
> + ;;
> *)
> [[ $prev == $object ]] && \
> - COMPREPLY=( $( compgen -W 'dump help pin attach detach load \
> - show list tracelog' -- "$cur" ) )
> + COMPREPLY=( $( compgen -W 'dump help pin attach detach \
> + load show list tracelog run' -- "$cur" ) )
> ;;
> esac
> ;;
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 4879f6395c7e..e916ff25697f 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -117,6 +117,35 @@ bool is_prefix(const char *pfx, const char *str)
> return !memcmp(str, pfx, strlen(pfx));
> }
>
> +/* Last argument MUST be NULL pointer */
> +int detect_common_prefix(const char *arg, ...)
> +{
> + unsigned int count = 0;
> + const char *ref;
> + char msg[256];
> + va_list ap;
> +
> + snprintf(msg, sizeof(msg), "ambiguous prefix: '%s' could be '", arg);
> + va_start(ap, arg);
> + while ((ref = va_arg(ap, const char *))) {
> + if (!is_prefix(arg, ref))
> + continue;
> + count++;
> + if (count > 1)
> + strncat(msg, "' or '", sizeof(msg) - strlen(msg) - 1);
> + strncat(msg, ref, sizeof(msg) - strlen(msg) - 1);
> + }
> + va_end(ap);
> + strncat(msg, "'", sizeof(msg) - strlen(msg) - 1);
> +
> + if (count >= 2) {
> + p_err(msg);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
> {
> unsigned char *data = arg;
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 9c5d9c80f71e..3ef0d9051e10 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -101,6 +101,7 @@ void p_err(const char *fmt, ...);
> void p_info(const char *fmt, ...);
>
> bool is_prefix(const char *pfx, const char *str);
> +int detect_common_prefix(const char *arg, ...);
> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
> void usage(void) __noreturn;
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 9b0db5d14e31..8dcbaa0a8ab1 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -15,6 +15,7 @@
> #include <sys/stat.h>
>
> #include <linux/err.h>
> +#include <linux/sizes.h>
>
> #include <bpf.h>
> #include <btf.h>
> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
> return 0;
> }
>
> +static int check_single_stdin(char *file_in, char *other_file_in)
> +{
> + if (file_in && other_file_in &&
> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
> + p_err("cannot use standard input for both data_in and ctx_in");
The error message says data_in and ctx_in.
Maybe the input parameter should be file_data_in and file_ctx_in?
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int get_run_data(const char *fname, void **data_ptr, unsigned int *size)
> +{
> + size_t block_size = 256;
> + size_t buf_size = block_size;
> + size_t nb_read = 0;
> + void *tmp;
> + FILE *f;
> +
> + if (!fname) {
> + *data_ptr = NULL;
> + *size = 0;
> + return 0;
> + }
> +
> + if (!strcmp(fname, "-"))
> + f = stdin;
> + else
> + f = fopen(fname, "r");
> + if (!f) {
> + p_err("failed to open %s: %s", fname, strerror(errno));
> + return -1;
> + }
> +
> + *data_ptr = malloc(block_size);
> + if (!*data_ptr) {
> + p_err("failed to allocate memory for data_in/ctx_in: %s",
> + strerror(errno));
> + goto err_fclose;
> + }
> +
> + while ((nb_read += fread(*data_ptr + nb_read, 1, block_size, f))) {
> + if (feof(f))
> + break;
> + if (ferror(f)) {
> + p_err("failed to read data_in/ctx_in from %s: %s",
> + fname, strerror(errno));
> + goto err_free;
> + }
> + if (nb_read > buf_size - block_size) {
> + if (buf_size == UINT32_MAX) {
> + p_err("data_in/ctx_in is too long (max: %d)",
> + UINT32_MAX);
> + goto err_free;
> + }
> + /* No space for fread()-ing next chunk; realloc() */
> + buf_size *= 2;
> + tmp = realloc(*data_ptr, buf_size);
> + if (!tmp) {
> + p_err("failed to reallocate data_in/ctx_in: %s",
> + strerror(errno));
> + goto err_free;
> + }
> + *data_ptr = tmp;
> + }
> + }
> + if (f != stdin)
> + fclose(f);
> +
> + *size = nb_read;
> + return 0;
> +
> +err_free:
> + free(*data_ptr);
> + *data_ptr = NULL;
> +err_fclose:
> + if (f != stdin)
> + fclose(f);
> + return -1;
> +}
> +
> +static void hex_print(void *data, unsigned int size, FILE *f)
> +{
> + size_t i, j;
> + char c;
> +
> + for (i = 0; i < size; i += 16) {
> + /* Row offset */
> + fprintf(f, "%07zx\t", i);
> +
> + /* Hexadecimal values */
> + for (j = i; j < i + 16 && j < size; j++)
> + fprintf(f, "%02x%s", *(uint8_t *)(data + j),
> + j % 2 ? " " : "");
> + for (; j < i + 16; j++)
> + fprintf(f, " %s", j % 2 ? " " : "");
> +
> + /* ASCII values (if relevant), '.' otherwise */
> + fprintf(f, "| ");
> + for (j = i; j < i + 16 && j < size; j++) {
> + c = *(char *)(data + j);
> + if (c < ' ' || c > '~')
> + c = '.';
> + fprintf(f, "%c%s", c, j == i + 7 ? " " : "");
> + }
> +
> + fprintf(f, "\n");
> + }
> +}
> +
> +static int
> +print_run_output(void *data, unsigned int size, const char *fname,
> + const char *json_key)
> +{
> + size_t nb_written;
> + FILE *f;
> +
> + if (!fname)
> + return 0;
> +
> + if (!strcmp(fname, "-")) {
> + f = stdout;
> + if (json_output) {
> + jsonw_name(json_wtr, json_key);
> + print_data_json(data, size);
> + } else {
> + hex_print(data, size, f);
> + }
> + return 0;
> + }
> +
> + f = fopen(fname, "w");
> + if (!f) {
> + p_err("failed to open %s: %s", fname, strerror(errno));
> + return -1;
> + }
> +
> + nb_written = fwrite(data, 1, size, f);
> + fclose(f);
> + if (nb_written != size) {
> + p_err("failed to write output data/ctx: %s", strerror(errno));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int alloc_run_data(void **data_ptr, unsigned int size_out)
> +{
> + *data_ptr = calloc(size_out, 1);
> + if (!*data_ptr) {
> + p_err("failed to allocate memory for output data/ctx: %s",
> + strerror(errno));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int do_run(int argc, char **argv)
> +{
> + char *data_fname_in = NULL, *data_fname_out = NULL;
> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
> + struct bpf_prog_test_run_attr test_attr = {0};
> + const unsigned int default_size = SZ_32K;
> + void *data_in = NULL, *data_out = NULL;
> + void *ctx_in = NULL, *ctx_out = NULL;
> + unsigned int repeat = 1;
> + int fd, err;
> +
> + if (!REQ_ARGS(4))
> + return -1;
> +
> + fd = prog_parse_fd(&argc, &argv);
> + if (fd < 0)
> + return -1;
> +
> + while (argc) {
> + if (detect_common_prefix(*argv, "data_in", "data_out",
> + "data_size_out", NULL))
> + return -1;
> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
> + "ctx_size_out", NULL))
> + return -1;
> +
> + if (is_prefix(*argv, "data_in")) {
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + data_fname_in = GET_ARG();
> + if (check_single_stdin(data_fname_in, ctx_fname_in))
> + return -1;
> + } else if (is_prefix(*argv, "data_out")) {
Here, we all use is_prefix() to match "data_in", "data_out",
"data_size_out" etc.
That means users can use "data_i" instead of "data_in" as below
... | ./bpftool prog run id 283 data_i - data_out - repeat 5
is this expected?
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + data_fname_out = GET_ARG();
> + } else if (is_prefix(*argv, "data_size_out")) {
> + char *endptr;
> +
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + test_attr.data_size_out = strtoul(*argv, &endptr, 0);
> + if (*endptr) {
> + p_err("can't parse %s as output data size",
> + *argv);
> + return -1;
> + }
> + NEXT_ARG();
> + } else if (is_prefix(*argv, "ctx_in")) {
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + ctx_fname_in = GET_ARG();
> + if (check_single_stdin(ctx_fname_in, data_fname_in))
> + return -1;
> + } else if (is_prefix(*argv, "ctx_out")) {
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + ctx_fname_out = GET_ARG();
> + } else if (is_prefix(*argv, "ctx_size_out")) {
> + char *endptr;
> +
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + test_attr.ctx_size_out = strtoul(*argv, &endptr, 0);
> + if (*endptr) {
> + p_err("can't parse %s as output context size",
> + *argv);
> + return -1;
> + }
> + NEXT_ARG();
> + } else if (is_prefix(*argv, "repeat")) {
> + char *endptr;
> +
> + NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> +
> + repeat = strtoul(*argv, &endptr, 0);
> + if (*endptr) {
> + p_err("can't parse %s as repeat number",
> + *argv);
> + return -1;
> + }
> + NEXT_ARG();
> + } else {
> + p_err("expected no more arguments, 'data_in', 'data_out', 'data_size_out', 'ctx_in', 'ctx_out', 'ctx_size_out' or 'repeat', got: '%s'?",
> + *argv);
> + return -1;
> + }
> + }
> +
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
2019-07-05 5:49 ` Y Song
@ 2019-07-05 8:21 ` Quentin Monnet
2019-07-05 15:42 ` Y Song
0 siblings, 1 reply; 7+ messages in thread
From: Quentin Monnet @ 2019-07-05 8:21 UTC (permalink / raw)
To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
> On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
>>
>> Add a new "bpftool prog run" subcommand to run a loaded program on input
>> data (and possibly with input context) passed by the user.
>>
>> Print output data (and output context if relevant) into a file or into
>> the console. Print return value and duration for the test run into the
>> console.
>>
>> A "repeat" argument can be passed to run the program several times in a
>> row.
>>
>> The command does not perform any kind of verification based on program
>> type (Is this program type allowed to use an input context?) or on data
>> consistency (Can I work with empty input data?), this is left to the
>> kernel.
>>
>> Example invocation:
>>
>> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
>> pinned /sys/fs/bpf/sample_ret0 \
>> data_in - data_out - repeat 5
>> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
>> Return value: 0, duration (average): 260ns
>>
>> When one of data_in or ctx_in is "-", bpftool reads from standard input,
>> in binary format. Other formats (JSON, hexdump) might be supported (via
>> an optional command line keyword like "data_fmt_in") in the future if
>> relevant, but this would require doing more parsing in bpftool.
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> ---
[...]
>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>> index 9b0db5d14e31..8dcbaa0a8ab1 100644
>> --- a/tools/bpf/bpftool/prog.c
>> +++ b/tools/bpf/bpftool/prog.c
>> @@ -15,6 +15,7 @@
>> #include <sys/stat.h>
>>
>> #include <linux/err.h>
>> +#include <linux/sizes.h>
>>
>> #include <bpf.h>
>> #include <btf.h>
>> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
>> return 0;
>> }
>>
>> +static int check_single_stdin(char *file_in, char *other_file_in)
>> +{
>> + if (file_in && other_file_in &&
>> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
>> + p_err("cannot use standard input for both data_in and ctx_in");
>
> The error message says data_in and ctx_in.
> Maybe the input parameter should be file_data_in and file_ctx_in?
Hi Yonghong,
It's true those parameters should be file names. But having
"file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
command line seems a bit heavy to me? (And relying on keyword prefixing
for typing the command won't help much.)
My opinion is that it should be clear from the man page or the "help"
command that the parameters are file names. What do you think? I can
prefix all four arguments with "file_" if you believe this is better.
[...]
>> +static int do_run(int argc, char **argv)
>> +{
>> + char *data_fname_in = NULL, *data_fname_out = NULL;
>> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
>> + struct bpf_prog_test_run_attr test_attr = {0};
>> + const unsigned int default_size = SZ_32K;
>> + void *data_in = NULL, *data_out = NULL;
>> + void *ctx_in = NULL, *ctx_out = NULL;
>> + unsigned int repeat = 1;
>> + int fd, err;
>> +
>> + if (!REQ_ARGS(4))
>> + return -1;
>> +
>> + fd = prog_parse_fd(&argc, &argv);
>> + if (fd < 0)
>> + return -1;
>> +
>> + while (argc) {
>> + if (detect_common_prefix(*argv, "data_in", "data_out",
>> + "data_size_out", NULL))
>> + return -1;
>> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
>> + "ctx_size_out", NULL))
>> + return -1;
>> +
>> + if (is_prefix(*argv, "data_in")) {
>> + NEXT_ARG();
>> + if (!REQ_ARGS(1))
>> + return -1;
>> +
>> + data_fname_in = GET_ARG();
>> + if (check_single_stdin(data_fname_in, ctx_fname_in))
>> + return -1;
>> + } else if (is_prefix(*argv, "data_out")) {
>
> Here, we all use is_prefix() to match "data_in", "data_out",
> "data_size_out" etc.
> That means users can use "data_i" instead of "data_in" as below
> ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
> is this expected?
Yes, this is expected. We use prefix matching as we do pretty much
everywhere else in bpftool. It's not as useful here because most of the
strings for the names are similar. I agree that typing "data_i" instead
of "data_in" brings little advantage, but I see no reason why we should
reject prefixing for those keywords. And we accept "data_s" instead of
"data_size_out", which is still shorter to type than the complete keyword.
Thanks for the review!
Quentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
2019-07-05 8:21 ` Quentin Monnet
@ 2019-07-05 15:42 ` Y Song
2019-07-05 16:03 ` Quentin Monnet
0 siblings, 1 reply; 7+ messages in thread
From: Y Song @ 2019-07-05 15:42 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
> > On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
> > <quentin.monnet@netronome.com> wrote:
> >>
> >> Add a new "bpftool prog run" subcommand to run a loaded program on input
> >> data (and possibly with input context) passed by the user.
> >>
> >> Print output data (and output context if relevant) into a file or into
> >> the console. Print return value and duration for the test run into the
> >> console.
> >>
> >> A "repeat" argument can be passed to run the program several times in a
> >> row.
> >>
> >> The command does not perform any kind of verification based on program
> >> type (Is this program type allowed to use an input context?) or on data
> >> consistency (Can I work with empty input data?), this is left to the
> >> kernel.
> >>
> >> Example invocation:
> >>
> >> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
> >> pinned /sys/fs/bpf/sample_ret0 \
> >> data_in - data_out - repeat 5
> >> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
> >> Return value: 0, duration (average): 260ns
> >>
> >> When one of data_in or ctx_in is "-", bpftool reads from standard input,
> >> in binary format. Other formats (JSON, hexdump) might be supported (via
> >> an optional command line keyword like "data_fmt_in") in the future if
> >> relevant, but this would require doing more parsing in bpftool.
> >>
> >> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> >> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >> ---
>
> [...]
>
> >> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> >> index 9b0db5d14e31..8dcbaa0a8ab1 100644
> >> --- a/tools/bpf/bpftool/prog.c
> >> +++ b/tools/bpf/bpftool/prog.c
> >> @@ -15,6 +15,7 @@
> >> #include <sys/stat.h>
> >>
> >> #include <linux/err.h>
> >> +#include <linux/sizes.h>
> >>
> >> #include <bpf.h>
> >> #include <btf.h>
> >> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
> >> return 0;
> >> }
> >>
> >> +static int check_single_stdin(char *file_in, char *other_file_in)
> >> +{
> >> + if (file_in && other_file_in &&
> >> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
> >> + p_err("cannot use standard input for both data_in and ctx_in");
> >
> > The error message says data_in and ctx_in.
> > Maybe the input parameter should be file_data_in and file_ctx_in?
>
>
> Hi Yonghong,
>
> It's true those parameters should be file names. But having
> "file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
> command line seems a bit heavy to me? (And relying on keyword prefixing
> for typing the command won't help much.)
>
> My opinion is that it should be clear from the man page or the "help"
> command that the parameters are file names. What do you think? I can
> prefix all four arguments with "file_" if you believe this is better.
I think you misunderstood my question above. The command line
parameters are fine.
I am talking about the function parameter names. Since in the error message,
the input parameters are referred for data_in and ctx_in
p_err("cannot use standard input for both data_in and ctx_in")
maybe the function signature should be
static int check_single_stdin(char *file_data_in, char *file_ctx_in)
If you are worried that later on the same function can be used in different
contexts, then alternatively, you can have signature like
static int check_single_stdin(char *file_in, char *other_file_in,
const char *file_in_arg, const char *other_file_in_arg)
where file_in_arg will be passed in as "data_in" and other_file_in_arg
as "ctx_in".
I think we could delay this until it is really needed.
>
> [...]
>
> >> +static int do_run(int argc, char **argv)
> >> +{
> >> + char *data_fname_in = NULL, *data_fname_out = NULL;
> >> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
> >> + struct bpf_prog_test_run_attr test_attr = {0};
> >> + const unsigned int default_size = SZ_32K;
> >> + void *data_in = NULL, *data_out = NULL;
> >> + void *ctx_in = NULL, *ctx_out = NULL;
> >> + unsigned int repeat = 1;
> >> + int fd, err;
> >> +
> >> + if (!REQ_ARGS(4))
> >> + return -1;
> >> +
> >> + fd = prog_parse_fd(&argc, &argv);
> >> + if (fd < 0)
> >> + return -1;
> >> +
> >> + while (argc) {
> >> + if (detect_common_prefix(*argv, "data_in", "data_out",
> >> + "data_size_out", NULL))
> >> + return -1;
> >> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
> >> + "ctx_size_out", NULL))
> >> + return -1;
> >> +
> >> + if (is_prefix(*argv, "data_in")) {
> >> + NEXT_ARG();
> >> + if (!REQ_ARGS(1))
> >> + return -1;
> >> +
> >> + data_fname_in = GET_ARG();
> >> + if (check_single_stdin(data_fname_in, ctx_fname_in))
> >> + return -1;
> >> + } else if (is_prefix(*argv, "data_out")) {
> >
> > Here, we all use is_prefix() to match "data_in", "data_out",
> > "data_size_out" etc.
> > That means users can use "data_i" instead of "data_in" as below
> > ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
> > is this expected?
> Yes, this is expected. We use prefix matching as we do pretty much
> everywhere else in bpftool. It's not as useful here because most of the
> strings for the names are similar. I agree that typing "data_i" instead
> of "data_in" brings little advantage, but I see no reason why we should
> reject prefixing for those keywords. And we accept "data_s" instead of
> "data_size_out", which is still shorter to type than the complete keyword.
This makes sense. Thanks for explanation.
Another question. Currently, you are proposing "./bpftool prog run ...",
but actually it is just a test_run. Do you think we should rename it
to "./bpftool prog test_run ..." to make it clear for its intention?
>
> Thanks for the review!
> Quentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
2019-07-05 15:42 ` Y Song
@ 2019-07-05 16:03 ` Quentin Monnet
2019-07-05 17:08 ` Y Song
0 siblings, 1 reply; 7+ messages in thread
From: Quentin Monnet @ 2019-07-05 16:03 UTC (permalink / raw)
To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
2019-07-05 08:42 UTC-0700 ~ Y Song <ys114321@gmail.com>
> On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
>>
>> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
>>> On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
>>> <quentin.monnet@netronome.com> wrote:
>>>>
>>>> Add a new "bpftool prog run" subcommand to run a loaded program on input
>>>> data (and possibly with input context) passed by the user.
>>>>
>>>> Print output data (and output context if relevant) into a file or into
>>>> the console. Print return value and duration for the test run into the
>>>> console.
>>>>
>>>> A "repeat" argument can be passed to run the program several times in a
>>>> row.
>>>>
>>>> The command does not perform any kind of verification based on program
>>>> type (Is this program type allowed to use an input context?) or on data
>>>> consistency (Can I work with empty input data?), this is left to the
>>>> kernel.
>>>>
>>>> Example invocation:
>>>>
>>>> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
>>>> pinned /sys/fs/bpf/sample_ret0 \
>>>> data_in - data_out - repeat 5
>>>> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
>>>> Return value: 0, duration (average): 260ns
>>>>
>>>> When one of data_in or ctx_in is "-", bpftool reads from standard input,
>>>> in binary format. Other formats (JSON, hexdump) might be supported (via
>>>> an optional command line keyword like "data_fmt_in") in the future if
>>>> relevant, but this would require doing more parsing in bpftool.
>>>>
>>>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>>> ---
>>
>> [...]
>>
>>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>>>> index 9b0db5d14e31..8dcbaa0a8ab1 100644
>>>> --- a/tools/bpf/bpftool/prog.c
>>>> +++ b/tools/bpf/bpftool/prog.c
>>>> @@ -15,6 +15,7 @@
>>>> #include <sys/stat.h>
>>>>
>>>> #include <linux/err.h>
>>>> +#include <linux/sizes.h>
>>>>
>>>> #include <bpf.h>
>>>> #include <btf.h>
>>>> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
>>>> return 0;
>>>> }
>>>>
>>>> +static int check_single_stdin(char *file_in, char *other_file_in)
>>>> +{
>>>> + if (file_in && other_file_in &&
>>>> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
>>>> + p_err("cannot use standard input for both data_in and ctx_in");
>>>
>>> The error message says data_in and ctx_in.
>>> Maybe the input parameter should be file_data_in and file_ctx_in?
>>
>>
>> Hi Yonghong,
>>
>> It's true those parameters should be file names. But having
>> "file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
>> command line seems a bit heavy to me? (And relying on keyword prefixing
>> for typing the command won't help much.)
>>
>> My opinion is that it should be clear from the man page or the "help"
>> command that the parameters are file names. What do you think? I can
>> prefix all four arguments with "file_" if you believe this is better.
>
> I think you misunderstood my question above.
Totally did, sorry :/.
> The command line parameters are fine.
> I am talking about the function parameter names. Since in the error message,
> the input parameters are referred for data_in and ctx_in
> p_err("cannot use standard input for both data_in and ctx_in")
> maybe the function signature should be
> static int check_single_stdin(char *file_data_in, char *file_ctx_in)
>
> If you are worried that later on the same function can be used in different
> contexts, then alternatively, you can have signature like
> static int check_single_stdin(char *file_in, char *other_file_in,
> const char *file_in_arg, const char *other_file_in_arg)
> where file_in_arg will be passed in as "data_in" and other_file_in_arg
> as "ctx_in".
> I think we could delay this until it is really needed.
As a matter of fact, the opposite thing happened. I first used the
function for data_in/ctx_in, and also for data_out/ctx_out. But I
changed my mind eventually because there is no real reason not to print
both data_out and ctx_out to stdout if we want to do so. So I updated
the name of the parameters in the error messages, but forgot to change
the arguments for the function. Silly me.
So I totally agree, I'll respin and change the argument names for the
function. And yes, we could also pass the names to print in the error
message, but I agree that this is not needed, and not helpful at the moment.
Thanks for catching this!
>>
>> [...]
>>
>>>> +static int do_run(int argc, char **argv)
>>>> +{
>>>> + char *data_fname_in = NULL, *data_fname_out = NULL;
>>>> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
>>>> + struct bpf_prog_test_run_attr test_attr = {0};
>>>> + const unsigned int default_size = SZ_32K;
>>>> + void *data_in = NULL, *data_out = NULL;
>>>> + void *ctx_in = NULL, *ctx_out = NULL;
>>>> + unsigned int repeat = 1;
>>>> + int fd, err;
>>>> +
>>>> + if (!REQ_ARGS(4))
>>>> + return -1;
>>>> +
>>>> + fd = prog_parse_fd(&argc, &argv);
>>>> + if (fd < 0)
>>>> + return -1;
>>>> +
>>>> + while (argc) {
>>>> + if (detect_common_prefix(*argv, "data_in", "data_out",
>>>> + "data_size_out", NULL))
>>>> + return -1;
>>>> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
>>>> + "ctx_size_out", NULL))
>>>> + return -1;
>>>> +
>>>> + if (is_prefix(*argv, "data_in")) {
>>>> + NEXT_ARG();
>>>> + if (!REQ_ARGS(1))
>>>> + return -1;
>>>> +
>>>> + data_fname_in = GET_ARG();
>>>> + if (check_single_stdin(data_fname_in, ctx_fname_in))
>>>> + return -1;
>>>> + } else if (is_prefix(*argv, "data_out")) {
>>>
>>> Here, we all use is_prefix() to match "data_in", "data_out",
>>> "data_size_out" etc.
>>> That means users can use "data_i" instead of "data_in" as below
>>> ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
>>> is this expected?
>> Yes, this is expected. We use prefix matching as we do pretty much
>> everywhere else in bpftool. It's not as useful here because most of the
>> strings for the names are similar. I agree that typing "data_i" instead
>> of "data_in" brings little advantage, but I see no reason why we should
>> reject prefixing for those keywords. And we accept "data_s" instead of
>> "data_size_out", which is still shorter to type than the complete keyword.
>
> This makes sense. Thanks for explanation.
>
> Another question. Currently, you are proposing "./bpftool prog run ...",
> but actually it is just a test_run. Do you think we should rename it
> to "./bpftool prog test_run ..." to make it clear for its intention?
Good question. Hmm. It would make it more explicit that we use the
BPF_PROG_TEST_RUN command, but at the same time, from the point of view
of the user, there is nothing in particular that makes it a test run, is
it? I mean, you provide input data, you get output data and return
value, that makes it a real BPF run somehow, except that it's not on a
packet or anything. Do you think it is ambiguous and people may confuse
it with something like "attach"?
Thanks,
Quentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
2019-07-05 16:03 ` Quentin Monnet
@ 2019-07-05 17:08 ` Y Song
2019-07-05 17:50 ` Quentin Monnet
0 siblings, 1 reply; 7+ messages in thread
From: Y Song @ 2019-07-05 17:08 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
On Fri, Jul 5, 2019 at 9:03 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> 2019-07-05 08:42 UTC-0700 ~ Y Song <ys114321@gmail.com>
> > On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
> > <quentin.monnet@netronome.com> wrote:
> >>
> >> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
> >>> On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
> >>> <quentin.monnet@netronome.com> wrote:
> >>>>
> >>>> Add a new "bpftool prog run" subcommand to run a loaded program on input
> >>>> data (and possibly with input context) passed by the user.
> >>>>
> >>>> Print output data (and output context if relevant) into a file or into
> >>>> the console. Print return value and duration for the test run into the
> >>>> console.
> >>>>
> >>>> A "repeat" argument can be passed to run the program several times in a
> >>>> row.
> >>>>
> >>>> The command does not perform any kind of verification based on program
> >>>> type (Is this program type allowed to use an input context?) or on data
> >>>> consistency (Can I work with empty input data?), this is left to the
> >>>> kernel.
> >>>>
> >>>> Example invocation:
> >>>>
> >>>> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
> >>>> pinned /sys/fs/bpf/sample_ret0 \
> >>>> data_in - data_out - repeat 5
> >>>> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
> >>>> Return value: 0, duration (average): 260ns
> >>>>
> >>>> When one of data_in or ctx_in is "-", bpftool reads from standard input,
> >>>> in binary format. Other formats (JSON, hexdump) might be supported (via
> >>>> an optional command line keyword like "data_fmt_in") in the future if
> >>>> relevant, but this would require doing more parsing in bpftool.
> >>>>
> >>>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> >>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >>>> ---
> >>
> >> [...]
> >>
> >>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> >>>> index 9b0db5d14e31..8dcbaa0a8ab1 100644
> >>>> --- a/tools/bpf/bpftool/prog.c
> >>>> +++ b/tools/bpf/bpftool/prog.c
> >>>> @@ -15,6 +15,7 @@
> >>>> #include <sys/stat.h>
> >>>>
> >>>> #include <linux/err.h>
> >>>> +#include <linux/sizes.h>
> >>>>
> >>>> #include <bpf.h>
> >>>> #include <btf.h>
> >>>> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static int check_single_stdin(char *file_in, char *other_file_in)
> >>>> +{
> >>>> + if (file_in && other_file_in &&
> >>>> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
> >>>> + p_err("cannot use standard input for both data_in and ctx_in");
> >>>
> >>> The error message says data_in and ctx_in.
> >>> Maybe the input parameter should be file_data_in and file_ctx_in?
> >>
> >>
> >> Hi Yonghong,
> >>
> >> It's true those parameters should be file names. But having
> >> "file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
> >> command line seems a bit heavy to me? (And relying on keyword prefixing
> >> for typing the command won't help much.)
> >>
> >> My opinion is that it should be clear from the man page or the "help"
> >> command that the parameters are file names. What do you think? I can
> >> prefix all four arguments with "file_" if you believe this is better.
> >
> > I think you misunderstood my question above.
>
> Totally did, sorry :/.
>
> > The command line parameters are fine.
> > I am talking about the function parameter names. Since in the error message,
> > the input parameters are referred for data_in and ctx_in
> > p_err("cannot use standard input for both data_in and ctx_in")
> > maybe the function signature should be
> > static int check_single_stdin(char *file_data_in, char *file_ctx_in)
> >
> > If you are worried that later on the same function can be used in different
> > contexts, then alternatively, you can have signature like
> > static int check_single_stdin(char *file_in, char *other_file_in,
> > const char *file_in_arg, const char *other_file_in_arg)
> > where file_in_arg will be passed in as "data_in" and other_file_in_arg
> > as "ctx_in".
> > I think we could delay this until it is really needed.
>
> As a matter of fact, the opposite thing happened. I first used the
> function for data_in/ctx_in, and also for data_out/ctx_out. But I
> changed my mind eventually because there is no real reason not to print
> both data_out and ctx_out to stdout if we want to do so. So I updated
> the name of the parameters in the error messages, but forgot to change
> the arguments for the function. Silly me.
>
> So I totally agree, I'll respin and change the argument names for the
> function. And yes, we could also pass the names to print in the error
> message, but I agree that this is not needed, and not helpful at the moment.
>
> Thanks for catching this!
>
> >>
> >> [...]
> >>
> >>>> +static int do_run(int argc, char **argv)
> >>>> +{
> >>>> + char *data_fname_in = NULL, *data_fname_out = NULL;
> >>>> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
> >>>> + struct bpf_prog_test_run_attr test_attr = {0};
> >>>> + const unsigned int default_size = SZ_32K;
> >>>> + void *data_in = NULL, *data_out = NULL;
> >>>> + void *ctx_in = NULL, *ctx_out = NULL;
> >>>> + unsigned int repeat = 1;
> >>>> + int fd, err;
> >>>> +
> >>>> + if (!REQ_ARGS(4))
> >>>> + return -1;
> >>>> +
> >>>> + fd = prog_parse_fd(&argc, &argv);
> >>>> + if (fd < 0)
> >>>> + return -1;
> >>>> +
> >>>> + while (argc) {
> >>>> + if (detect_common_prefix(*argv, "data_in", "data_out",
> >>>> + "data_size_out", NULL))
> >>>> + return -1;
> >>>> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
> >>>> + "ctx_size_out", NULL))
> >>>> + return -1;
> >>>> +
> >>>> + if (is_prefix(*argv, "data_in")) {
> >>>> + NEXT_ARG();
> >>>> + if (!REQ_ARGS(1))
> >>>> + return -1;
> >>>> +
> >>>> + data_fname_in = GET_ARG();
> >>>> + if (check_single_stdin(data_fname_in, ctx_fname_in))
> >>>> + return -1;
> >>>> + } else if (is_prefix(*argv, "data_out")) {
> >>>
> >>> Here, we all use is_prefix() to match "data_in", "data_out",
> >>> "data_size_out" etc.
> >>> That means users can use "data_i" instead of "data_in" as below
> >>> ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
> >>> is this expected?
> >> Yes, this is expected. We use prefix matching as we do pretty much
> >> everywhere else in bpftool. It's not as useful here because most of the
> >> strings for the names are similar. I agree that typing "data_i" instead
> >> of "data_in" brings little advantage, but I see no reason why we should
> >> reject prefixing for those keywords. And we accept "data_s" instead of
> >> "data_size_out", which is still shorter to type than the complete keyword.
> >
> > This makes sense. Thanks for explanation.
> >
> > Another question. Currently, you are proposing "./bpftool prog run ...",
> > but actually it is just a test_run. Do you think we should rename it
> > to "./bpftool prog test_run ..." to make it clear for its intention?
>
> Good question. Hmm. It would make it more explicit that we use the
> BPF_PROG_TEST_RUN command, but at the same time, from the point of view
> of the user, there is nothing in particular that makes it a test run, is
> it? I mean, you provide input data, you get output data and return
> value, that makes it a real BPF run somehow, except that it's not on a
> packet or anything. Do you think it is ambiguous and people may confuse
> it with something like "attach"?
I am more thinking about whether we could have a real "bpftool prog run ..."
in the future which could really run the program in some kind of production
environment...
But I could be wrong since after "bpf prog attach" it may already just start
to run in production, so "bpf prog run ..." not really needed for it.
So "bpf prog run ..." is probably fine.
>
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
2019-07-05 17:08 ` Y Song
@ 2019-07-05 17:50 ` Quentin Monnet
0 siblings, 0 replies; 7+ messages in thread
From: Quentin Monnet @ 2019-07-05 17:50 UTC (permalink / raw)
To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
2019-07-05 10:08 UTC-0700 ~ Y Song <ys114321@gmail.com>
> On Fri, Jul 5, 2019 at 9:03 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
>>
>> 2019-07-05 08:42 UTC-0700 ~ Y Song <ys114321@gmail.com>
>>> On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
>>> <quentin.monnet@netronome.com> wrote:
>>>>
>>>> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
>>>>> On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
>>>>> <quentin.monnet@netronome.com> wrote:
>>>>>>
>>>>>> Add a new "bpftool prog run" subcommand to run a loaded program on input
>>>>>> data (and possibly with input context) passed by the user.
>>>>>>
>>>>>> Print output data (and output context if relevant) into a file or into
>>>>>> the console. Print return value and duration for the test run into the
>>>>>> console.
>>>>>>
>>>>>> A "repeat" argument can be passed to run the program several times in a
>>>>>> row.
>>>>>>
>>>>>> The command does not perform any kind of verification based on program
>>>>>> type (Is this program type allowed to use an input context?) or on data
>>>>>> consistency (Can I work with empty input data?), this is left to the
>>>>>> kernel.
>>>>>>
>>>>>> Example invocation:
>>>>>>
>>>>>> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
>>>>>> pinned /sys/fs/bpf/sample_ret0 \
>>>>>> data_in - data_out - repeat 5
>>>>>> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
>>>>>> Return value: 0, duration (average): 260ns
>>>>>>
>>>>>> When one of data_in or ctx_in is "-", bpftool reads from standard input,
>>>>>> in binary format. Other formats (JSON, hexdump) might be supported (via
>>>>>> an optional command line keyword like "data_fmt_in") in the future if
>>>>>> relevant, but this would require doing more parsing in bpftool.
>>>>>>
>>>>>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>>>>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>>>>>> index 9b0db5d14e31..8dcbaa0a8ab1 100644
>>>>>> --- a/tools/bpf/bpftool/prog.c
>>>>>> +++ b/tools/bpf/bpftool/prog.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>> #include <sys/stat.h>
>>>>>>
>>>>>> #include <linux/err.h>
>>>>>> +#include <linux/sizes.h>
>>>>>>
>>>>>> #include <bpf.h>
>>>>>> #include <btf.h>
>>>>>> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +static int check_single_stdin(char *file_in, char *other_file_in)
>>>>>> +{
>>>>>> + if (file_in && other_file_in &&
>>>>>> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
>>>>>> + p_err("cannot use standard input for both data_in and ctx_in");
>>>>>
>>>>> The error message says data_in and ctx_in.
>>>>> Maybe the input parameter should be file_data_in and file_ctx_in?
>>>>
>>>>
>>>> Hi Yonghong,
>>>>
>>>> It's true those parameters should be file names. But having
>>>> "file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
>>>> command line seems a bit heavy to me? (And relying on keyword prefixing
>>>> for typing the command won't help much.)
>>>>
>>>> My opinion is that it should be clear from the man page or the "help"
>>>> command that the parameters are file names. What do you think? I can
>>>> prefix all four arguments with "file_" if you believe this is better.
>>>
>>> I think you misunderstood my question above.
>>
>> Totally did, sorry :/.
>>
>>> The command line parameters are fine.
>>> I am talking about the function parameter names. Since in the error message,
>>> the input parameters are referred for data_in and ctx_in
>>> p_err("cannot use standard input for both data_in and ctx_in")
>>> maybe the function signature should be
>>> static int check_single_stdin(char *file_data_in, char *file_ctx_in)
>>>
>>> If you are worried that later on the same function can be used in different
>>> contexts, then alternatively, you can have signature like
>>> static int check_single_stdin(char *file_in, char *other_file_in,
>>> const char *file_in_arg, const char *other_file_in_arg)
>>> where file_in_arg will be passed in as "data_in" and other_file_in_arg
>>> as "ctx_in".
>>> I think we could delay this until it is really needed.
>>
>> As a matter of fact, the opposite thing happened. I first used the
>> function for data_in/ctx_in, and also for data_out/ctx_out. But I
>> changed my mind eventually because there is no real reason not to print
>> both data_out and ctx_out to stdout if we want to do so. So I updated
>> the name of the parameters in the error messages, but forgot to change
>> the arguments for the function. Silly me.
>>
>> So I totally agree, I'll respin and change the argument names for the
>> function. And yes, we could also pass the names to print in the error
>> message, but I agree that this is not needed, and not helpful at the moment.
>>
>> Thanks for catching this!
>>
>>>>
>>>> [...]
>>>>
>>>>>> +static int do_run(int argc, char **argv)
>>>>>> +{
>>>>>> + char *data_fname_in = NULL, *data_fname_out = NULL;
>>>>>> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
>>>>>> + struct bpf_prog_test_run_attr test_attr = {0};
>>>>>> + const unsigned int default_size = SZ_32K;
>>>>>> + void *data_in = NULL, *data_out = NULL;
>>>>>> + void *ctx_in = NULL, *ctx_out = NULL;
>>>>>> + unsigned int repeat = 1;
>>>>>> + int fd, err;
>>>>>> +
>>>>>> + if (!REQ_ARGS(4))
>>>>>> + return -1;
>>>>>> +
>>>>>> + fd = prog_parse_fd(&argc, &argv);
>>>>>> + if (fd < 0)
>>>>>> + return -1;
>>>>>> +
>>>>>> + while (argc) {
>>>>>> + if (detect_common_prefix(*argv, "data_in", "data_out",
>>>>>> + "data_size_out", NULL))
>>>>>> + return -1;
>>>>>> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
>>>>>> + "ctx_size_out", NULL))
>>>>>> + return -1;
>>>>>> +
>>>>>> + if (is_prefix(*argv, "data_in")) {
>>>>>> + NEXT_ARG();
>>>>>> + if (!REQ_ARGS(1))
>>>>>> + return -1;
>>>>>> +
>>>>>> + data_fname_in = GET_ARG();
>>>>>> + if (check_single_stdin(data_fname_in, ctx_fname_in))
>>>>>> + return -1;
>>>>>> + } else if (is_prefix(*argv, "data_out")) {
>>>>>
>>>>> Here, we all use is_prefix() to match "data_in", "data_out",
>>>>> "data_size_out" etc.
>>>>> That means users can use "data_i" instead of "data_in" as below
>>>>> ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
>>>>> is this expected?
>>>> Yes, this is expected. We use prefix matching as we do pretty much
>>>> everywhere else in bpftool. It's not as useful here because most of the
>>>> strings for the names are similar. I agree that typing "data_i" instead
>>>> of "data_in" brings little advantage, but I see no reason why we should
>>>> reject prefixing for those keywords. And we accept "data_s" instead of
>>>> "data_size_out", which is still shorter to type than the complete keyword.
>>>
>>> This makes sense. Thanks for explanation.
>>>
>>> Another question. Currently, you are proposing "./bpftool prog run ...",
>>> but actually it is just a test_run. Do you think we should rename it
>>> to "./bpftool prog test_run ..." to make it clear for its intention?
>>
>> Good question. Hmm. It would make it more explicit that we use the
>> BPF_PROG_TEST_RUN command, but at the same time, from the point of view
>> of the user, there is nothing in particular that makes it a test run, is
>> it? I mean, you provide input data, you get output data and return
>> value, that makes it a real BPF run somehow, except that it's not on a
>> packet or anything. Do you think it is ambiguous and people may confuse
>> it with something like "attach"?
>
> I am more thinking about whether we could have a real "bpftool prog run ..."
> in the future which could really run the program in some kind of production
> environment...
>
> But I could be wrong since after "bpf prog attach" it may already just start
> to run in production, so "bpf prog run ..." not really needed for it.
>
> So "bpf prog run ..." is probably fine.
Ok, I'll stick to "prog run" unless someone else comments, then. I
suppose we can find something else, like "bpftool prog start", if we
need something like the feature you describe someday.
I'll send a v2 with the fix for the arguments in check_single_stdin().
Thanks,
Quentin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-05 17:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04 8:56 [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs Quentin Monnet
2019-07-05 5:49 ` Y Song
2019-07-05 8:21 ` Quentin Monnet
2019-07-05 15:42 ` Y Song
2019-07-05 16:03 ` Quentin Monnet
2019-07-05 17:08 ` Y Song
2019-07-05 17:50 ` Quentin Monnet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.