[PATCH bpf-next] tools: bpftool: do not pass json_wtr to emit_obj_refs_json()

[PATCH bpf-next] tools: bpftool: do not pass json_wtr to emit_obj_refs_json()

[Quentin Monnet]

Building bpftool yields the following complaint:

    pids.c: In function ‘emit_obj_refs_json’:
    pids.c:175:80: warning: declaration of ‘json_wtr’ shadows a global declaration [-Wshadow]
      175 | void emit_obj_refs_json(struct obj_refs_table *table, __u32 id, json_writer_t *json_wtr)
          |                                                                 ~~~~~~~~~~~~~~~^~~~~~~~
    In file included from pids.c:11:
    main.h:141:23: note: shadowed declaration is here
      141 | extern json_writer_t *json_wtr;
          |                       ^~~~~~~~

json_wtr being exposed in main.h (included in pids.c) as an extern, it
is directly available and there is no need to pass it through the
function. Let's simply use the global variable.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 tools/bpf/bpftool/btf.c  | 2 +-
 tools/bpf/bpftool/link.c | 2 +-
 tools/bpf/bpftool/main.h | 3 +--
 tools/bpf/bpftool/map.c  | 2 +-
 tools/bpf/bpftool/pids.c | 2 +-
 tools/bpf/bpftool/prog.c | 2 +-
 6 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index fc9bc7a23db6..81a1c301ccf4 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -843,7 +843,7 @@ show_btf_json(struct bpf_btf_info *info, int fd,
 	}
 	jsonw_end_array(json_wtr);	/* map_ids */
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr); /* pids */
+	emit_obj_refs_json(&refs_table, info->id); /* pids */
 
 	jsonw_end_object(json_wtr);	/* btf object */
 }
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index 7329f3134283..ac39b1f80838 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -144,7 +144,7 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
 		jsonw_end_array(json_wtr);
 	}
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr);
+	emit_obj_refs_json(&refs_table, info->id);
 
 	jsonw_end_object(json_wtr);
 
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index ce26271e5f0c..ad5a67bf6cf1 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -197,8 +197,7 @@ void delete_pinned_obj_table(struct pinned_obj_table *tab);
 __weak int build_obj_refs_table(struct obj_refs_table *table,
 				enum bpf_obj_type type);
 __weak void delete_obj_refs_table(struct obj_refs_table *table);
-__weak void emit_obj_refs_json(struct obj_refs_table *table, __u32 id,
-			       json_writer_t *json_wtr);
+__weak void emit_obj_refs_json(struct obj_refs_table *table, __u32 id);
 __weak void emit_obj_refs_plain(struct obj_refs_table *table, __u32 id,
 				const char *prefix);
 void print_dev_plain(__u32 ifindex, __u64 ns_dev, __u64 ns_inode);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 0a6a5d82d380..0f88f1de1500 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -509,7 +509,7 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
 		jsonw_end_array(json_wtr);
 	}
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr);
+	emit_obj_refs_json(&refs_table, info->id);
 
 	jsonw_end_object(json_wtr);
 
diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
index 3474a91743ff..d5dc55641230 100644
--- a/tools/bpf/bpftool/pids.c
+++ b/tools/bpf/bpftool/pids.c
@@ -172,7 +172,7 @@ void delete_obj_refs_table(struct obj_refs_table *table)
 	}
 }
 
-void emit_obj_refs_json(struct obj_refs_table *table, __u32 id, json_writer_t *json_wtr)
+void emit_obj_refs_json(struct obj_refs_table *table, __u32 id)
 {
 	struct obj_refs *refs;
 	struct obj_ref *ref;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index e21fa8ad2efa..0095fb3faa17 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -190,7 +190,7 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
 		jsonw_end_array(json_wtr);
 	}
 
-	emit_obj_refs_json(&refs_table, info->id, json_wtr);
+	emit_obj_refs_json(&refs_table, info->id);
 
 	jsonw_end_object(json_wtr);
 }
-- 
2.25.1

Re: [PATCH bpf-next] tools: bpftool: do not pass json_wtr to emit_obj_refs_json()

[Andrii Nakryiko]

On Tue, Jun 23, 2020 at 8:54 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Building bpftool yields the following complaint:
>
>     pids.c: In function ‘emit_obj_refs_json’:
>     pids.c:175:80: warning: declaration of ‘json_wtr’ shadows a global declaration [-Wshadow]
>       175 | void emit_obj_refs_json(struct obj_refs_table *table, __u32 id, json_writer_t *json_wtr)
>           |                                                                 ~~~~~~~~~~~~~~~^~~~~~~~
>     In file included from pids.c:11:
>     main.h:141:23: note: shadowed declaration is here
>       141 | extern json_writer_t *json_wtr;
>           |                       ^~~~~~~~
>
> json_wtr being exposed in main.h (included in pids.c) as an extern, it
> is directly available and there is no need to pass it through the
> function. Let's simply use the global variable.

I don't think it's a good approach to assume that emit_obj_refs_json
is always going to be using a global json writer. I think this shadow
warning is bogus in this case, honestly. But if it bothers you, let's
just rename json_wtr into whatever other name of argument you prefer.

>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>  tools/bpf/bpftool/btf.c  | 2 +-
>  tools/bpf/bpftool/link.c | 2 +-
>  tools/bpf/bpftool/main.h | 3 +--
>  tools/bpf/bpftool/map.c  | 2 +-
>  tools/bpf/bpftool/pids.c | 2 +-
>  tools/bpf/bpftool/prog.c | 2 +-
>  6 files changed, 6 insertions(+), 7 deletions(-)
>

[...]

Re: [PATCH bpf-next] tools: bpftool: do not pass json_wtr to emit_obj_refs_json()

[Quentin Monnet]

On Tue, 23 Jun 2020 at 19:27, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jun 23, 2020 at 8:54 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >
> > Building bpftool yields the following complaint:
> >
> >     pids.c: In function ‘emit_obj_refs_json’:
> >     pids.c:175:80: warning: declaration of ‘json_wtr’ shadows a global declaration [-Wshadow]
> >       175 | void emit_obj_refs_json(struct obj_refs_table *table, __u32 id, json_writer_t *json_wtr)
> >           |                                                                 ~~~~~~~~~~~~~~~^~~~~~~~
> >     In file included from pids.c:11:
> >     main.h:141:23: note: shadowed declaration is here
> >       141 | extern json_writer_t *json_wtr;
> >           |                       ^~~~~~~~
> >
> > json_wtr being exposed in main.h (included in pids.c) as an extern, it
> > is directly available and there is no need to pass it through the
> > function. Let's simply use the global variable.
>
> I don't think it's a good approach to assume that emit_obj_refs_json
> is always going to be using a global json writer. I think this shadow
> warning is bogus in this case, honestly. But if it bothers you, let's
> just rename json_wtr into whatever other name of argument you prefer.

I didn't mind using the global JSON writer, but I'm not strictly opposed to
passing a writer down to emit_obj_refs_json() either, so ok. I'll
still respin to
rename the variable, it will be clearer that the function does not rely on the
global writer.

Thanks for the feedback.
Quentin