From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Prashant Bhole" Subject: RE: [PATCH net-next V3 2/3] tools: bpftool: show filenames of pinned objects Date: Wed, 8 Nov 2017 11:34:44 +0900 Message-ID: <00d801d3583a$23219ff0$6964dfd0$@lab.ntt.co.jp> References: <20171106060632.3380-1-bhole_prashant_q7@lab.ntt.co.jp> <20171106060632.3380-3-bhole_prashant_q7@lab.ntt.co.jp> <20171107043656.769e4124@cakuba> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: "'David S . Miller'" , , "'Alexei Starovoitov'" , "'Daniel Borkmann'" , "'Quentin Monnet'" To: "'Jakub Kicinski'" Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:55748 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752062AbdKHCgC (ORCPT ); Tue, 7 Nov 2017 21:36:02 -0500 In-Reply-To: <20171107043656.769e4124@cakuba> Content-Language: en-us Sender: netdev-owner@vger.kernel.org List-ID: > -----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 > > 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