RE: [PATCH net-next V2 2/3] tools: bpftool: show filenames of pinned objects

[Prashant Bhole]

> From: Quentin Monnet [mailto:quentin.mon...@netronome.com]
> 
> 2017-11-02 16:59 UTC+0900 ~ Prashant Bhole
> 
> > Added support to show filenames of pinned objects.
> >
> > For example:
> >
> 
> […]
> 
> >
> > Signed-off-by: Prashant Bhole 
> > ---
> > v2:
> >  - Dynamically identify bpf-fs moutpoint
> >  - Close files descriptors before returning on error
> >  - Fixed line break for proper output formatting
> >  - Code style: wrapped lines > 80, used reverse christmastree style
> 
> Thanks for those changes!
> 
> >
> >  tools/bpf/bpftool/common.c | 93
> ++
> >  tools/bpf/bpftool/main.c   |  8 
> >  tools/bpf/bpftool/main.h   | 17 +
> >  tools/bpf/bpftool/map.c| 21 +++
> >  tools/bpf/bpftool/prog.c   | 24 
> >  5 files changed, 163 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 4556947709ee..78a16c02c778 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -45,6 +45,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >
> >  #include 
> >
> > @@ -290,3 +292,94 @@ void print_hex_data_json(uint8_t *data, size_t len)
> > jsonw_printf(json_wtr, "\"0x%02hhx\"", data[i]);
> > jsonw_end_array(json_wtr);
> >  }
> > +
> > +int build_pinned_obj_table(struct pinned_obj_table *tab,
> > +  enum bpf_obj_type type)
> > +{
> > +   struct bpf_prog_info pinned_info = {};
> > +   __u32 len = sizeof(pinned_info);
> > +   struct pinned_obj *obj_node = NULL;
> > +   enum bpf_obj_type objtype;
> > +   struct mntent *mntent = NULL;
> > +   FILE *mntfile = NULL;
> > +   char *bpf_dir = NULL;
> > +   FTSENT *ftse = NULL;
> > +   FTS *ftsp = NULL;
> > +   int fd, err;
> > +
> > +   mntfile = setmntent("/proc/mounts", "r");
> > +   if (!mntfile)
> > +   return -1;
> > +
> > +   while ((mntent = getmntent(mntfile)) != NULL) {
> > +   if (strncmp(mntent->mnt_type, "bpf", 3) == 0) {
> > +   bpf_dir = mntent->mnt_dir;
> > +   break;
> 
> It works well to find a bpf virtual file system, but it stops after the first 
> one it
> finds, although it is possible to have several bpffs on the system. Since you
> already have all the logics, could you move the
> fts_read() step inside this loop, so as to browse all existing bpffs instead 
> of just
> the first one?
> 

Thanks. Sending V3 soon with this change and other coding style fixes.

Prashant

Re: [PATCH net-next V2 2/3] tools: bpftool: show filenames of pinned objects

[Quentin Monnet]

2017-11-02 16:59 UTC+0900 ~ Prashant Bhole 
> Added support to show filenames of pinned objects.
> 
> For example:
> 

[…]

> 
> Signed-off-by: Prashant Bhole 
> ---
> v2:
>  - Dynamically identify bpf-fs moutpoint
>  - Close files descriptors before returning on error
>  - Fixed line break for proper output formatting
>  - Code style: wrapped lines > 80, used reverse christmastree style

Thanks for those changes!

> 
>  tools/bpf/bpftool/common.c | 93 
> ++
>  tools/bpf/bpftool/main.c   |  8 
>  tools/bpf/bpftool/main.h   | 17 +
>  tools/bpf/bpftool/map.c| 21 +++
>  tools/bpf/bpftool/prog.c   | 24 
>  5 files changed, 163 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 4556947709ee..78a16c02c778 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -45,6 +45,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #include 
>  
> @@ -290,3 +292,94 @@ void print_hex_data_json(uint8_t *data, size_t len)
>   jsonw_printf(json_wtr, "\"0x%02hhx\"", data[i]);
>   jsonw_end_array(json_wtr);
>  }
> +
> +int build_pinned_obj_table(struct pinned_obj_table *tab,
> +enum bpf_obj_type type)
> +{
> + struct bpf_prog_info pinned_info = {};
> + __u32 len = sizeof(pinned_info);
> + struct pinned_obj *obj_node = NULL;
> + enum bpf_obj_type objtype;
> + struct mntent *mntent = NULL;
> + FILE *mntfile = NULL;
> + char *bpf_dir = NULL;
> + FTSENT *ftse = NULL;
> + FTS *ftsp = NULL;
> + int fd, err;
> +
> + mntfile = setmntent("/proc/mounts", "r");
> + if (!mntfile)
> + return -1;
> +
> + while ((mntent = getmntent(mntfile)) != NULL) {
> + if (strncmp(mntent->mnt_type, "bpf", 3) == 0) {
> + bpf_dir = mntent->mnt_dir;
> + break;

It works well to find a bpf virtual file system, but it stops after the
first one it finds, although it is possible to have several bpffs on the
system. Since you already have all the logics, could you move the
fts_read() step inside this loop, so as to browse all existing bpffs
instead of just the first one?

> + }
> + }
> + fclose(mntfile);
> +
> + if (bpf_dir) {
> + char *path[] = {bpf_dir, 0};
> +
> + ftsp = fts_open(path, 0, NULL);
> + if (!ftsp)
> + return -1;
> + } else {
> + return -1;
> + }
> +
> +

Nitpicking again: this time, you have too many blank lines here :-)

> + while ((ftse = fts_read(ftsp)) != NULL) {
> + if (!(ftse->fts_info & FTS_F))
> + continue;
> + fd = open_obj_pinned(ftse->fts_path);
> + if (fd < 0)
> + continue;
> +
> + objtype = get_fd_type(fd);
> + if (objtype != type) {
> + close(fd);
> + continue;
> + }
> + memset(_info, 0, sizeof(pinned_info));
> + err = bpf_obj_get_info_by_fd(fd, _info, );
> + if (err) {
> + close(fd);
> + continue;
> + }
> +
> + obj_node = malloc(sizeof(*obj_node));
> + if (!obj_node) {
> + close(fd);
> + fts_close(ftsp);
> + 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, _node->hash, obj_node->id);
> +
> + close(fd);
> + }
> +
> + fts_close(ftsp);
> + return 0;
> +}
> +
> +void delete_pinned_obj_table(struct pinned_obj_table *tab)
> +{
> + struct pinned_obj *obj;
> + struct hlist_node *tmp;
> + unsigned int bkt;
> +
> + if (hash_empty(tab->table))
> + return;
> +
> + hash_for_each_safe(tab->table, bkt, tmp, obj, hash) {
> + hash_del(>hash);
> + free(obj->path);
> + free(obj);
> + }
> +}
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 78d9afb74ef4..6ad53f1797fa 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -54,6 +54,8 @@ static int (*last_do_help)(int argc, char **argv);
>  json_writer_t *json_wtr;
>  bool pretty_output;
>  bool json_output;
> +struct pinned_obj_table prog_table;
> +struct pinned_obj_table map_table;
>  
>  void usage(void)
>  {
> @@ -272,6 +274,9 @@ int main(int argc, char **argv)
>   json_output = false;
>   bin_name = argv[0];
>  
> + hash_init(prog_table.table);
> + hash_init(map_table.table);
> +
>   while ((opt = getopt_long(argc, argv, "Vhpj",
>