[PATCH bpf-next v2 0/3] bpf: btf: print bpftool map data with btf

Hi,

This is v2. There are two key changes which mostly affect patches 2 and 3.
First, we have two BTF outputs:

1. when -j or -p switches are supplied to a map command - this is json- and
backward- compatible
2. when neither of -j and -p is supplied - this makes no promises on json- or
backward- compatibility, and aimed for humans

Second, in addition to map dump command, map lookup command has also been
updated to print data with btf. The rules around -j and -p are same as above.

Here is a summary of changes in v2:

patch 1:
    - line continuation alignment fixes + other style fixes

patch 2:
    - introduce struct btf_dumper which contains context for btf_dumper operation
    - line continuation alignment fixes + other style fixes
    - fix SPDX licence comment style to be C++ style
    - reverse christmas tree style comments
    - in btf_dumper_array() ensure we end json_writer array in case of error

patch 3:
    - btf output for humans is shown when neither -j nor -p is supplied
    - when -j or -p are supplied, augment output with "formatted" object which shows btf data in json
    - added btf output to map lookup command also
    - declarations to follow reverse christmas tree style
    - error message grammar fix and remove full stop
    - line continuation alignment fixes + other style fixes
    - reorganise do_dump_btf() to remove goto and make it clearer
    - remove misleading comment about end of root json object
    - add comment to explain allocation btf buffer
    - brackets around else clause to harmonise with braces on if clause

Thanks,
Okash

This consumes functionality exported in the previous patch. It does the
main job of printing with BTF data. This is used in the following patch
to provide a more readable output of a map's dump. It relies on
json_writer to do json printing. Below is sample output where map keys
are ints and values are of type struct A:

typedef int int_type;
enum E {
        E0,
        E1,
};

struct B {
        int x;
        int y;
};

struct A {
        int m;
        unsigned long long n;
        char o;
        int p[8];
        int q[4][8];
        enum E r;
        void *s;
        struct B t;
        const int u;
        int_type v;
        unsigned int w1: 3;
        unsigned int w2: 3;
};

$ sudo bpftool map dump id 14
[{
        "key": 0,
        "value": {
            "m": 1,
            "n": 2,
            "o": "c",
            "p": [15,16,17,18,15,16,17,18
            ],
            "q": [[25,26,27,28,25,26,27,28
                ],[35,36,37,38,35,36,37,38
                ],[45,46,47,48,45,46,47,48
                ],[55,56,57,58,55,56,57,58
                ]
            ],
            "r": 1,
            "s": 0x7ffd80531cf8,
            "t": {
                "x": 5,
                "y": 10
            },
            "u": 100,
            "v": 20,
            "w1": 0x7,
            "w2": 0x3
        }
    }
]

This patch uses json's {} and [] to imply struct/union and array. More
explicit information can be added later. For example, a command line
option can be introduced to print whether a key or value is struct
or union, name of a struct etc. This will however come at the expense
of duplicating info when, for example, printing an array of structs.
enums are printed as ints without their names.

Signed-off-by: Okash Khawaja <redacted>

---
 tools/bpf/bpftool/btf_dumper.c |  263 +++++++++++++++++++++++++++++++++++++++++
 tools/bpf/bpftool/btf_dumper.h |   23 +++
 2 files changed, 286 insertions(+)

--- /dev/null
+++ b/tools/bpf/bpftool/btf_dumper.c

@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018 Facebook */
+
+#include <linux/btf.h>
+#include <linux/err.h>
+#include <stdio.h> /* for (FILE *) used by json_writer */
+#include <linux/bitops.h>
+#include <string.h>
+#include <ctype.h>
+
+#include "btf.h"
+#include "json_writer.h"
+#include "btf_dumper.h"
+
+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
+#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
+#define BITS_ROUNDUP_BYTES(bits) \
+	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+			      uint8_t bit_offset, const void *data);
+
+static void btf_dumper_ptr(const void *data, json_writer_t *jw,
+			   bool is_plain_text)
+{
+	if (is_plain_text)
+		jsonw_printf(jw, "%p", *((uintptr_t *)data));
+	else
+		jsonw_printf(jw, "%u", *((uintptr_t *)data));
+}
+
+static int btf_dumper_modifier(const struct btf_dumper *d, uint32_t type_id,
+			       const void *data)
+{
+	int32_t actual_type_id = btf__resolve_type(d->btf, type_id);
+	int ret;
+
+	if (actual_type_id < 0)
+		return actual_type_id;
+
+	ret = btf_dumper_do_type(d, actual_type_id, 0, data);
+
+	return ret;
+}
+
+static void btf_dumper_enum(const void *data, json_writer_t *jw)
+{
+	jsonw_printf(jw, "%d", *((int32_t *)data));
+}
+
+static int btf_dumper_array(const struct btf_dumper *d, uint32_t type_id,
+			    const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	struct btf_array *arr = (struct btf_array *)(t + 1);
+	int64_t elem_size;
+	int ret = 0;
+	uint32_t i;
+
+	elem_size = btf__resolve_size(d->btf, arr->type);
+	if (elem_size < 0)
+		return elem_size;
+
+	jsonw_start_array(d->jw);
+	for (i = 0; i < arr->nelems; i++) {
+		ret = btf_dumper_do_type(d, arr->type, 0,
+					 data + (i * elem_size));
+		if (ret)
+			break;
+	}
+
+	jsonw_end_array(d->jw);
+	return ret;
+}
+
+static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
+				const void *data, json_writer_t *jw,
+				bool is_plain_text)
+{
+	uint32_t bits = BTF_INT_BITS(int_type);
+	uint16_t total_bits_offset;
+	uint16_t bytes_to_copy;
+	uint16_t bits_to_copy;
+	uint8_t upper_bits;
+	union {
+		uint64_t u64_num;
+		uint8_t u8_nums[8];
+	} print_num;
+
+	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	bits_to_copy = bits + bit_offset;
+	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
+
+	print_num.u64_num = 0;
+	memcpy(&print_num.u64_num, data, bytes_to_copy);
+
+	upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
+	if (upper_bits) {
+		uint8_t mask = (1 << upper_bits) - 1;
+
+		print_num.u8_nums[bytes_to_copy - 1] &= mask;
+	}
+
+	print_num.u64_num >>= bit_offset;
+
+	if (is_plain_text)
+		jsonw_printf(jw, "0x%llx", print_num.u64_num);
+	else
+		jsonw_printf(jw, "%llu", print_num.u64_num);
+}
+
+static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
+			  const void *data, json_writer_t *jw,
+			  bool is_plain_text)
+{
+	uint32_t *int_type = (uint32_t *)(t + 1);
+	uint32_t bits = BTF_INT_BITS(*int_type);
+	int ret = 0;
+
+	/* if this is bit field */
+	if (bit_offset || BTF_INT_OFFSET(*int_type) ||
+	    BITS_PER_BYTE_MASKED(bits)) {
+		btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+				    is_plain_text);
+		return ret;
+	}
+
+	switch (BTF_INT_ENCODING(*int_type)) {
+	case 0:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%lu", *((uint64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%u", *((uint32_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 16)
+			jsonw_printf(jw, "%hu", *((uint16_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 8)
+			jsonw_printf(jw, "%hhu", *((uint8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_SIGNED:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%ld", *((int64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%d", *((int32_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  16)
+			jsonw_printf(jw, "%hd", *((int16_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  8)
+			jsonw_printf(jw, "%hhd", *((int8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_CHAR:
+		if (*((char *)data) == '\0')
+			jsonw_null(jw);
+		else if (isprint(*((char *)data)))
+			jsonw_printf(jw, "\"%c\"", *((char *)data));
+		else
+			if (is_plain_text)
+				jsonw_printf(jw, "%hhx", *((char *)data));
+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));
+		break;
+	case BTF_INT_BOOL:
+		jsonw_bool(jw, *((int *)data));
+		break;
+	default:
+		/* shouldn't happen */
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int btf_dumper_struct(const struct btf_dumper *d, uint32_t type_id,
+			     const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	struct btf_member *m;
+	int ret = 0;
+	int i, vlen;
+
+	if (!t)
+		return -EINVAL;
+
+	vlen = BTF_INFO_VLEN(t->info);
+	jsonw_start_object(d->jw);
+	m = (struct btf_member *)(t + 1);
+
+	for (i = 0; i < vlen; i++) {
+		jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
+		ret = btf_dumper_do_type(d, m[i].type,
+					 BITS_PER_BYTE_MASKED(m[i].offset), data
+					 + BITS_ROUNDDOWN_BYTES(m[i].offset));
+		if (ret)
+			return ret;
+	}
+
+	jsonw_end_object(d->jw);
+
+	return 0;
+}
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+			      uint8_t bit_offset, const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	int ret = 0;
+
+	switch (BTF_INFO_KIND(t->info)) {
+	case BTF_KIND_INT:
+		ret = btf_dumper_int(t, bit_offset, data, d->jw,
+				     d->is_plain_text);
+		break;
+	case BTF_KIND_STRUCT:
+	case BTF_KIND_UNION:
+		ret = btf_dumper_struct(d, type_id, data);
+		break;
+	case BTF_KIND_ARRAY:
+		ret = btf_dumper_array(d, type_id, data);
+		break;
+	case BTF_KIND_ENUM:
+		btf_dumper_enum(data, d->jw);
+		break;
+	case BTF_KIND_PTR:
+		btf_dumper_ptr(data, d->jw, d->is_plain_text);
+		break;
+	case BTF_KIND_UNKN:
+		jsonw_printf(d->jw, "(unknown)");
+		break;
+	case BTF_KIND_FWD:
+		/* map key or value can't be forward */
+		ret = -EINVAL;
+		break;
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_CONST:
+	case BTF_KIND_RESTRICT:
+		ret = btf_dumper_modifier(d, type_id, data);
+		break;
+	default:
+		jsonw_printf(d->jw, "(unsupported-kind");
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+			const void *data)
+{
+	if (!d)
+		return -EINVAL;
+
+	return btf_dumper_do_type(d, type_id, 0, data);
+}
--- /dev/null
+++ b/tools/bpf/bpftool/btf_dumper.h

@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 Facebook */
+
+#ifndef BTF_DUMPER_H
+#define BTF_DUMPER_H
+
+struct btf_dumper {
+	const struct btf *btf;
+	json_writer_t *jw;
+	bool is_plain_text;
+};
+
+/* btf_dumper_type - print data along with type information
+ * @d: an instance containing context for dumping types
+ * @type_id: index in btf->types array. this points to the type to be dumped
+ * @data: pointer the actual data, i.e. the values to be printed
+ *
+ * Returns zero on success and negative error code otherwise
+ */
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+			const void *data);
+
+#endif

This patch augments the output of bpftool's map dump and map lookup
commands to print data along side btf info, if the correspondin btf
info is available. The outputs for each of  map dump and map lookup
commands are augmented in two ways:

1. when neither of -j and -p are supplied, btf-ful map data is printed
whose aim is human readability. This means no commitments for json- or
backward- compatibility.

2. when either -j or -p are supplied, a new json object named
"formatted" is added for each key-value pair. This object contains the
same data as the key-value pair, but with btf info. "formatted" object
promises json- and backward- compatibility. Below is a sample output.

$ bpftool map dump -p id 8
[{
        "key": ["0x0f","0x00","0x00","0x00"
        ],
        "value": ["0x03", "0x00", "0x00", "0x00", ...
        ],
        "formatted": {
                "key": 15,
                "value": {
                        "int_field":  3,
                        ...
                }
        }
}
]

This patch calls btf_dumper introduced in previous patch to accomplish
the above. Indeed, btf-ful info is only displayed if btf data for the
given map is available. Otherwise existing output is displayed as-is.

Signed-off-by: Okash Khawaja <redacted>

---
 tools/bpf/bpftool/map.c |  174 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 166 insertions(+), 8 deletions(-)

--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c

@@ -41,9 +41,13 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <linux/err.h>
 
 #include <bpf.h>
 
+#include "json_writer.h"
+#include "btf.h"
+#include "btf_dumper.h"
 #include "main.h"
 
 static const char * const map_type_name[] = {

@@ -148,8 +152,99 @@ int map_parse_fd_and_info(int *argc, cha
 	return fd;
 }
 
+static int do_dump_btf(const struct btf_dumper *d,
+		       struct bpf_map_info *map_info, void *key,
+		       void *value)
+{
+	int ret;
+
+	/* start of key-value pair */
+	jsonw_start_object(d->jw);
+
+	jsonw_name(d->jw, "key");
+
+	ret = btf_dumper_type(d, map_info->btf_key_type_id, key);
+	if (ret)
+		return ret;
+
+	jsonw_name(d->jw, "value");
+
+	ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
+
+	/* end of key-value pair */
+	jsonw_end_object(d->jw);
+
+	return ret;
+}
+
+static struct btf *get_btf(struct bpf_map_info *map_info)
+{
+	int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
+	struct bpf_btf_info btf_info = { 0 };
+	__u32 len = sizeof(btf_info);
+	void *ptr = NULL, *temp_ptr;
+	struct btf *btf = NULL;
+	uint32_t last_size;
+	int err;
+
+	if (btf_fd < 0)
+		return NULL;
+
+	/* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
+	 * let's start with a sane default - 4KiB here - and resize it only if
+	 * bpf_obj_get_info_by_fd() needs a bigger buffer. the do-while loop
+	 * below should run a maximum of two iterations and that will be when
+	 * we have to resize to a bigger buffer.
+	 */
+	btf_info.btf_size = 4096;
+	do {
+		last_size = btf_info.btf_size;
+		temp_ptr = realloc(ptr, last_size);
+		if (!temp_ptr) {
+			p_err("unable to allocate memory for debug info");
+			goto exit_free;
+		}
+
+		ptr = temp_ptr;
+		bzero(ptr, last_size);
+		btf_info.btf = ptr_to_u64(ptr);
+		err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+	} while (!err && btf_info.btf_size > last_size && last_size == 4096);
+
+	if (err || btf_info.btf_size > last_size) {
+		p_info("can't get btf info. debug info won't be displayed. error: %s",
+		       err ? strerror(errno) : "exceeds size retry");
+		goto exit_free;
+	}
+
+	btf = btf__new((uint8_t *)btf_info.btf,
+		       btf_info.btf_size, NULL);
+	if (IS_ERR(btf)) {
+		printf("error when initialising btf: %s\n",
+		       strerror(PTR_ERR(btf)));
+		btf = NULL;
+	}
+
+exit_free:
+	close(btf_fd);
+	free(ptr);
+
+	return btf;
+}
+
+static json_writer_t *get_btf_writer(void)
+{
+	json_writer_t *jw = jsonw_new(stdout);
+
+	if (!jw)
+		return NULL;
+	jsonw_pretty(jw, true);
+
+	return jw;
+}
+
 static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
-			     unsigned char *value)
+			     unsigned char *value, struct btf *btf)
 {
 	jsonw_start_object(json_wtr);
 

@@ -158,6 +253,15 @@ static void print_entry_json(struct bpf_
 		print_hex_data_json(key, info->key_size);
 		jsonw_name(json_wtr, "value");
 		print_hex_data_json(value, info->value_size);
+		if (btf) {
+			struct btf_dumper d = {
+				.btf = btf,
+				.jw = json_wtr,
+				.is_plain_text = false,
+			};
+			jsonw_name(json_wtr, "formatted");
+			do_dump_btf(&d, info, key, value);
+		}
 	} else {
 		unsigned int i, n;
 

@@ -508,10 +612,12 @@ static int do_show(int argc, char **argv
 
 static int do_dump(int argc, char **argv)
 {
+	struct bpf_map_info info = {};
 	void *key, *value, *prev_key;
 	unsigned int num_elems = 0;
-	struct bpf_map_info info = {};
 	__u32 len = sizeof(info);
+	json_writer_t *btf_wtr;
+	struct btf *btf = NULL;
 	int err;
 	int fd;
 

@@ -537,8 +643,22 @@ static int do_dump(int argc, char **argv
 	}
 
 	prev_key = NULL;
+
+	btf = get_btf(&info);
 	if (json_output)
 		jsonw_start_array(json_wtr);
+	else
+		if (btf) {
+			btf_wtr = get_btf_writer();
+			if (!btf_wtr) {
+				p_info("failed to create json writer for btf. falling back to plain output");
+				btf__free(btf);
+				btf = NULL;
+			} else {
+				jsonw_start_array(btf_wtr);
+			}
+		}
+
 	while (true) {
 		err = bpf_map_get_next_key(fd, prev_key, key);
 		if (err) {

@@ -549,9 +669,18 @@ static int do_dump(int argc, char **argv
 
 		if (!bpf_map_lookup_elem(fd, key, value)) {
 			if (json_output)
-				print_entry_json(&info, key, value);
+				print_entry_json(&info, key, value, btf);
 			else
-				print_entry_plain(&info, key, value);
+				if (btf) {
+					struct btf_dumper d = {
+						.btf = btf,
+						.jw = btf_wtr,
+						.is_plain_text = true,
+					};
+					do_dump_btf(&d, &info, key, value);
+				} else {
+					print_entry_plain(&info, key, value);
+				}
 		} else {
 			if (json_output) {
 				jsonw_name(json_wtr, "key");

@@ -574,14 +703,19 @@ static int do_dump(int argc, char **argv
 
 	if (json_output)
 		jsonw_end_array(json_wtr);
-	else
+	else if (btf) {
+		jsonw_end_array(btf_wtr);
+		jsonw_destroy(&btf_wtr);
+	} else {
 		printf("Found %u element%s\n", num_elems,
 		       num_elems != 1 ? "s" : "");
+	}
 
 exit_free:
 	free(key);
 	free(value);
 	close(fd);
+	btf__free(btf);
 
 	return err;
 }

@@ -637,6 +771,8 @@ static int do_lookup(int argc, char **ar
 {
 	struct bpf_map_info info = {};
 	__u32 len = sizeof(info);
+	json_writer_t *btf_wtr;
+	struct btf *btf = NULL;
 	void *key, *value;
 	int err;
 	int fd;

@@ -662,10 +798,31 @@ static int do_lookup(int argc, char **ar
 
 	err = bpf_map_lookup_elem(fd, key, value);
 	if (!err) {
-		if (json_output)
-			print_entry_json(&info, key, value);
-		else
+		btf = get_btf(&info);
+		if (json_output) {
+			print_entry_json(&info, key, value, btf);
+		} else if (btf) {
+			/* if here json_wtr wouldn't have been initialised,
+			 * so let's create separate writer for btf
+			 */
+			btf_wtr = get_btf_writer();
+			if (!btf_wtr) {
+				p_info("failed to create json writer for btf. falling back to plain output");
+				btf__free(btf);
+				btf = NULL;
+				print_entry_plain(&info, key, value);
+			} else {
+				struct btf_dumper d = {
+					.btf = btf,
+					.jw = btf_wtr,
+					.is_plain_text = true,
+				};
+				do_dump_btf(&d, &info, key, value);
+				jsonw_destroy(&btf_wtr);
+			}
+		} else {
 			print_entry_plain(&info, key, value);
+		}
 	} else if (errno == ENOENT) {
 		if (json_output) {
 			jsonw_null(json_wtr);

@@ -682,6 +839,7 @@ exit_free:
 	free(key);
 	free(value);
 	close(fd);
+	btf__free(btf);
 
 	return err;
 }

On Mon, 2 Jul 2018 11:39:15 -0700, Okash Khawaja wrote:

+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)

Perhaps it's just me but BIT_OFFSET or BIT_COUNT as a name of this macro
would make it more obvious to parse in the code below.

+#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
+#define BITS_ROUNDUP_BYTES(bits) \
+	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+			      uint8_t bit_offset, const void *data);
+
+static void btf_dumper_ptr(const void *data, json_writer_t *jw,
+			   bool is_plain_text)
+{
+	if (is_plain_text)
+		jsonw_printf(jw, "%p", *((uintptr_t *)data));
+	else
+		jsonw_printf(jw, "%u", *((uintptr_t *)data));
+}
+
+static int btf_dumper_modifier(const struct btf_dumper *d, uint32_t type_id,
+			       const void *data)
+{
+	int32_t actual_type_id = btf__resolve_type(d->btf, type_id);

Please prefer kernel types like __u32 wherever possible.

+	int ret;
+
+	if (actual_type_id < 0)
+		return actual_type_id;
+
+	ret = btf_dumper_do_type(d, actual_type_id, 0, data);
+
+	return ret;

ret is unnecessary.

+}
+
+static void btf_dumper_enum(const void *data, json_writer_t *jw)
+{
+	jsonw_printf(jw, "%d", *((int32_t *)data));

Unnecessary parenthesis.  There is a lot of those, please remove them
all.

+}
+
+static int btf_dumper_array(const struct btf_dumper *d, uint32_t type_id,
+			    const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	struct btf_array *arr = (struct btf_array *)(t + 1);
+	int64_t elem_size;
+	int ret = 0;
+	uint32_t i;
+
+	elem_size = btf__resolve_size(d->btf, arr->type);
+	if (elem_size < 0)
+		return elem_size;
+
+	jsonw_start_array(d->jw);
+	for (i = 0; i < arr->nelems; i++) {
+		ret = btf_dumper_do_type(d, arr->type, 0,
+					 data + (i * elem_size));

Unnecessary parenthesis.

+		if (ret)
+			break;
+	}
+
+	jsonw_end_array(d->jw);
+	return ret;
+}
+
+static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
+				const void *data, json_writer_t *jw,
+				bool is_plain_text)
+{
+	uint32_t bits = BTF_INT_BITS(int_type);
+	uint16_t total_bits_offset;
+	uint16_t bytes_to_copy;
+	uint16_t bits_to_copy;

Please use normal int types for things which don't have to be
explicitly sized.  Using explicitly sized variables is bad style, 
and ALU operations other than on word or byte quantities are usually
slower on modern CPUs.

+	uint8_t upper_bits;
+	union {
+		uint64_t u64_num;
+		uint8_t u8_nums[8];

Are the int types in BTF constrained to 64bit at most?

+	} print_num;
+
+	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	bits_to_copy = bits + bit_offset;
+	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
+
+	print_num.u64_num = 0;
+	memcpy(&print_num.u64_num, data, bytes_to_copy);

This scheme is unlikely to work on big endian machines...

+	upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
+	if (upper_bits) {
+		uint8_t mask = (1 << upper_bits) - 1;
+
+		print_num.u8_nums[bytes_to_copy - 1] &= mask;
+	}
+
+	print_num.u64_num >>= bit_offset;
+
+	if (is_plain_text)
+		jsonw_printf(jw, "0x%llx", print_num.u64_num);
+	else
+		jsonw_printf(jw, "%llu", print_num.u64_num);
+}
+
+static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
+			  const void *data, json_writer_t *jw,
+			  bool is_plain_text)
+{
+	uint32_t *int_type = (uint32_t *)(t + 1);
+	uint32_t bits = BTF_INT_BITS(*int_type);
+	int ret = 0;
+
+	/* if this is bit field */
+	if (bit_offset || BTF_INT_OFFSET(*int_type) ||
+	    BITS_PER_BYTE_MASKED(bits)) {
+		btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+				    is_plain_text);
+		return ret;
+	}
+
+	switch (BTF_INT_ENCODING(*int_type)) {
+	case 0:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%lu", *((uint64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%u", *((uint32_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 16)
+			jsonw_printf(jw, "%hu", *((uint16_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 8)
+			jsonw_printf(jw, "%hhu", *((uint8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_SIGNED:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%ld", *((int64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%d", *((int32_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  16)

Please drop the double space.  Both for 16 where it makes no sense and
for 8 where it's marginally useful but not really.

+			jsonw_printf(jw, "%hd", *((int16_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  8)
+			jsonw_printf(jw, "%hhd", *((int8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_CHAR:
+		if (*((char *)data) == '\0')
+			jsonw_null(jw);

Mm.. I don't think 0 char is equivalent to null.

+		else if (isprint(*((char *)data)))
+			jsonw_printf(jw, "\"%c\"", *((char *)data));

This looks very suspicious.  So if I see a "6" for a char field it's
either a 6 ('\u0006') or a 54 ('6')...

+		else
+			if (is_plain_text)
+				jsonw_printf(jw, "%hhx", *((char *)data));
+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.

+		break;
+	case BTF_INT_BOOL:
+		jsonw_bool(jw, *((int *)data));
+		break;
+	default:
+		/* shouldn't happen */
+		ret = -EINVAL;

You only set ret to something else than 0 here just to break and
immediately return.  Please remove the ret variable.

+		break;
+	}
+
+	return ret;
+}
+
+static int btf_dumper_struct(const struct btf_dumper *d, uint32_t type_id,
+			     const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);

Please don't call functions which need error checking as initialized
the if below looks very awkward..

+	struct btf_member *m;
+	int ret = 0;
+
+	int i, vlen;
+
+	if (!t)
+		return -EINVAL;
+
+	vlen = BTF_INFO_VLEN(t->info);
+	jsonw_start_object(d->jw);
+	m = (struct btf_member *)(t + 1);
+
+	for (i = 0; i < vlen; i++) {
+		jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
+		ret = btf_dumper_do_type(d, m[i].type,
+					 BITS_PER_BYTE_MASKED(m[i].offset), data
+					 + BITS_ROUNDDOWN_BYTES(m[i].offset));

Please use a temp variable to avoid this awkward multi-line sum.

+		if (ret)
+			return ret;

You can't return without jsonw_end_object().

+	}
+
+	jsonw_end_object(d->jw);
+
+	return 0;
+}
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+			      uint8_t bit_offset, const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	int ret = 0;
+
+	switch (BTF_INFO_KIND(t->info)) {
+	case BTF_KIND_INT:
+		ret = btf_dumper_int(t, bit_offset, data, d->jw,
+				     d->is_plain_text);
+		break;
+	case BTF_KIND_STRUCT:
+	case BTF_KIND_UNION:
+		ret = btf_dumper_struct(d, type_id, data);
+		break;
+	case BTF_KIND_ARRAY:
+		ret = btf_dumper_array(d, type_id, data);
+		break;
+	case BTF_KIND_ENUM:
+		btf_dumper_enum(data, d->jw);
+		break;
+	case BTF_KIND_PTR:
+		btf_dumper_ptr(data, d->jw, d->is_plain_text);
+		break;
+	case BTF_KIND_UNKN:
+		jsonw_printf(d->jw, "(unknown)");
+		break;
+	case BTF_KIND_FWD:
+		/* map key or value can't be forward */

Right, but you have to print _something_, otherwise we would have a
name without a value, which would break JSON, no?

+		ret = -EINVAL;
+		break;
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_CONST:
+	case BTF_KIND_RESTRICT:
+		ret = btf_dumper_modifier(d, type_id, data);
+		break;
+	default:
+		jsonw_printf(d->jw, "(unsupported-kind");
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;

Why return ret; at all, there is no code after the switch just return
directly from cases and save 9 LOC.

+}
+
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+			const void *data)
+{
+	if (!d)
+		return -EINVAL;

No need for defensive programming.

Please don't add header files for a single struct and single function.
Just put this in main.h.

On Mon, 2 Jul 2018 11:39:16 -0700, Okash Khawaja wrote:

Please keep the headers in alphabetical order.

goto err_end_obj;

+	jsonw_name(d->jw, "value");
+
+	ret = btf_dumper_type(d, map_info->btf_value_type_id, value);

err_end_obj:

+	/* end of key-value pair */
+	jsonw_end_object(d->jw);
+
+	return ret;
+}
+
+static struct btf *get_btf(struct bpf_map_info *map_info)
+{
+	int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);

No failing functions in initializers please.

+	struct bpf_btf_info btf_info = { 0 };
+	__u32 len = sizeof(btf_info);
+	void *ptr = NULL, *temp_ptr;
+	struct btf *btf = NULL;
+	uint32_t last_size;
+	int err;
+
+	if (btf_fd < 0)
+		return NULL;
+
+	/* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
+	 * let's start with a sane default - 4KiB here - and resize it only if
+	 * bpf_obj_get_info_by_fd() needs a bigger buffer. the do-while loop
+	 * below should run a maximum of two iterations and that will be when
+	 * we have to resize to a bigger buffer.
+	 */
+	btf_info.btf_size = 4096;
+	do {
+		last_size = btf_info.btf_size;
+		temp_ptr = realloc(ptr, last_size);
+		if (!temp_ptr) {
+			p_err("unable to allocate memory for debug info");
+			goto exit_free;
+		}
+
+		ptr = temp_ptr;
+		bzero(ptr, last_size);
+		btf_info.btf = ptr_to_u64(ptr);
+		err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+	} while (!err && btf_info.btf_size > last_size && last_size == 4096);
+
+	if (err || btf_info.btf_size > last_size) {
+		p_info("can't get btf info. debug info won't be displayed. error: %s",
+		       err ? strerror(errno) : "exceeds size retry");
+		goto exit_free;
+	}

This "run me twice while handling realloc failure" loop seems very
unappealing.  Could you just open code this?

+	btf = btf__new((uint8_t *)btf_info.btf,
+		       btf_info.btf_size, NULL);
+	if (IS_ERR(btf)) {
+		printf("error when initialising btf: %s\n",
+		       strerror(PTR_ERR(btf)));

printfs are not allowed, they break JSON.

Empty line after variables, please fix everywhere.

Do we need to start array for non-JSON output?

This is way too much code in a if (!err) branch.

Please refactor so that err = bpf_map_lookup_elem(fd, key, value); is
followed by error handling and the actual printing is non-indented.

On Mon, Jul 02, 2018 at 10:06:59PM -0700, Jakub Kicinski wrote:

On Mon, 2 Jul 2018 11:39:15 -0700, Okash Khawaja wrote:

[...]

quoted

+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)

Perhaps it's just me but BIT_OFFSET or BIT_COUNT as a name of this macro
would make it more obvious to parse in the code below.

I don't mind either. However these macro names are also used inside
kernel for same purpose. For sake of consistency, I'd recommend we keep
them :)

quoted

+#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
+#define BITS_ROUNDUP_BYTES(bits) \
+	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+			      uint8_t bit_offset, const void *data);
+
+static void btf_dumper_ptr(const void *data, json_writer_t *jw,
+			   bool is_plain_text)
+{
+	if (is_plain_text)
+		jsonw_printf(jw, "%p", *((uintptr_t *)data));
+	else
+		jsonw_printf(jw, "%u", *((uintptr_t *)data));
+}
+
+static int btf_dumper_modifier(const struct btf_dumper *d, uint32_t type_id,
+			       const void *data)
+{
+	int32_t actual_type_id = btf__resolve_type(d->btf, type_id);

Please prefer kernel types like __u32 wherever possible.

quoted

+	int ret;
+
+	if (actual_type_id < 0)
+		return actual_type_id;
+
+	ret = btf_dumper_do_type(d, actual_type_id, 0, data);
+
+	return ret;

ret is unnecessary.

quoted

+}
+
+static void btf_dumper_enum(const void *data, json_writer_t *jw)
+{
+	jsonw_printf(jw, "%d", *((int32_t *)data));

Unnecessary parenthesis.  There is a lot of those, please remove them
all.

quoted

+}
+
+static int btf_dumper_array(const struct btf_dumper *d, uint32_t type_id,
+			    const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	struct btf_array *arr = (struct btf_array *)(t + 1);
+	int64_t elem_size;
+	int ret = 0;
+	uint32_t i;
+
+	elem_size = btf__resolve_size(d->btf, arr->type);
+	if (elem_size < 0)
+		return elem_size;
+
+	jsonw_start_array(d->jw);
+	for (i = 0; i < arr->nelems; i++) {
+		ret = btf_dumper_do_type(d, arr->type, 0,
+					 data + (i * elem_size));

Unnecessary parenthesis.

quoted

+		if (ret)
+			break;
+	}
+
+	jsonw_end_array(d->jw);
+	return ret;
+}
+
+static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
+				const void *data, json_writer_t *jw,
+				bool is_plain_text)
+{
+	uint32_t bits = BTF_INT_BITS(int_type);
+	uint16_t total_bits_offset;
+	uint16_t bytes_to_copy;
+	uint16_t bits_to_copy;

Please use normal int types for things which don't have to be
explicitly sized.  Using explicitly sized variables is bad style, 
and ALU operations other than on word or byte quantities are usually
slower on modern CPUs.

quoted

+	uint8_t upper_bits;
+	union {
+		uint64_t u64_num;
+		uint8_t u8_nums[8];

Are the int types in BTF constrained to 64bit at most?

quoted

+	} print_num;
+
+	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	bits_to_copy = bits + bit_offset;
+	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
+
+	print_num.u64_num = 0;
+	memcpy(&print_num.u64_num, data, bytes_to_copy);

This scheme is unlikely to work on big endian machines...

Can you give an example how?

quoted

+	upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
+	if (upper_bits) {
+		uint8_t mask = (1 << upper_bits) - 1;
+
+		print_num.u8_nums[bytes_to_copy - 1] &= mask;
+	}
+
+	print_num.u64_num >>= bit_offset;
+
+	if (is_plain_text)
+		jsonw_printf(jw, "0x%llx", print_num.u64_num);
+	else
+		jsonw_printf(jw, "%llu", print_num.u64_num);
+}
+
+static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
+			  const void *data, json_writer_t *jw,
+			  bool is_plain_text)
+{
+	uint32_t *int_type = (uint32_t *)(t + 1);
+	uint32_t bits = BTF_INT_BITS(*int_type);
+	int ret = 0;
+
+	/* if this is bit field */
+	if (bit_offset || BTF_INT_OFFSET(*int_type) ||
+	    BITS_PER_BYTE_MASKED(bits)) {
+		btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+				    is_plain_text);
+		return ret;
+	}
+
+	switch (BTF_INT_ENCODING(*int_type)) {
+	case 0:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%lu", *((uint64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%u", *((uint32_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 16)
+			jsonw_printf(jw, "%hu", *((uint16_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 8)
+			jsonw_printf(jw, "%hhu", *((uint8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_SIGNED:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%ld", *((int64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%d", *((int32_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  16)

Please drop the double space.  Both for 16 where it makes no sense and
for 8 where it's marginally useful but not really.

quoted

+			jsonw_printf(jw, "%hd", *((int16_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  8)
+			jsonw_printf(jw, "%hhd", *((int8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_CHAR:
+		if (*((char *)data) == '\0')
+			jsonw_null(jw);

Mm.. I don't think 0 char is equivalent to null.

Yes, thanks. Will fix.

quoted

+		else if (isprint(*((char *)data)))
+			jsonw_printf(jw, "\"%c\"", *((char *)data));

This looks very suspicious.  So if I see a "6" for a char field it's
either a 6 ('\u0006') or a 54 ('6')...

It will always be 54. May be I missed your point. Could you explain why
it would be other than 54?

quoted

+		else
+			if (is_plain_text)
+				jsonw_printf(jw, "%hhx", *((char *)data));
+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.

Okay that makes sense

quoted

+		break;
+	case BTF_INT_BOOL:
+		jsonw_bool(jw, *((int *)data));
+		break;
+	default:
+		/* shouldn't happen */
+		ret = -EINVAL;

You only set ret to something else than 0 here just to break and
immediately return.  Please remove the ret variable.

quoted

+		break;
+	}
+
+	return ret;
+}
+
+static int btf_dumper_struct(const struct btf_dumper *d, uint32_t type_id,
+			     const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);

Please don't call functions which need error checking as initialized
the if below looks very awkward..

quoted

+	struct btf_member *m;
+	int ret = 0;
+
+	int i, vlen;
+
+	if (!t)
+		return -EINVAL;
+
+	vlen = BTF_INFO_VLEN(t->info);
+	jsonw_start_object(d->jw);
+	m = (struct btf_member *)(t + 1);
+
+	for (i = 0; i < vlen; i++) {
+		jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
+		ret = btf_dumper_do_type(d, m[i].type,
+					 BITS_PER_BYTE_MASKED(m[i].offset), data
+					 + BITS_ROUNDDOWN_BYTES(m[i].offset));

Please use a temp variable to avoid this awkward multi-line sum.

quoted

+		if (ret)
+			return ret;

You can't return without jsonw_end_object().

quoted

+	}
+
+	jsonw_end_object(d->jw);
+
+	return 0;
+}
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+			      uint8_t bit_offset, const void *data)
+{
+	const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+	int ret = 0;
+
+	switch (BTF_INFO_KIND(t->info)) {
+	case BTF_KIND_INT:
+		ret = btf_dumper_int(t, bit_offset, data, d->jw,
+				     d->is_plain_text);
+		break;
+	case BTF_KIND_STRUCT:
+	case BTF_KIND_UNION:
+		ret = btf_dumper_struct(d, type_id, data);
+		break;
+	case BTF_KIND_ARRAY:
+		ret = btf_dumper_array(d, type_id, data);
+		break;
+	case BTF_KIND_ENUM:
+		btf_dumper_enum(data, d->jw);
+		break;
+	case BTF_KIND_PTR:
+		btf_dumper_ptr(data, d->jw, d->is_plain_text);
+		break;
+	case BTF_KIND_UNKN:
+		jsonw_printf(d->jw, "(unknown)");
+		break;
+	case BTF_KIND_FWD:
+		/* map key or value can't be forward */

Right, but you have to print _something_, otherwise we would have a
name without a value, which would break JSON, no?

quoted

+		ret = -EINVAL;
+		break;
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_CONST:
+	case BTF_KIND_RESTRICT:
+		ret = btf_dumper_modifier(d, type_id, data);
+		break;
+	default:
+		jsonw_printf(d->jw, "(unsupported-kind");
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;

Why return ret; at all, there is no code after the switch just return
directly from cases and save 9 LOC.

quoted

+}
+
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+			const void *data)
+{
+	if (!d)
+		return -EINVAL;

No need for defensive programming.

quoted

+	return btf_dumper_do_type(d, type_id, 0, data);
+}

--- /dev/null
+++ b/tools/bpf/bpftool/btf_dumper.h

@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 Facebook */
+
+#ifndef BTF_DUMPER_H
+#define BTF_DUMPER_H
+
+struct btf_dumper {
+	const struct btf *btf;
+	json_writer_t *jw;
+	bool is_plain_text;
+};
+
+/* btf_dumper_type - print data along with type information
+ * @d: an instance containing context for dumping types
+ * @type_id: index in btf->types array. this points to the type to be dumped
+ * @data: pointer the actual data, i.e. the values to be printed
+ *
+ * Returns zero on success and negative error code otherwise
+ */
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+			const void *data);
+
+#endif

Please don't add header files for a single struct and single function.
Just put this in main.h.

Thanks for your feedback. I'll reply with v3.

Okash

On Tue, 3 Jul 2018 22:46:00 +0100, Okash Khawaja wrote:

On Mon, Jul 02, 2018 at 10:06:59PM -0700, Jakub Kicinski wrote:

quoted

On Mon, 2 Jul 2018 11:39:15 -0700, Okash Khawaja wrote:  

quoted

+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)  

Perhaps it's just me but BIT_OFFSET or BIT_COUNT as a name of this macro
would make it more obvious to parse in the code below.  

I don't mind either. However these macro names are also used inside
kernel for same purpose. For sake of consistency, I'd recommend we keep
them :)

Ugh, okay :)

quoted

quoted

+	} print_num;
+
+	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	bits_to_copy = bits + bit_offset;
+	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
+
+	print_num.u64_num = 0;
+	memcpy(&print_num.u64_num, data, bytes_to_copy);  

This scheme is unlikely to work on big endian machines...  

Can you give an example how?

On BE:

Input:         [0x01, 0x82]
Bit length:    15
Bytes to copy:  2
bit_offset:     0
upper_bits:     7

print_num.u64_num = 0;
# [0, 0, 0, 0,   0, 0, 0, 0]

memcpy(&print_num.u64_num, data, bytes_to_copy);  
# [0x01, 0x82, 0, 0,   0, 0, 0, 0]

mask = (1 << upper_bits) - 1;
# mask = 0x7f

print_num.u8_nums[bytes_to_copy - 1] &= mask;
# [0x01, 0x02, 0, 0,   0, 0, 0, 0]

printf("0x%llx", print_num.u64_num);
# 0x0102000000000000 AKA 72620543991349248
# expected:
# 0x0102             AKA 258

Am I missing something?

quoted

quoted

+	upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
+	if (upper_bits) {
+		uint8_t mask = (1 << upper_bits) - 1;
+
+		print_num.u8_nums[bytes_to_copy - 1] &= mask;
+	}
+
+	print_num.u64_num >>= bit_offset;
+
+	if (is_plain_text)
+		jsonw_printf(jw, "0x%llx", print_num.u64_num);
+	else
+		jsonw_printf(jw, "%llu", print_num.u64_num);
+}
+
+static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
+			  const void *data, json_writer_t *jw,
+			  bool is_plain_text)
+{
+	uint32_t *int_type = (uint32_t *)(t + 1);
+	uint32_t bits = BTF_INT_BITS(*int_type);
+	int ret = 0;
+
+	/* if this is bit field */
+	if (bit_offset || BTF_INT_OFFSET(*int_type) ||
+	    BITS_PER_BYTE_MASKED(bits)) {
+		btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+				    is_plain_text);
+		return ret;
+	}
+
+	switch (BTF_INT_ENCODING(*int_type)) {
+	case 0:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%lu", *((uint64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%u", *((uint32_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 16)
+			jsonw_printf(jw, "%hu", *((uint16_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 8)
+			jsonw_printf(jw, "%hhu", *((uint8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_SIGNED:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%ld", *((int64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%d", *((int32_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  16)  

Please drop the double space.  Both for 16 where it makes no sense and
for 8 where it's marginally useful but not really.
  

quoted

+			jsonw_printf(jw, "%hd", *((int16_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  8)
+			jsonw_printf(jw, "%hhd", *((int8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_CHAR:
+		if (*((char *)data) == '\0')
+			jsonw_null(jw);  

Mm.. I don't think 0 char is equivalent to null.  

Yes, thanks. Will fix.

quoted

quoted

+		else if (isprint(*((char *)data)))
+			jsonw_printf(jw, "\"%c\"", *((char *)data));  

This looks very suspicious.  So if I see a "6" for a char field it's
either a 6 ('\u0006') or a 54 ('6')...  

It will always be 54. May be I missed your point. Could you explain why
it would be other than 54?

Ah, I think I missed that %c is in quotes...

quoted

quoted

+		else
+			if (is_plain_text)
+				jsonw_printf(jw, "%hhx", *((char *)data));

This seems to be missing a "0x" prefix?

quoted

quoted

+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));  

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.  

Okay that makes sense

Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
it may actually be more likely to just be used as a raw byte buffer...
Either way I think it may be nice to keep it consistent, at least for
the JSON output could we do either always ints or always characters?

On Tue, 3 Jul 2018 15:23:31 -0700, Jakub Kicinski wrote:

quoted

quoted

quoted

+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));    

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.    

Okay that makes sense  

Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
it may actually be more likely to just be used as a raw byte buffer...

Actually, what is the definition/purpose of BTF_INT_CHAR?  There seems
to be no BTF_INT_SHORT and BTF_INT_SIGNED can simply be of size 8...
Is normal int only used for bitfields of size 8 and BTF_INT_CHAR for
char variables?

The kernel seems to be rejecting combinations of those flags, is
unsigned char going to not be marked as char then?

Either way I think it may be nice to keep it consistent, at least for
the JSON output could we do either always ints or always characters?

On Tue, Jul 03, 2018 at 03:38:43PM -0700, Jakub Kicinski wrote:

On Tue, 3 Jul 2018 15:23:31 -0700, Jakub Kicinski wrote:

quoted

quoted

quoted

quoted

+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));    

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.    

Okay that makes sense  

Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
it may actually be more likely to just be used as a raw byte buffer...

Actually, what is the definition/purpose of BTF_INT_CHAR?  There seems
to be no BTF_INT_SHORT and BTF_INT_SIGNED can simply be of size 8...
Is normal int only used for bitfields of size 8 and BTF_INT_CHAR for
char variables?

The kernel seems to be rejecting combinations of those flags, is
unsigned char going to not be marked as char then?

BTF_INT_ENOCODING (CHAR/SIGNED/BOOL) is for formatting (e.g. pretty
print).  It is mainly how CTF is using it also.  Hence, BTF_INT_ENCODINGs
is not a 1:1 mapping to C integer types.
The size of an interger is described by BTF_INT_BITS instead.  

quoted

Either way I think it may be nice to keep it consistent, at least for
the JSON output could we do either always ints or always characters?

hi,

On Tue, Jul 03, 2018 at 03:23:31PM -0700, Jakub Kicinski wrote:

On Tue, 3 Jul 2018 22:46:00 +0100, Okash Khawaja wrote:

quoted

On Mon, Jul 02, 2018 at 10:06:59PM -0700, Jakub Kicinski wrote:

quoted

On Mon, 2 Jul 2018 11:39:15 -0700, Okash Khawaja wrote:  

quoted

+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)  

Perhaps it's just me but BIT_OFFSET or BIT_COUNT as a name of this macro
would make it more obvious to parse in the code below.  

I don't mind either. However these macro names are also used inside
kernel for same purpose. For sake of consistency, I'd recommend we keep
them :)

Ugh, okay :)

quoted

quoted

quoted

+	} print_num;
+
+	total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+	bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+	bits_to_copy = bits + bit_offset;
+	bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
+
+	print_num.u64_num = 0;
+	memcpy(&print_num.u64_num, data, bytes_to_copy);  

This scheme is unlikely to work on big endian machines...  

Can you give an example how?

On BE:

Input:         [0x01, 0x82]
Bit length:    15
Bytes to copy:  2
bit_offset:     0
upper_bits:     7

print_num.u64_num = 0;
# [0, 0, 0, 0,   0, 0, 0, 0]

memcpy(&print_num.u64_num, data, bytes_to_copy);  
# [0x01, 0x82, 0, 0,   0, 0, 0, 0]

mask = (1 << upper_bits) - 1;
# mask = 0x7f

print_num.u8_nums[bytes_to_copy - 1] &= mask;
# [0x01, 0x02, 0, 0,   0, 0, 0, 0]

printf("0x%llx", print_num.u64_num);
# 0x0102000000000000 AKA 72620543991349248
# expected:
# 0x0102             AKA 258

Am I missing something?

yes you're right, good catch! i'll fix this. thanks vrey much :)

quoted

quoted

quoted

+	upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
+	if (upper_bits) {
+		uint8_t mask = (1 << upper_bits) - 1;
+
+		print_num.u8_nums[bytes_to_copy - 1] &= mask;
+	}
+
+	print_num.u64_num >>= bit_offset;
+
+	if (is_plain_text)
+		jsonw_printf(jw, "0x%llx", print_num.u64_num);
+	else
+		jsonw_printf(jw, "%llu", print_num.u64_num);
+}
+
+static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
+			  const void *data, json_writer_t *jw,
+			  bool is_plain_text)
+{
+	uint32_t *int_type = (uint32_t *)(t + 1);
+	uint32_t bits = BTF_INT_BITS(*int_type);
+	int ret = 0;
+
+	/* if this is bit field */
+	if (bit_offset || BTF_INT_OFFSET(*int_type) ||
+	    BITS_PER_BYTE_MASKED(bits)) {
+		btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+				    is_plain_text);
+		return ret;
+	}
+
+	switch (BTF_INT_ENCODING(*int_type)) {
+	case 0:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%lu", *((uint64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%u", *((uint32_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 16)
+			jsonw_printf(jw, "%hu", *((uint16_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 8)
+			jsonw_printf(jw, "%hhu", *((uint8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_SIGNED:
+		if (BTF_INT_BITS(*int_type) == 64)
+			jsonw_printf(jw, "%ld", *((int64_t *)data));
+		else if (BTF_INT_BITS(*int_type) == 32)
+			jsonw_printf(jw, "%d", *((int32_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  16)  

Please drop the double space.  Both for 16 where it makes no sense and
for 8 where it's marginally useful but not really.
  

quoted

+			jsonw_printf(jw, "%hd", *((int16_t *)data));
+		else if (BTF_INT_BITS(*int_type) ==  8)
+			jsonw_printf(jw, "%hhd", *((int8_t *)data));
+		else
+			btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+					    is_plain_text);
+		break;
+	case BTF_INT_CHAR:
+		if (*((char *)data) == '\0')
+			jsonw_null(jw);  

Mm.. I don't think 0 char is equivalent to null.  

Yes, thanks. Will fix.

quoted

quoted

+		else if (isprint(*((char *)data)))
+			jsonw_printf(jw, "\"%c\"", *((char *)data));  

This looks very suspicious.  So if I see a "6" for a char field it's
either a 6 ('\u0006') or a 54 ('6')...  

It will always be 54. May be I missed your point. Could you explain why
it would be other than 54?

Ah, I think I missed that %c is in quotes...

quoted

quoted

quoted

+		else
+			if (is_plain_text)
+				jsonw_printf(jw, "%hhx", *((char *)data));

This seems to be missing a "0x" prefix?

yes it does. will add 0x.

quoted

quoted

quoted

+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));  

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.  

Okay that makes sense

Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
it may actually be more likely to just be used as a raw byte buffer...
Either way I think it may be nice to keep it consistent, at least for
the JSON output could we do either always ints or always characters?

yes, makes sense. i'll keep them always characters.

On Tue, Jul 03, 2018 at 04:33:50PM -0700, Martin KaFai Lau wrote:

On Tue, Jul 03, 2018 at 03:38:43PM -0700, Jakub Kicinski wrote:

quoted

On Tue, 3 Jul 2018 15:23:31 -0700, Jakub Kicinski wrote:

quoted

quoted

quoted

quoted

+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));    

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.    

Okay that makes sense  

Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
it may actually be more likely to just be used as a raw byte buffer...

Actually, what is the definition/purpose of BTF_INT_CHAR?  There seems
to be no BTF_INT_SHORT and BTF_INT_SIGNED can simply be of size 8...
Is normal int only used for bitfields of size 8 and BTF_INT_CHAR for
char variables?

The kernel seems to be rejecting combinations of those flags, is
unsigned char going to not be marked as char then?

BTF_INT_ENOCODING (CHAR/SIGNED/BOOL) is for formatting (e.g. pretty
print).  It is mainly how CTF is using it also.  Hence, BTF_INT_ENCODINGs
is not a 1:1 mapping to C integer types.
The size of an interger is described by BTF_INT_BITS instead.  

quoted

quoted

Either way I think it may be nice to keep it consistent, at least for
the JSON output could we do either always ints or always characters?

for !isprint() case, will "\x%02hhx" make more sense?

On Sat, 7 Jul 2018 14:30:58 +0100, Okash Khawaja wrote:

On Tue, Jul 03, 2018 at 04:33:50PM -0700, Martin KaFai Lau wrote:

quoted

On Tue, Jul 03, 2018 at 03:38:43PM -0700, Jakub Kicinski wrote:  

quoted

On Tue, 3 Jul 2018 15:23:31 -0700, Jakub Kicinski wrote:  

quoted

quoted

quoted

quoted

+			else
+				jsonw_printf(jw, "%hhd", *((char *)data));      

... I think you need to always print a string, and express it as
\u00%02hhx for non-printable.      

Okay that makes sense    

Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
it may actually be more likely to just be used as a raw byte buffer...  

Actually, what is the definition/purpose of BTF_INT_CHAR?  There seems
to be no BTF_INT_SHORT and BTF_INT_SIGNED can simply be of size 8...
Is normal int only used for bitfields of size 8 and BTF_INT_CHAR for
char variables?

The kernel seems to be rejecting combinations of those flags, is
unsigned char going to not be marked as char then?  

BTF_INT_ENOCODING (CHAR/SIGNED/BOOL) is for formatting (e.g. pretty
print).  It is mainly how CTF is using it also.  Hence, BTF_INT_ENCODINGs
is not a 1:1 mapping to C integer types.
The size of an interger is described by BTF_INT_BITS instead.  
  

quoted

quoted

Either way I think it may be nice to keep it consistent, at least for
the JSON output could we do either always ints or always characters?  

for !isprint() case, will "\x%02hhx" make more sense?

According to (quick look over) the JSON standard \x%02hhx is not a
valid escape sequence, everything has to be Unicode, so \u00%02hhx.
JSON validators online agree seem to reject \x as well.