Message-ID: <33e77eec-524a-ffb0-9efc-a58da532a578@isovalent.com>
Date:   Wed, 12 Jan 2022 18:08:56 +0000
From:   Quentin Monnet <quentin@...valent.com>
To:     Mauricio Vásquez <mauricio@...volk.io>,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Cc:     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

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.

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.

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

> 
> 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?

> +
> +	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? 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.

> +		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)"?

> +	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