| Message ID | 20200715051214.28099-1-Tony.Ambardar@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpftool: use only nftw for file tree parsing | expand |
2020-07-14 22:12 UTC-0700 ~ Tony Ambardar <tony.ambardar@gmail.com> > The bpftool sources include code to walk file trees, but use multiple > frameworks to do so: nftw and fts. While nftw conforms to POSIX/SUSv3 and > is widely available, fts is not conformant and less common, especially on > non-glibc systems. The inconsistent framework usage hampers maintenance > and portability of bpftool, in particular for embedded systems. > > Standardize usage by rewriting one fts-based function to use nftw. This > change allows building bpftool against musl for OpenWrt. > > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> Thanks! I tested your set, and bpftool does not compile on my setup. The definitions from <ftw.h> are not picked up by gcc, common.c should have a "#define _GNU_SOURCE" above its list of includes for this to work (like perf.c has). I also get a warning on this line: > +static int do_build_table_cb(const char *fpath, const struct stat *sb, > + int typeflag, struct FTW *ftwbuf) > { Because passing fptath to open_obj_pinned() below discards the "const" qualifier: > + fd = open_obj_pinned(fpath, true); Fixed by having simply "char *fpath" as the first argument for do_build_table_cb(). With those two modifications, bpftool compiles fine and listing objects with the "-f" option works as expected. Regards, Quentin
On Wed, 15 Jul 2020 at 10:35, Quentin Monnet <quentin@isovalent.com> wrote: > > 2020-07-14 22:12 UTC-0700 ~ Tony Ambardar <tony.ambardar@gmail.com> > > The bpftool sources include code to walk file trees, but use multiple > > frameworks to do so: nftw and fts. While nftw conforms to POSIX/SUSv3 and > > is widely available, fts is not conformant and less common, especially on > > non-glibc systems. The inconsistent framework usage hampers maintenance > > and portability of bpftool, in particular for embedded systems. > > > > Standardize usage by rewriting one fts-based function to use nftw. This > > change allows building bpftool against musl for OpenWrt. > > > > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> > > Thanks! > Thanks for your feedback and testing, Quentin, I really appreciate it. > I tested your set, and bpftool does not compile on my setup. The > definitions from <ftw.h> are not picked up by gcc, common.c should have > a "#define _GNU_SOURCE" above its list of includes for this to work > (like perf.c has). > OK, I see what happened. I omitted a required "#define _XOPEN_SOURCE ..." (like in cgroup.c). Strictly speaking, "_GNU_SOURCE" is only needed for a nftw() GNU extension not used in common.c or cgroup.c (but used perf.c). It turns out there are still problems with missing definitions for getpagesize() and getline(), which are most easily pulled in with "_GNU_SOURCE". Will update as you suggest. > I also get a warning on this line: > > > > +static int do_build_table_cb(const char *fpath, const struct stat *sb, > > + int typeflag, struct FTW *ftwbuf) > > { > > Because passing fptath to open_obj_pinned() below discards the "const" > qualifier: > > > + fd = open_obj_pinned(fpath, true); > > Fixed by having simply "char *fpath" as the first argument for > do_build_table_cb(). Hmm, that only shifts the warning, since the cb function signature for nftw still specifies "const char": > common.c: In function ‘build_pinned_obj_table’: > common.c:438:18: warning: passing argument 2 of ‘nftw’ from incompatible pointer type [-Wincompatible-pointer-types] > if (nftw(path, do_build_table_cb, nopenfd, flags) == -1) > ^~~~~~~~~~~~~~~~~ > In file included from common.c:9:0: > /usr/include/ftw.h:158:12: note: expected ‘__nftw_func_t {aka int (*)(const char *, const struct stat *, int, struct FTW *)}’ but argument is of type ‘int (*)(char *, const struct stat *, int, struct FTW *)’ > extern int nftw (const char *__dir, __nftw_func_t __func, int __descriptors, > ^~~~ Wouldn't it be better/safer in general to constify the passed char to 'open_obj_pinned' and 'open_obj_pinned_any'? However, doing so revealed a problem in open_obj_pinned(), where dirname() is called directly on the passed string. This could be dangerous since some dirname() implementations may modify the string. Let's copy the string instead (same approach in tools/lib/bpf/libbpf.c). > With those two modifications, bpftool compiles fine and listing objects > with the "-f" option works as expected. > > Regards, > Quentin Let me make these changes and see what you think. Best regards, Tony
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index 29f4e7611ae8..765de99770fb 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -4,7 +4,7 @@ #include <ctype.h> #include <errno.h> #include <fcntl.h> -#include <fts.h> +#include <ftw.h> #include <libgen.h> #include <mntent.h> #include <stdbool.h> @@ -367,68 +367,74 @@ void print_hex_data_json(uint8_t *data, size_t len) jsonw_end_array(json_wtr); } -int build_pinned_obj_table(struct pinned_obj_table *tab, - enum bpf_obj_type type) +static struct pinned_obj_table *build_fn_table; /* params for nftw cb*/ +static enum bpf_obj_type build_fn_type; + +static int do_build_table_cb(const char *fpath, const struct stat *sb, + int typeflag, struct FTW *ftwbuf) { struct bpf_prog_info pinned_info = {}; struct pinned_obj *obj_node = NULL; __u32 len = sizeof(pinned_info); - struct mntent *mntent = NULL; enum bpf_obj_type objtype; + int fd, err = 0; + + if (typeflag != FTW_F) + goto out_ret; + fd = open_obj_pinned(fpath, true); + if (fd < 0) + goto out_ret; + + objtype = get_fd_type(fd); + if (objtype != build_fn_type) + goto out_close; + + memset(&pinned_info, 0, sizeof(pinned_info)); + if (bpf_obj_get_info_by_fd(fd, &pinned_info, &len)) { + p_err("can't get obj info: %s", strerror(errno)); + goto out_close; + } + + obj_node = malloc(sizeof(*obj_node)); + if (!obj_node) { + p_err("mem alloc failed"); + err = -1; + goto out_close; + } + + memset(obj_node, 0, sizeof(*obj_node)); + obj_node->id = pinned_info.id; + obj_node->path = strdup(fpath); + hash_add(build_fn_table->table, &obj_node->hash, obj_node->id); + +out_close: + close(fd); +out_ret: + return err; +} + +int build_pinned_obj_table(struct pinned_obj_table *tab, + enum bpf_obj_type type) +{ + struct mntent *mntent = NULL; FILE *mntfile = NULL; - FTSENT *ftse = NULL; - FTS *fts = NULL; - int fd, err; + int flags = FTW_PHYS; + int nopenfd = 16; mntfile = setmntent("/proc/mounts", "r"); if (!mntfile) return -1; + build_fn_table = tab; + build_fn_type = type; + while ((mntent = getmntent(mntfile))) { - char *path[] = { mntent->mnt_dir, NULL }; + char *path = mntent->mnt_dir; if (strncmp(mntent->mnt_type, "bpf", 3) != 0) continue; - - fts = fts_open(path, 0, NULL); - if (!fts) - continue; - - while ((ftse = fts_read(fts))) { - if (!(ftse->fts_info & FTS_F)) - continue; - fd = open_obj_pinned(ftse->fts_path, true); - if (fd < 0) - continue; - - objtype = get_fd_type(fd); - if (objtype != type) { - close(fd); - continue; - } - memset(&pinned_info, 0, sizeof(pinned_info)); - err = bpf_obj_get_info_by_fd(fd, &pinned_info, &len); - if (err) { - close(fd); - continue; - } - - obj_node = malloc(sizeof(*obj_node)); - if (!obj_node) { - close(fd); - fts_close(fts); - fclose(mntfile); - return -1; - } - - memset(obj_node, 0, sizeof(*obj_node)); - obj_node->id = pinned_info.id; - obj_node->path = strdup(ftse->fts_path); - hash_add(tab->table, &obj_node->hash, obj_node->id); - - close(fd); - } - fts_close(fts); + if (nftw(path, do_build_table_cb, nopenfd, flags) == -1) + break; } fclose(mntfile); return 0;
The bpftool sources include code to walk file trees, but use multiple frameworks to do so: nftw and fts. While nftw conforms to POSIX/SUSv3 and is widely available, fts is not conformant and less common, especially on non-glibc systems. The inconsistent framework usage hampers maintenance and portability of bpftool, in particular for embedded systems. Standardize usage by rewriting one fts-based function to use nftw. This change allows building bpftool against musl for OpenWrt. Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> --- tools/bpf/bpftool/common.c | 102 ++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 48 deletions(-)