Re: [PATCH net-next 09/12] tools: bpftool: turn err() and info() macros into functions

[Quentin Monnet]

2017-11-002 17:59 UTC-0700 ~ Joe Perches 
> On Mon, 2017-10-23 at 09:24 -0700, Jakub Kicinski wrote:
>> From: Quentin Monnet 
>>
>> Turn err() and info() macros into functions.
>>
>> In order to avoid naming conflicts with variables in the code, rename
>> them as p_err() and p_info() respectively.
>>
>> The behavior of these functions is similar to the one of the macros for
>> plain output. However, when JSON output is requested, these macros
>> return a JSON-formatted "error" object instead of printing a message to
>> stderr.
>>
>> To handle error messages correctly with JSON, a modification was brought
>> to their behavior nonetheless: the functions now append a end-of-line
>> character at the end of the message. This way, we can remove end-of-line
>> characters at the end of the argument strings, and not have them in the
>> JSON output.
>>
>> All error messages are formatted to hold in a single call to p_err(), in
>> order to produce a single JSON field.
>> Signed-off-by: Quentin Monnet 
>> Acked-by: Jakub Kicinski 
> []
>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> []
>> @@ -97,4 +93,35 @@ int prog_parse_fd(int *argc, char ***argv);
>>  void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes);
>>  void print_hex_data_json(uint8_t *data, size_t len);
>>  
>> +static inline void p_err(const char *fmt, ...)
>> +{
>> +va_list ap;
>> +
>> +va_start(ap, fmt);
>> +if (json_output) {
>> +jsonw_start_object(json_wtr);
>> +jsonw_name(json_wtr, "error");
>> +jsonw_vprintf_enquote(json_wtr, fmt, ap);
>> +jsonw_end_object(json_wtr);
>> +} else {
>> +fprintf(stderr, "Error: ");
>> +vfprintf(stderr, fmt, ap);
>> +fprintf(stderr, "\n");
>> +}
>> +va_end(ap);
>> +}
> inline seems very wasteful.
>
> Why not move p_err and p_info to common.c ?

Hi Joe,
That's a good point. I wrote a patch to change that, Jakub posted it
some minutes ago. Thanks for your feedback!

Quentin

Re: [PATCH net-next 09/12] tools: bpftool: turn err() and info() macros into functions

[Joe Perches]

On Mon, 2017-10-23 at 09:24 -0700, Jakub Kicinski wrote:
> From: Quentin Monnet 
> 
> Turn err() and info() macros into functions.
> 
> In order to avoid naming conflicts with variables in the code, rename
> them as p_err() and p_info() respectively.
> 
> The behavior of these functions is similar to the one of the macros for
> plain output. However, when JSON output is requested, these macros
> return a JSON-formatted "error" object instead of printing a message to
> stderr.
> 
> To handle error messages correctly with JSON, a modification was brought
> to their behavior nonetheless: the functions now append a end-of-line
> character at the end of the message. This way, we can remove end-of-line
> characters at the end of the argument strings, and not have them in the
> JSON output.
> 
> All error messages are formatted to hold in a single call to p_err(), in
> order to produce a single JSON field.

> Signed-off-by: Quentin Monnet 
> Acked-by: Jakub Kicinski 
[]
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
[]
> @@ -97,4 +93,35 @@ int prog_parse_fd(int *argc, char ***argv);
>  void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes);
>  void print_hex_data_json(uint8_t *data, size_t len);
>  
> +static inline void p_err(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + if (json_output) {
> + jsonw_start_object(json_wtr);
> + jsonw_name(json_wtr, "error");
> + jsonw_vprintf_enquote(json_wtr, fmt, ap);
> + jsonw_end_object(json_wtr);
> + } else {
> + fprintf(stderr, "Error: ");
> + vfprintf(stderr, fmt, ap);
> + fprintf(stderr, "\n");
> + }
> + va_end(ap);
> +}

inline seems very wasteful.

Why not move p_err and p_info to common.c ?

> +
> +static inline void p_info(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + if (json_output)
> + return;
> +
> + va_start(ap, fmt);
> + vfprintf(stderr, fmt, ap);
> + fprintf(stderr, "\n");
> + va_end(ap);
> +}
> 

Re: [PATCH net-next 09/12] tools: bpftool: turn err() and info() macros into functions

[Daniel Borkmann]

On 10/23/2017 06:24 PM, Jakub Kicinski wrote:

From: Quentin Monnet 

Turn err() and info() macros into functions.

In order to avoid naming conflicts with variables in the code, rename
them as p_err() and p_info() respectively.

The behavior of these functions is similar to the one of the macros for
plain output. However, when JSON output is requested, these macros
return a JSON-formatted "error" object instead of printing a message to
stderr.

To handle error messages correctly with JSON, a modification was brought
to their behavior nonetheless: the functions now append a end-of-line
character at the end of the message. This way, we can remove end-of-line
characters at the end of the argument strings, and not have them in the
JSON output.

All error messages are formatted to hold in a single call to p_err(), in
order to produce a single JSON field.

Signed-off-by: Quentin Monnet 
Acked-by: Jakub Kicinski 


Acked-by: Daniel Borkmann 

[PATCH net-next 09/12] tools: bpftool: turn err() and info() macros into functions

[Jakub Kicinski]

From: Quentin Monnet 

Turn err() and info() macros into functions.

In order to avoid naming conflicts with variables in the code, rename
them as p_err() and p_info() respectively.

The behavior of these functions is similar to the one of the macros for
plain output. However, when JSON output is requested, these macros
return a JSON-formatted "error" object instead of printing a message to
stderr.

To handle error messages correctly with JSON, a modification was brought
to their behavior nonetheless: the functions now append a end-of-line
character at the end of the message. This way, we can remove end-of-line
characters at the end of the argument strings, and not have them in the
JSON output.

All error messages are formatted to hold in a single call to p_err(), in
order to produce a single JSON field.

Signed-off-by: Quentin Monnet 
Acked-by: Jakub Kicinski 
---
 tools/bpf/bpftool/common.c | 26 +++---
 tools/bpf/bpftool/main.c   | 16 -
 tools/bpf/bpftool/main.h   | 37 +---
 tools/bpf/bpftool/map.c| 87 +-
 tools/bpf/bpftool/prog.c   | 51 +--
 5 files changed, 126 insertions(+), 91 deletions(-)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index e7a756b8ee21..b2533f1cae3e 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -66,8 +66,8 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type 
exp_type)
 
fd = bpf_obj_get(path);
if (fd < 0) {
-   err("bpf obj get (%s): %s\n", path,
-   errno == EACCES && !is_bpffs(dirname(path)) ?
+   p_err("bpf obj get (%s): %s", path,
+ errno == EACCES && !is_bpffs(dirname(path)) ?
"directory not in bpf file system (bpffs)" :
strerror(errno));
return -1;
@@ -79,7 +79,7 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type 
exp_type)
return type;
}
if (type != exp_type) {
-   err("incorrect object type: %s\n", get_fd_type_name(type));
+   p_err("incorrect object type: %s", get_fd_type_name(type));
close(fd);
return -1;
}
@@ -95,14 +95,14 @@ int do_pin_any(int argc, char **argv, int 
(*get_fd_by_id)(__u32))
int fd;
 
if (!is_prefix(*argv, "id")) {
-   err("expected 'id' got %s\n", *argv);
+   p_err("expected 'id' got %s", *argv);
return -1;
}
NEXT_ARG();
 
id = strtoul(*argv, , 0);
if (*endptr) {
-   err("can't parse %s as ID\n", *argv);
+   p_err("can't parse %s as ID", *argv);
return -1;
}
NEXT_ARG();
@@ -112,15 +112,15 @@ int do_pin_any(int argc, char **argv, int 
(*get_fd_by_id)(__u32))
 
fd = get_fd_by_id(id);
if (fd < 0) {
-   err("can't get prog by id (%u): %s\n", id, strerror(errno));
+   p_err("can't get prog by id (%u): %s", id, strerror(errno));
return -1;
}
 
err = bpf_obj_pin(fd, *argv);
close(fd);
if (err) {
-   err("can't pin the object (%s): %s\n", *argv,
-   errno == EACCES && !is_bpffs(dirname(*argv)) ?
+   p_err("can't pin the object (%s): %s", *argv,
+ errno == EACCES && !is_bpffs(dirname(*argv)) ?
"directory not in bpf file system (bpffs)" :
strerror(errno));
return -1;
@@ -153,11 +153,11 @@ int get_fd_type(int fd)
 
n = readlink(path, buf, sizeof(buf));
if (n < 0) {
-   err("can't read link type: %s\n", strerror(errno));
+   p_err("can't read link type: %s", strerror(errno));
return -1;
}
if (n == sizeof(path)) {
-   err("can't read link type: path too long!\n");
+   p_err("can't read link type: path too long!");
return -1;
}
 
@@ -181,7 +181,7 @@ char *get_fdinfo(int fd, const char *key)
 
fdi = fopen(path, "r");
if (!fdi) {
-   err("can't open fdinfo: %s\n", strerror(errno));
+   p_err("can't open fdinfo: %s", strerror(errno));
return NULL;
}
 
@@ -196,7 +196,7 @@ char *get_fdinfo(int fd, const char *key)
 
value = strchr(line, '\t');
if (!value || !value[1]) {
-   err("malformed fdinfo!?\n");
+   p_err("malformed fdinfo!?");
free(line);
return NULL;
}
@@ -209,7 +209,7 @@ char *get_fdinfo(int fd, const char *key)
return line;
}
 
-   err("key '%s' not found in fdinfo\n", key);
+   p_err("key '%s'