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

["Prashant Bhole" <bhole_prashant_q7@lab.ntt.co.jp>]

> -----Original Message-----
> From: Jakub Kicinski [mailto:jakub.kicinski@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 <bhole_prashant_q7@lab.ntt.co.jp>
> 
> 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 <sys/mount.h>
> >  #include <sys/types.h>
> >  #include <sys/vfs.h>
> > +#include <mntent.h>
> > +#include <fts.h>
> 
> Please try to keep includes in an alphabetical order.
> 
> >  #include <bpf.h>
> >
> > @@ -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