* [PATCH bpf-next v2] bpftool: use only nftw for file tree parsing
[not found] <20200715051214.28099-1-Tony.Ambardar () gmail ! com>
@ 2020-07-16 5:29 ` Tony Ambardar
2020-07-16 9:14 ` Quentin Monnet
0 siblings, 1 reply; 3+ messages in thread
From: Tony Ambardar @ 2020-07-16 5:29 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Tony Ambardar, netdev, bpf, Quentin Monnet
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 code usage by rewriting one fts-based function to use nftw.
Clean up related function warnings by using "const char *" arguments and
fixing an unsafe call to dirname().
These changes help in building bpftool against musl for OpenWrt.
Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
---
V2:
* use _GNU_SOURCE to pull in getpagesize(), getline(), nftw() definitions
* use "const char *" in open_obj_pinned() and open_obj_pinned_any()
* make dirname() safely act on a string copy
---
tools/bpf/bpftool/common.c | 129 +++++++++++++++++++++----------------
tools/bpf/bpftool/main.h | 4 +-
2 files changed, 76 insertions(+), 57 deletions(-)
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 29f4e7611ae8..7c2e52fc5784 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -1,10 +1,11 @@
// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
/* Copyright (C) 2017-2018 Netronome Systems, Inc. */
+#define _GNU_SOURCE
#include <ctype.h>
#include <errno.h>
#include <fcntl.h>
-#include <fts.h>
+#include <ftw.h>
#include <libgen.h>
#include <mntent.h>
#include <stdbool.h>
@@ -160,24 +161,36 @@ int mount_tracefs(const char *target)
return err;
}
-int open_obj_pinned(char *path, bool quiet)
+int open_obj_pinned(const char *path, bool quiet)
{
- int fd;
+ char *pname;
+ int fd = -1;
+
+ pname = strdup(path);
+ if (pname == NULL) {
+ if (!quiet)
+ p_err("bpf obj get (%s): %s", path, strerror(errno));
+ goto out_ret;
+ }
+
- fd = bpf_obj_get(path);
+ fd = bpf_obj_get(pname);
if (fd < 0) {
if (!quiet)
- p_err("bpf obj get (%s): %s", path,
- errno == EACCES && !is_bpffs(dirname(path)) ?
+ p_err("bpf obj get (%s): %s", pname,
+ errno == EACCES && !is_bpffs(dirname(pname)) ?
"directory not in bpf file system (bpffs)" :
strerror(errno));
- return -1;
+ goto out_free;
}
+out_free:
+ free(pname);
+out_ret:
return fd;
}
-int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
+int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type)
{
enum bpf_obj_type type;
int fd;
@@ -367,68 +380,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;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 78d34e860713..e3a79b5a9960 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -152,8 +152,8 @@ int cmd_select(const struct cmd *cmds, int argc, char **argv,
int get_fd_type(int fd);
const char *get_fd_type_name(enum bpf_obj_type type);
char *get_fdinfo(int fd, const char *key);
-int open_obj_pinned(char *path, bool quiet);
-int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type);
+int open_obj_pinned(const char *path, bool quiet);
+int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type);
int mount_bpffs_for_pin(const char *name);
int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(int *, char ***));
int do_pin_fd(int fd, const char *name);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: use only nftw for file tree parsing
2020-07-16 5:29 ` [PATCH bpf-next v2] bpftool: use only nftw for file tree parsing Tony Ambardar
@ 2020-07-16 9:14 ` Quentin Monnet
2020-07-17 22:43 ` Tony Ambardar
0 siblings, 1 reply; 3+ messages in thread
From: Quentin Monnet @ 2020-07-16 9:14 UTC (permalink / raw)
To: Tony Ambardar, Alexei Starovoitov, Daniel Borkmann; +Cc: netdev, bpf
2020-07-15 22:29 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 code usage by rewriting one fts-based function to use nftw.
> Clean up related function warnings by using "const char *" arguments and
> fixing an unsafe call to dirname().
>
> These changes help in building bpftool against musl for OpenWrt.
Could you please add a line to the log about the reason for the path
copy in open_obj_pinned()?
>
> Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
> ---
>
> V2:
> * use _GNU_SOURCE to pull in getpagesize(), getline(), nftw() definitions
> * use "const char *" in open_obj_pinned() and open_obj_pinned_any()
> * make dirname() safely act on a string copy
>
> ---
> tools/bpf/bpftool/common.c | 129 +++++++++++++++++++++----------------
> tools/bpf/bpftool/main.h | 4 +-
> 2 files changed, 76 insertions(+), 57 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 29f4e7611ae8..7c2e52fc5784 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -1,10 +1,11 @@
> // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> /* Copyright (C) 2017-2018 Netronome Systems, Inc. */
>
> +#define _GNU_SOURCE
> #include <ctype.h>
> #include <errno.h>
> #include <fcntl.h>
> -#include <fts.h>
> +#include <ftw.h>
> #include <libgen.h>
> #include <mntent.h>
> #include <stdbool.h>
> @@ -160,24 +161,36 @@ int mount_tracefs(const char *target)
> return err;
> }
>
> -int open_obj_pinned(char *path, bool quiet)
> +int open_obj_pinned(const char *path, bool quiet)
> {
> - int fd;
> + char *pname;
> + int fd = -1;
> +
> + pname = strdup(path);
> + if (pname == NULL) {
Simply "if (!pname) {"
> + if (!quiet)
> + p_err("bpf obj get (%s): %s", path, strerror(errno));
Please update the error message, this one was for a failure on
bpf_obj_get().
> + goto out_ret;
> + }
> +
You're adding a second blank line here, please fix.
>
> - fd = bpf_obj_get(path);
> + fd = bpf_obj_get(pname);
> if (fd < 0) {
> if (!quiet)
> - p_err("bpf obj get (%s): %s", path,
> - errno == EACCES && !is_bpffs(dirname(path)) ?
> + p_err("bpf obj get (%s): %s", pname,
> + errno == EACCES && !is_bpffs(dirname(pname)) ?
> "directory not in bpf file system (bpffs)" :
> strerror(errno));
> - return -1;
> + goto out_free;
> }
>
> +out_free:
> + free(pname);
> +out_ret:
> return fd;
> }
>
> -int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
> +int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type)
> {
> enum bpf_obj_type type;
> int fd;
> @@ -367,68 +380,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;
I would move the comments above the lines, since it applies to both of them.
> +
> +static int do_build_table_cb(const char *fpath, const struct stat *sb,
> + int typeflag, struct FTW *ftwbuf)
Please align the second line on the open parenthesis.
> {
> struct bpf_prog_info pinned_info = {};
A few suggestions on this code, even though I realise you simply moved
some parts. We can skip zero-initialising here (" = {}") because we
memset() it below before using it anyway.
> 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));
We are simply building a table here to show the paths where objects are
pinned when listing progs/maps/etc., and I don't believe we want to
print an error in that case. And with such a message I would expect the
function to return and bpftool to stop, but again I don't believe this
is necessary here and we can just go on listing objects, even if we fail
to show their pinned paths.
> + goto out_close;
> + }
> +
> + obj_node = malloc(sizeof(*obj_node));
> + if (!obj_node) {
> + p_err("mem alloc failed");
Same here, let's not add an error message.
> + err = -1;
> + goto out_close;
> + }
> +
> + memset(obj_node, 0, sizeof(*obj_node));
Instead of malloc() + memset(), we could simply use calloc().
> + 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)
> +{
The end looks good.
Apart from the minor points mentioned above, the patch is good, copying
the string in open_obj_pinned() looks like the right approach. It does
compile (with no warnings) and works as intended on my machine :).
Thanks,
Quentin
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] bpftool: use only nftw for file tree parsing
2020-07-16 9:14 ` Quentin Monnet
@ 2020-07-17 22:43 ` Tony Ambardar
0 siblings, 0 replies; 3+ messages in thread
From: Tony Ambardar @ 2020-07-17 22:43 UTC (permalink / raw)
To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf
On Thu, 16 Jul 2020 at 02:14, Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2020-07-15 22:29 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 code usage by rewriting one fts-based function to use nftw.
> > Clean up related function warnings by using "const char *" arguments and
> > fixing an unsafe call to dirname().
> >
> > These changes help in building bpftool against musl for OpenWrt.
>
> Could you please add a line to the log about the reason for the path
> copy in open_obj_pinned()?
Good point, will do.
> >
> > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
> > ---
> >
> > V2:
> > * use _GNU_SOURCE to pull in getpagesize(), getline(), nftw() definitions
> > * use "const char *" in open_obj_pinned() and open_obj_pinned_any()
> > * make dirname() safely act on a string copy
> >
> > ---
> > tools/bpf/bpftool/common.c | 129 +++++++++++++++++++++----------------
> > tools/bpf/bpftool/main.h | 4 +-
> > 2 files changed, 76 insertions(+), 57 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 29f4e7611ae8..7c2e52fc5784 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -1,10 +1,11 @@
> > // SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > /* Copyright (C) 2017-2018 Netronome Systems, Inc. */
> >
> > +#define _GNU_SOURCE
> > #include <ctype.h>
> > #include <errno.h>
> > #include <fcntl.h>
> > -#include <fts.h>
> > +#include <ftw.h>
> > #include <libgen.h>
> > #include <mntent.h>
> > #include <stdbool.h>
> > @@ -160,24 +161,36 @@ int mount_tracefs(const char *target)
> > return err;
> > }
> >
> > -int open_obj_pinned(char *path, bool quiet)
> > +int open_obj_pinned(const char *path, bool quiet)
> > {
> > - int fd;
> > + char *pname;
> > + int fd = -1;
> > +
> > + pname = strdup(path);
> > + if (pname == NULL) {
>
> Simply "if (!pname) {"
>
> > + if (!quiet)
> > + p_err("bpf obj get (%s): %s", path, strerror(errno));
>
> Please update the error message, this one was for a failure on
> bpf_obj_get().
>
> > + goto out_ret;
> > + }
> > +
>
> You're adding a second blank line here, please fix.
>
> >
> > - fd = bpf_obj_get(path);
> > + fd = bpf_obj_get(pname);
> > if (fd < 0) {
> > if (!quiet)
> > - p_err("bpf obj get (%s): %s", path,
> > - errno == EACCES && !is_bpffs(dirname(path)) ?
> > + p_err("bpf obj get (%s): %s", pname,
> > + errno == EACCES && !is_bpffs(dirname(pname)) ?
> > "directory not in bpf file system (bpffs)" :
> > strerror(errno));
> > - return -1;
> > + goto out_free;
> > }
> >
> > +out_free:
> > + free(pname);
> > +out_ret:
> > return fd;
> > }
> >
> > -int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
> > +int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type)
> > {
> > enum bpf_obj_type type;
> > int fd;
> > @@ -367,68 +380,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;
>
> I would move the comments above the lines, since it applies to both of them.
>
> > +
> > +static int do_build_table_cb(const char *fpath, const struct stat *sb,
> > + int typeflag, struct FTW *ftwbuf)
>
> Please align the second line on the open parenthesis.
>
> > {
> > struct bpf_prog_info pinned_info = {};
>
> A few suggestions on this code, even though I realise you simply moved
> some parts. We can skip zero-initialising here (" = {}") because we
> memset() it below before using it anyway.
>
> > 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));
>
> We are simply building a table here to show the paths where objects are
> pinned when listing progs/maps/etc., and I don't believe we want to
> print an error in that case. And with such a message I would expect the
> function to return and bpftool to stop, but again I don't believe this
> is necessary here and we can just go on listing objects, even if we fail
> to show their pinned paths.
>
> > + goto out_close;
> > + }
> > +
> > + obj_node = malloc(sizeof(*obj_node));
> > + if (!obj_node) {
> > + p_err("mem alloc failed");
>
> Same here, let's not add an error message.
You're right. I just realized those aren't pre-existing messages; I
added them but then forgot.
> > + err = -1;
> > + goto out_close;
> > + }
> > +
> > + memset(obj_node, 0, sizeof(*obj_node));
>
> Instead of malloc() + memset(), we could simply use calloc().
>
> > + obj_node->id = pinned_info.id;
> > + obj_node->path = strdup(fpath);
I should check for failed allocation here too.
> > + 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)
> > +{
>
> The end looks good.
>
> Apart from the minor points mentioned above, the patch is good, copying
> the string in open_obj_pinned() looks like the right approach. It does
> compile (with no warnings) and works as intended on my machine :).
Glad to hear it! I'll update the patch based on your comments above
and resend shortly. Thanks again for the testing and feedback.
Regards,
Tony
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-17 22:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200715051214.28099-1-Tony.Ambardar () gmail ! com>
2020-07-16 5:29 ` [PATCH bpf-next v2] bpftool: use only nftw for file tree parsing Tony Ambardar
2020-07-16 9:14 ` Quentin Monnet
2020-07-17 22:43 ` Tony Ambardar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.