Message-ID: <CAHap4ztH=EbFMtj1h5s3-23h06u_L3o8NU9cOL=6nzENZiq_XA@mail.gmail.com>
Date:   Fri, 21 Jan 2022 15:40:22 -0500
From:   Mauricio Vásquez Bernal <mauricio@...volk.io>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Rafael David Tinoco <rafaeldtinoco@...il.com>,
        Lorenzo Fontana <lorenzo.fontana@...stic.co>,
        Leonardo Di Donato <leonardo.didonato@...stic.co>
Subject: Re: [PATCH bpf-next v4 3/8] bpftool: Add gen btf command

On Wed, Jan 12, 2022 at 1:08 PM Quentin Monnet <quentin@...valent.com> wrote:
>
> 2022-01-12 09:27 UTC-0500 ~ Mauricio Vásquez <mauricio@...volk.io>
> > This command is implemented under the "gen" command in bpftool and the
> > syntax is the following:
> >
> > $ bpftool gen btf INPUT OUTPUT OBJECT(S)
>
> Thanks a lot for this work!
>
> Please update the relevant manual page under Documentation, to let users
> know how to use the feature. You may also consider adding an example at
> the end of that document.
>

We're working on it, and will be part of the next spin.

> The bash completion script should also be updated with the new "btf"
> subcommand for "gen". Given that all the arguments are directories and
> files, it should not be hard.
>

Will do.

> Have you considered adding tests for the feature? There are a few
> bpftool-related selftests under tools/testing/selftests/bpf/.
>

Yes, we have but it seems not that trivial. One idea I have is to
include a couple of BTF source files from
https://github.com/aquasecurity/btfhub-archive/ and create a test
program that generates some field, type and enum relocations. Then we
could check if the generated BTF file has the expected types, fields
and so on by parsing it and using the BTF API from libbpf. One concern
about it is the size of those two source BTF files (~5MB each),
perhaps we should not include a full file but something that is
already "trimmed"? Another possibility is to use
"/sys/kernel/btf/vmlinux" but it'll limit the test to machines with
CONFIG_DEBUG_INFO_BTF.

Do you have any ideas / feedback on this one? Should the tests be
included in this series or can we push them later on?

> >
> > INPUT can be either a single BTF file or a folder containing BTF files,
> > when it's a folder, a BTF file is generated for each BTF file contained
> > in this folder. OUTPUT is the file (or folder) where generated files are
> > stored and OBJECT(S) is the list of bpf objects we want to generate the
> > BTF file(s) for (each generated BTF file contains all the types needed
> > by all the objects).
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@...volk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@...asec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@...stic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@...stic.co>
> > ---
> >  tools/bpf/bpftool/gen.c | 117 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 43e3f8700ecc..cdeb1047d79d 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -5,6 +5,7 @@
> >  #define _GNU_SOURCE
> >  #endif
> >  #include <ctype.h>
> > +#include <dirent.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <linux/err.h>
> > @@ -1084,6 +1085,7 @@ static int do_help(int argc, char **argv)
> >       fprintf(stderr,
> >               "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
> >               "       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
> > +             "       %1$s %2$s btf INPUT OUTPUT OBJECT(S)\n"
> >               "       %1$s %2$s help\n"
> >               "\n"
> >               "       " HELP_SPEC_OPTIONS " |\n"
> > @@ -1094,9 +1096,124 @@ static int do_help(int argc, char **argv)
> >       return 0;
> >  }
> >
> > +/* Create BTF file for a set of BPF objects */
> > +static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static int is_file(const char *path)
> > +{
> > +     struct stat st;
> > +
> > +     if (stat(path, &st) < 0)
> > +             return -1;
> > +
> > +     switch (st.st_mode & S_IFMT) {
> > +     case S_IFDIR:
> > +             return 0;
> > +     case S_IFREG:
> > +             return 1;
> > +     default:
> > +             return -1;
> > +     }
> > +}
> > +
> > +static int do_gen_btf(int argc, char **argv)
> > +{
> > +     char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
> > +     bool input_is_file, output_is_file = false;
> > +     const char *input, *output;
> > +     const char **objs = NULL;
> > +     struct dirent *dir;
> > +     DIR *d = NULL;
> > +     int i, err;
> > +
> > +     if (!REQ_ARGS(3)) {
> > +             usage();
> > +             return -1;
> > +     }
> > +
> > +     input = GET_ARG();
> > +     err = is_file(input);
> > +     if (err < 0) {
> > +             p_err("failed to stat %s: %s", input, strerror(errno));
> > +             return err;
> > +     }
> > +     input_is_file = err;
> > +
> > +     output = GET_ARG();
> > +     err = is_file(output);
> > +     if (err != 0)
> > +             output_is_file = true;
>
> Why not return if err < 0? This will set output_is_file to true and will
> fail later, I think?
>

I knew it was going to be confusing. is_file() returns -1 when the
file is not found, for the output one it's not a big deal as we're
going to create it, so this logic was correct. I decided to drop this
is_file() function and using stat() directly in the code.

> > +
> > +     objs = (const char **) malloc((argc + 1) * sizeof(*objs));
> > +     if (!objs)
> > +             return -ENOMEM;
>
> Let's p_err() a message.
>
> > +
> > +     i = 0;
> > +     while (argc > 0)
> > +             objs[i++] = GET_ARG();
> > +
> > +     objs[i] = NULL;
> > +
> > +     /* single BTF file */
> > +     if (input_is_file) {
> > +             printf("SBTF: %s\n", input);
>
> We can use "p_info()" instead of "printf()". In particular, this avoids
> printing the message when the JSON output is required.
>
> > +
> > +             if (output_is_file) {
> > +                     err = btfgen(input, output, objs);
> > +                     goto out;
> > +             }
> > +             snprintf(dst_btf_path, sizeof(dst_btf_path), "%s/%s", output,
> > +                      basename(input));
>
> Am I right that the output file should be just a file name, and not a
> path, and that it will be created under the same directory as the input
> file?

No exactly. There are two ways we support (1) the source BTF file is a
single file and (2) it's a folder containing BTF files.
For the first case, we require the output path to be a file or a
folder and the second case requires the output to be a folder. The
reason we don't generate the files in the same input folder is that
the user can have a library with a lot of BTF files (like
https://github.com/aquasecurity/btfhub-archive/) and you don't want to
pollute it with the files you generate.

> And that providing a relative or absolute path instead will cause
> issues here? If so, please document it. It would be nice to be able to
> support paths too, but I'm not sure how much work that represents.
>

I tried with absolute and relative paths and it seems to work fine.

> > +             err = btfgen(input, dst_btf_path, objs);
> > +             goto out;
> > +     }
> > +
> > +     if (output_is_file) {
> > +             p_err("can't have just one file as output");
>
> See comment above, this message is misleading if stat() returned with an
> error.
>
> > +             err = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     /* directory with BTF files */
> > +     d = opendir(input);
> > +     if (!d) {
> > +             p_err("error opening input dir: %s", strerror(errno));
> > +             err = -errno;
> > +             goto out;
> > +     }
> > +
> > +     while ((dir = readdir(d)) != NULL) {
> > +             if (dir->d_type != DT_REG)
> > +                     continue;
> > +
> > +             if (strncmp(dir->d_name + strlen(dir->d_name) - 4, ".btf", 4))
> > +                     continue;
> > +
> > +             snprintf(src_btf_path, sizeof(src_btf_path), "%s/%s", input, dir->d_name);
> > +             snprintf(dst_btf_path, sizeof(dst_btf_path), "%s/%s", output, dir->d_name);
> > +
> > +             printf("SBTF: %s\n", src_btf_path);
> > +
> > +             err = btfgen(src_btf_path, dst_btf_path, objs);
> > +             if (err)
> > +                     goto out;
> > +     }
> > +
> > +out:
> > +     if (!err)
> > +             printf("STAT: done!\n");
>
> p_info()
>
> > +     free(objs);
> > +     closedir(d);
>
> If input is a single file, "d" will not be a valid directory stream
> descriptor, and I expect closedir() to fail and set errno even if
> everything else went fine. The return code does not matter, but "errno
> != 0" may cause bpftool's batch mode to error out. Can you please run
> closedir() only "if (d)"?
>

Yes, you're totally right.





> > +     return err;
> > +}
> > +
> >  static const struct cmd cmds[] = {
> >       { "object",     do_object },
> >       { "skeleton",   do_skeleton },
> > +     { "btf",        do_gen_btf},
> >       { "help",       do_help },
> >       { 0 }
> >  };
>

Powered by blists - more mailing lists