[PATCH net 0/6] tools: bpftool: fix a minor issues with JSON and Makefiles

[PATCH net 0/6] tools: bpftool: fix a minor issues with JSON and Makefiles

[Jakub Kicinski]

Quentin says:

First commit in this series fixes a crash that occurs when incorrect
arguments are passed to bpftool after the `--json` option. It comes from
the usage() function trying to use the JSON writer, although the latter
has not been created yet at that point.

Other patches add destruction of the writer in case the program exits in
usage(), fix error messages handling when an unrecognized option is
encountered, remove a spurious new-line character in an error message.

Last patches are related to the Makefiles. They fix the installation
directory prefix and .PHONY targets.

Quentin Monnet (6):
  tools: bpftool: fix crash on bad parameters with JSON
  tools: bpftool: clean up the JSON writer before exiting in usage()
  tools: bpftool: make error message from getopt_long() JSON-friendly
  tools: bpftool: remove spurious line break from error message
  tools: bpftool: change installation directory
  tools: bpftool: declare phony targets as such

 tools/bpf/bpftool/Documentation/Makefile |  2 +-
 tools/bpf/bpftool/Makefile               |  7 ++++---
 tools/bpf/bpftool/main.c                 | 36 +++++++++++++++++++++-----------
 tools/bpf/bpftool/main.h                 |  5 +++--
 4 files changed, 32 insertions(+), 18 deletions(-)

-- 
2.14.1

[PATCH net 1/6] tools: bpftool: fix crash on bad parameters with JSON

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

If bad or unrecognised parameters are specified after JSON output is
requested, `usage()` will try to output null JSON object before the
writer is created.

To prevent this, create the writer as soon as the `--json` option is
parsed.

Fixes: 004b45c0e51a ("tools: bpftool: provide JSON output for all possible commands")
Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/main.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index d6e4762170a4..14ad54a1c404 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -291,7 +291,15 @@ int main(int argc, char **argv)
 			pretty_output = true;
 			/* fall through */
 		case 'j':
-			json_output = true;
+			if (!json_output) {
+				json_wtr = jsonw_new(stdout);
+				if (!json_wtr) {
+					p_err("failed to create JSON writer");
+					return -1;
+				}
+				json_output = true;
+			}
+			jsonw_pretty(json_wtr, pretty_output);
 			break;
 		case 'f':
 			show_pinned = true;
@@ -306,15 +314,6 @@ int main(int argc, char **argv)
 	if (argc < 0)
 		usage();
 
-	if (json_output) {
-		json_wtr = jsonw_new(stdout);
-		if (!json_wtr) {
-			p_err("failed to create JSON writer");
-			return -1;
-		}
-		jsonw_pretty(json_wtr, pretty_output);
-	}
-
 	bfd_init();
 
 	ret = cmd_select(cmds, argc, argv, do_help);
-- 
2.14.1

[PATCH net 2/6] tools: bpftool: clean up the JSON writer before exiting in usage()

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

The writer is cleaned at the end of the main function, but not if the
program exits sooner in usage(). Let's keep it clean and destroy the
writer before exiting.

Destruction and actual call to exit() are moved to another function so
that clean exit can also be performed without printing usage() hints.

Fixes: d35efba99d92 ("tools: bpftool: introduce --json and --pretty options")
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/main.c | 10 +++++++++-
 tools/bpf/bpftool/main.h |  3 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 14ad54a1c404..d72dd73a4016 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -58,11 +58,19 @@ bool show_pinned;
 struct pinned_obj_table prog_table;
 struct pinned_obj_table map_table;
 
+static void __noreturn clean_and_exit(int i)
+{
+	if (json_output)
+		jsonw_destroy(&json_wtr);
+
+	exit(i);
+}
+
 void usage(void)
 {
 	last_do_help(last_argc - 1, last_argv + 1);
 
-	exit(-1);
+	clean_and_exit(-1);
 }
 
 static int do_help(int argc, char **argv)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 9c191e222d6f..0b60ddfb2b93 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -41,6 +41,7 @@
 #include <stdbool.h>
 #include <stdio.h>
 #include <linux/bpf.h>
+#include <linux/compiler.h>
 #include <linux/kernel.h>
 #include <linux/hashtable.h>
 
@@ -80,7 +81,7 @@ void p_info(const char *fmt, ...);
 
 bool is_prefix(const char *pfx, const char *str);
 void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
-void usage(void) __attribute__((noreturn));
+void usage(void) __noreturn;
 
 struct pinned_obj_table {
 	DECLARE_HASHTABLE(table, 16);
-- 
2.14.1

[PATCH net 3/6] tools: bpftool: make error message from getopt_long() JSON-friendly

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

If `getopt_long()` meets an unknown option, it prints its own error
message to standard error output. While this does not strictly break
JSON output, it is the only case bpftool prints something to standard
error output if JSON output is required. All other errors are printed on
standard output as JSON objects, so that an external program does not
have to parse stderr.

This is changed by setting the global variable `opterr` to 0.
Furthermore, p_err() is used to reproduce the error message in a more
JSON-friendly way, so that users still get to know what the erroneous
option is.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index d72dd73a4016..d294bc8168be 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -288,6 +288,7 @@ int main(int argc, char **argv)
 	hash_init(prog_table.table);
 	hash_init(map_table.table);
 
+	opterr = 0;
 	while ((opt = getopt_long(argc, argv, "Vhpjf",
 				  options, NULL)) >= 0) {
 		switch (opt) {
@@ -313,7 +314,11 @@ int main(int argc, char **argv)
 			show_pinned = true;
 			break;
 		default:
-			usage();
+			p_err("unrecognized option '%s'", argv[optind - 1]);
+			if (json_output)
+				clean_and_exit(-1);
+			else
+				usage();
 		}
 	}
 
-- 
2.14.1

[PATCH net 4/6] tools: bpftool: remove spurious line break from error message

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

The end-of-line character inside the string would break JSON compliance.
Remove it, `p_err()` already adds a '\n' character for plain output
anyway.

Fixes: 9a5ab8bf1d6d ("tools: bpftool: turn err() and info() macros into functions")
Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/main.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 0b60ddfb2b93..bff330b49791 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -51,7 +51,7 @@
 
 #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
 #define NEXT_ARGP()	({ (*argc)--; (*argv)++; if (*argc < 0) usage(); })
-#define BAD_ARG()	({ p_err("what is '%s'?\n", *argv); -1; })
+#define BAD_ARG()	({ p_err("what is '%s'?", *argv); -1; })
 
 #define ERR_MAX_LEN	1024
 
-- 
2.14.1

[PATCH net 5/6] tools: bpftool: unify installation directories

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

Programs and documentation not managed by package manager are generally
installed under /usr/local/, instead of the user's home directory. In
particular, `man` is generally able to find manual pages under
`/usr/local/share/man`.

bpftool generally follows perf's example, and perf installs to home
directory. However bpftool requires root credentials, so it seems
sensible to follow the more common convention of installing files under
/usr/local instead. So, make /usr/local the default prefix for
installing the binary with `make install`, and the documentation with
`make doc-install`. Also, create /usr/local/sbin if it does not exist.

Note that the bash-completion file, however, is still installed under
/usr/share/bash-completion/completions, as the default setup for bash
does not attempt to load completion files under /usr/local/.

Reported-by: David Beckett <david.beckett@netronome.com>
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/Documentation/Makefile | 2 +-
 tools/bpf/bpftool/Makefile               | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/Makefile b/tools/bpf/bpftool/Documentation/Makefile
index bde77d7c4390..37292bb5ce60 100644
--- a/tools/bpf/bpftool/Documentation/Makefile
+++ b/tools/bpf/bpftool/Documentation/Makefile
@@ -6,7 +6,7 @@ RM ?= rm -f
 
 # Make the path relative to DESTDIR, not prefix
 ifndef DESTDIR
-prefix?=$(HOME)
+prefix ?= /usr/local
 endif
 mandir ?= $(prefix)/share/man
 man8dir = $(mandir)/man8
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 813826c50936..c5b21f2cbca5 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -45,8 +45,8 @@ $(LIBBPF): FORCE
 	$(call QUIET_CLEAN, libbpf)
 	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) clean >/dev/null
 
-prefix = /usr
-bash_compdir ?= $(prefix)/share/bash-completion/completions
+prefix = /usr/local
+bash_compdir ?= /usr/share/bash-completion/completions
 
 CC = gcc
 
@@ -76,6 +76,7 @@ clean: $(LIBBPF)-clean
 	$(Q)rm -rf $(OUTPUT)bpftool $(OUTPUT)*.o $(OUTPUT)*.d
 
 install:
+	install -m 0755 -d $(prefix)/sbin
 	install $(OUTPUT)bpftool $(prefix)/sbin/bpftool
 	install -m 0755 -d $(bash_compdir)
 	install -m 0644 bash-completion/bpftool $(bash_compdir)
-- 
2.14.1

[PATCH net 6/6] tools: bpftool: declare phony targets as such

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

In the Makefile, targets install, doc and doc-install should be added to
.PHONY. Let's fix this.

Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index c5b21f2cbca5..ec3052c0b004 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -89,5 +89,5 @@ clean: $(LIBBPF)-clean
 
 FORCE:
 
-.PHONY: all clean FORCE
+.PHONY: all clean FORCE install doc doc-install
 .DEFAULT_GOAL := all
-- 
2.14.1

Re: [PATCH net 0/6] tools: bpftool: fix a minor issues with JSON and Makefiles

[Daniel Borkmann]

On 11/29/2017 02:44 AM, Jakub Kicinski wrote:
> Quentin says:
> 
> First commit in this series fixes a crash that occurs when incorrect
> arguments are passed to bpftool after the `--json` option. It comes from
> the usage() function trying to use the JSON writer, although the latter
> has not been created yet at that point.
> 
> Other patches add destruction of the writer in case the program exits in
> usage(), fix error messages handling when an unrecognized option is
> encountered, remove a spurious new-line character in an error message.
> 
> Last patches are related to the Makefiles. They fix the installation
> directory prefix and .PHONY targets.

Applied to bpf tree, thanks guys!