Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs

[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. 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