> -----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 <bhole_prashant...@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
