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

[Jakub Kicinski]

On Wed, 8 Nov 2017 11:34:44 +0900, Prashant Bhole wrote:
> > > + FILE *mntfile = NULL;
> > > + FTSENT *ftse = NULL;
> > > + FTS *fts = NULL;
> > > + int fd, err;
> > > +
> > > + mntfile = setmntent("/proc/mounts", "r");
> > > + if (!mntfile)
> > > + return -1;
> > > +
> > > + while ((mntent = getmntent(mntfile)) != NULL) {  
> > 
> > Please try to avoid comparisons to NULL, writing:
> > 
> > if (ptr)
> > 
> > is more intuitive to most C programmers than:
> > 
> > if (ptr != NULL)  
> 
> Jakub,
> Thank you for comments. I agree with using 'if (ptr)' for simple check, but
> here it is an assignment.
> It doesn't look intuitive and looks like a typo if I change it to:
>  while ((mntent = getmntent(mntfile))) {

I still prefer this, if you don't mind.

> >   
> > > + char *path[] = {mntent->mnt_dir, 0};  
> > 
> > Shouldn't there be spaces after and before the curly braces?  Does  
> checkpatch --
> > strict not warn about this?  
> 
> I will add spaces. checkpatch complained about this not being static const,
> but doing so causes compiler warning because it doesn't match with signature
> of fts_open()

Right, that's OK to ignore.  Could you also make the 0 into a NULL,
though?

> I will submit v4 with changes for other comments if you are ok with replies
> above.

Thank you!

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

[Prashant Bhole]

> -Original Message-
> From: Jakub Kicinski [mailto:jakub.kicin...@netronome.com]
> 
> On Mon,  6 Nov 2017 15:06:31 +0900, Prashant Bhole wrote:
> > Added support to show filenames of pinned objects.
> > ...
> > Signed-off-by: Prashant Bhole 
> 
> Thanks for the changes, a couple more nit picks, sorry for not spotting
them
> earlier.
> 
> > 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
> >
> > v3:
> >  - Handle multiple bpffs mountpoints
> >  - Code style: fixed line break indentation
> >
> >  tools/bpf/bpftool/common.c | 85
> ++
> >  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, 155 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 4556947709ee..152c5bdbe2e9 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -45,6 +45,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> 
> Please try to keep includes in an alphabetical order.
> 
> >  #include 
> >
> > @@ -290,3 +292,86 @@ 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;
> 
> Please try to order variable declarations longest to shortest.
> 
> > +   FILE *mntfile = NULL;
> > +   FTSENT *ftse = NULL;
> > +   FTS *fts = NULL;
> > +   int fd, err;
> > +
> > +   mntfile = setmntent("/proc/mounts", "r");
> > +   if (!mntfile)
> > +   return -1;
> > +
> > +   while ((mntent = getmntent(mntfile)) != NULL) {
> 
> Please try to avoid comparisons to NULL, writing:
> 
>   if (ptr)
> 
> is more intuitive to most C programmers than:
> 
>   if (ptr != NULL)

Jakub,
Thank you for comments. I agree with using 'if (ptr)' for simple check, but
here it is an assignment.
It doesn't look intuitive and looks like a typo if I change it to:
 while ((mntent = getmntent(mntfile))) {

> 
> > +   char *path[] = {mntent->mnt_dir, 0};
> 
> Shouldn't there be spaces after and before the curly braces?  Does
checkpatch --
> strict not warn about this?

I will add spaces. checkpatch complained about this not being static const,
but doing so causes compiler warning because it doesn't match with signature
of fts_open()

I will submit v4 with changes for other comments if you are ok with replies
above.

Prashant

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

[Jakub Kicinski]

On Mon,  6 Nov 2017 15:06:31 +0900, Prashant Bhole wrote:
> Added support to show filenames of pinned objects.
> ...
> Signed-off-by: Prashant Bhole 

Thanks for the changes, a couple more nit picks, sorry for not spotting
them earlier.

> 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
> 
> v3:
>  - Handle multiple bpffs mountpoints
>  - Code style: fixed line break indentation
> 
>  tools/bpf/bpftool/common.c | 85 
> ++
>  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, 155 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 4556947709ee..152c5bdbe2e9 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -45,6 +45,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 

Please try to keep includes in an alphabetical order.

>  #include 
>  
> @@ -290,3 +292,86 @@ 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;

Please try to order variable declarations longest to shortest.

> + FILE *mntfile = NULL;
> + FTSENT *ftse = NULL;
> + FTS *fts = NULL;
> + int fd, err;
> +
> + mntfile = setmntent("/proc/mounts", "r");
> + if (!mntfile)
> + return -1;
> +
> + while ((mntent = getmntent(mntfile)) != NULL) {

Please try to avoid comparisons to NULL, writing:

if (ptr)

is more intuitive to most C programmers than:

if (ptr != NULL)

> + char *path[] = {mntent->mnt_dir, 0};

Shouldn't there be spaces after and before the curly braces?  Does
checkpatch --strict not warn about this?

> +
> + if (strncmp(mntent->mnt_type, "bpf", 3) != 0)
> + continue;
> +
> + fts = fts_open(path, 0, NULL);
> + if (!fts)
> + continue;
> +
> + while ((ftse = fts_read(fts)) != 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(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, _node->hash, obj_node->id);
> +
> + close(fd);
> + }
> + fts_close(fts);
> + }
> + fclose(mntfile);
> + 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;

is this necessary?  Does hash_for_each_safe() not work with empty table?

> + hash_for_each_safe(tab->table, bkt, tmp, obj, hash) {
> + hash_del(>hash);
> + free(obj->path);
> + free(obj);
> + }
> +}

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

[Prashant Bhole]

Added support to show filenames of pinned objects.

For example:

root@test# ./bpftool prog
3: tracepoint  name tracepoint__irq  tag f677a7dd722299a3
loaded_at Oct 26/11:39  uid 0
xlated 160B  not jited  memlock 4096B  map_ids 4
pinned /sys/fs/bpf/softirq_prog

4: tracepoint  name tracepoint__irq  tag ea5dc530d00b92b6
loaded_at Oct 26/11:39  uid 0
xlated 392B  not jited  memlock 4096B  map_ids 4,6

root@test# ./bpftool --json --pretty prog
[{
"id": 3,
"type": "tracepoint",
"name": "tracepoint__irq",
"tag": "f677a7dd722299a3",
"loaded_at": "Oct 26/11:39",
"uid": 0,
"bytes_xlated": 160,
"jited": false,
"bytes_memlock": 4096,
"map_ids": [4
],
"pinned": ["/sys/fs/bpf/softirq_prog"
]
},{
"id": 4,
"type": "tracepoint",
"name": "tracepoint__irq",
"tag": "ea5dc530d00b92b6",
"loaded_at": "Oct 26/11:39",
"uid": 0,
"bytes_xlated": 392,
"jited": false,
"bytes_memlock": 4096,
"map_ids": [4,6
],
"pinned": []
}
]

root@test# ./bpftool map
4: hash  name start  flags 0x0
key 4B  value 16B  max_entries 10240  memlock 1003520B
pinned /sys/fs/bpf/softirq_map1
5: hash  name iptr  flags 0x0
key 4B  value 8B  max_entries 10240  memlock 921600B

root@test# ./bpftool --json --pretty map
[{
"id": 4,
"type": "hash",
"name": "start",
"flags": 0,
"bytes_key": 4,
"bytes_value": 16,
"max_entries": 10240,
"bytes_memlock": 1003520,
"pinned": ["/sys/fs/bpf/softirq_map1"
]
},{
"id": 5,
"type": "hash",
"name": "iptr",
"flags": 0,
"bytes_key": 4,
"bytes_value": 8,
"max_entries": 10240,
"bytes_memlock": 921600,
"pinned": []
}
]

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

v3:
 - Handle multiple bpffs mountpoints
 - Code style: fixed line break indentation

 tools/bpf/bpftool/common.c | 85 ++
 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, 155 insertions(+)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 4556947709ee..152c5bdbe2e9 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,86 @@ 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;
+   FTSENT *ftse = NULL;
+   FTS *fts = NULL;
+   int fd, err;
+
+   mntfile = setmntent("/proc/mounts", "r");
+   if (!mntfile)
+   return -1;
+
+   while ((mntent = getmntent(mntfile)) != NULL) {
+   char *path[] = {mntent->mnt_dir, 0};
+
+   if (strncmp(mntent->mnt_type, "bpf", 3) != 0)
+   continue;
+
+   fts = fts_open(path, 0, NULL);
+   if (!fts)
+   continue;
+
+   while ((ftse = fts_read(fts)) != 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(fts);
+   fclose(mntfile);
+   return -1;
+   }
+
+