On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
<quentin.monnet@xxxxxxxxxxxxx> wrote:
>
> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@xxxxxxxxx>
> > On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
> > <quentin.monnet@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> Reviewed-by: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> >> ---
>
> [...]
>
> >> 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
