netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] bpftool: use only nftw for file tree parsing
@ 2020-07-15  5:12 Tony Ambardar
  2020-07-15 17:35 ` Quentin Monnet
  0 siblings, 1 reply; 3+ messages in thread
From: Tony Ambardar @ 2020-07-15  5:12 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann; +Cc: netdev, bpf, Tony Ambardar

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(-)

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;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next] bpftool: use only nftw for file tree parsing
  2020-07-15  5:12 [PATCH bpf-next] bpftool: use only nftw for file tree parsing Tony Ambardar
@ 2020-07-15 17:35 ` Quentin Monnet
  2020-07-16  3:02   ` Tony Ambardar
  0 siblings, 1 reply; 3+ messages in thread
From: Quentin Monnet @ 2020-07-15 17:35 UTC (permalink / raw)
  To: Tony Ambardar, Alexei Starovoitov, Daniel Borkmann; +Cc: netdev, bpf

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next] bpftool: use only nftw for file tree parsing
  2020-07-15 17:35 ` Quentin Monnet
@ 2020-07-16  3:02   ` Tony Ambardar
  0 siblings, 0 replies; 3+ messages in thread
From: Tony Ambardar @ 2020-07-16  3:02 UTC (permalink / raw)
  To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-07-16  3:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15  5:12 [PATCH bpf-next] bpftool: use only nftw for file tree parsing Tony Ambardar
2020-07-15 17:35 ` Quentin Monnet
2020-07-16  3:02   ` Tony Ambardar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).