* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-20 20:30 ` [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality Okash Khawaja
@ 2018-06-20 23:14 ` Song Liu
2018-06-21 10:31 ` Okash Khawaja
2018-06-21 10:42 ` Quentin Monnet
2018-06-21 21:59 ` Jakub Kicinski
2 siblings, 1 reply; 45+ messages in thread
From: Song Liu @ 2018-06-20 23:14 UTC (permalink / raw)
To: Okash Khawaja
Cc: Daniel Borkmann, Martin Lau, Alexei Starovoitov, Yonghong Song,
Quentin Monnet, Jakub Kicinski, David S. Miller, netdev,
Kernel Team, linux-kernel
> On Jun 20, 2018, at 1:30 PM, Okash Khawaja <osk@fb.com> wrote:
>
> 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 -p 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": 0x7ffff6f70568,
> "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 <osk@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> ---
> tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
> tools/bpf/bpftool/btf_dumper.h | 18 ++
> 2 files changed, 265 insertions(+)
>
> --- /dev/null
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -0,0 +1,247 @@
> +/* 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"
> +
> +#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 *btf, uint32_t type_id,
> + uint8_t bit_offset, const void *data, json_writer_t *jw);
> +
> +static void btf_dumper_ptr(const void *data, json_writer_t *jw)
> +{
> + jsonw_printf(jw, "%p", *((uintptr_t *)data));
> +}
> +
> +static int btf_dumper_modifier(const struct btf *btf, uint32_t type_id,
> + const void *data, json_writer_t *jw)
> +{
> + int32_t actual_type_id = btf__resolve_type(btf, type_id);
> + int ret;
> +
> + if (actual_type_id < 0)
> + return actual_type_id;
> +
> + ret = btf_dumper_do_type(btf, actual_type_id, 0, data, jw);
> +
> + 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 *btf, uint32_t type_id,
> + const void *data, json_writer_t *jw)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, type_id);
> + struct btf_array *arr = (struct btf_array *)(t + 1);
> + int64_t elem_size;
> + uint32_t i;
> + int ret;
> +
> + elem_size = btf__resolve_size(btf, arr->type);
> + if (elem_size < 0)
> + return elem_size;
> +
> + jsonw_start_array(jw);
> + for (i = 0; i < arr->nelems; i++) {
> + ret = btf_dumper_do_type(btf, arr->type, 0,
> + data + (i * elem_size), jw);
> + if (ret)
> + return ret;
Shall we call jsonw_end_array() before return here?
> + }
> +
> + jsonw_end_array(jw);
> +
> + return 0;
> +}
> +
> +static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
> + const void *data, json_writer_t *jw)
> +{
> + uint16_t total_bits_offset;
> + uint32_t bits = BTF_INT_BITS(int_type);
> + 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;
> +
> + jsonw_printf(jw, "0x%llx", 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)
> +{
> + 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);
> + 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);
> + 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);
> + break;
> + case BTF_INT_CHAR:
> + if (*((char *)data) == '\0')
> + jsonw_null(jw);
> + else if (isprint(*((char *)data)))
> + jsonw_printf(jw, "\"%c\"", *((char *)data));
> + else
> + jsonw_printf(jw, "%hhx", *((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 *btf, uint32_t type_id,
> + const void *data, json_writer_t *jw)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, type_id);
> + struct btf_member *m;
> + int ret = 0;
> + int i, vlen;
> +
> + if (t == NULL)
> + return -EINVAL;
> +
> + vlen = BTF_INFO_VLEN(t->info);
> + jsonw_start_object(jw);
> + m = (struct btf_member *)(t + 1);
> +
> + for (i = 0; i < vlen; i++) {
> + jsonw_name(jw, btf__name_by_offset(btf, m[i].name_off));
> + ret = btf_dumper_do_type(btf, m[i].type,
> + BITS_PER_BYTE_MASKED(m[i].offset),
> + data + BITS_ROUNDDOWN_BYTES(m[i].offset), jw);
> + if (ret)
> + return ret;
Shall we call jsonw_end_object() before return here?
> + }
> +
> + jsonw_end_object(jw);
> +
> + return 0;
> +}
> +
> +static int btf_dumper_do_type(const struct btf *btf, uint32_t type_id,
> + uint8_t bit_offset, const void *data, json_writer_t *jw)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, type_id);
> + int ret = 0;
> +
> + switch (BTF_INFO_KIND(t->info)) {
> + case BTF_KIND_INT:
> + ret = btf_dumper_int(t, bit_offset, data, jw);
> + break;
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION:
> + ret = btf_dumper_struct(btf, type_id, data, jw);
> + break;
> + case BTF_KIND_ARRAY:
> + ret = btf_dumper_array(btf, type_id, data, jw);
> + break;
> + case BTF_KIND_ENUM:
> + btf_dumper_enum(data, jw);
> + break;
> + case BTF_KIND_PTR:
> + btf_dumper_ptr(data, jw);
> + break;
> + case BTF_KIND_UNKN:
> + jsonw_printf(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(btf, type_id, data, jw);
> + break;
> + default:
> + jsonw_printf(jw, "(unsupported-kind");
"(unsupported-kind)" (missing ')').
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +int32_t btf_dumper_type(const struct btf *btf, json_writer_t *jw,
> + uint32_t type_id, const void *data)
> +{
> + if (!jw)
> + return -EINVAL;
> +
> + return btf_dumper_do_type(btf, type_id, 0, data, jw);
nit: btf_dumper_do_type() returns int, while btf_dumper_type() returns int32_t.
This should not matter though.
> +}
> --- /dev/null
> +++ b/tools/bpf/bpftool/btf_dumper.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018 Facebook */
> +
> +#ifndef BTF_DUMPER_H
> +#define BTF_DUMPER_H
> +
> +/* btf_dumper_type - json print data along with type information
> + * @btf: btf instance initialised via btf__new()
> + * @jw: json writer used for printing
> + * @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 *btf, json_writer_t *jw,
> + uint32_t type_id, void *data);
> +
> +#endif
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-20 23:14 ` Song Liu
@ 2018-06-21 10:31 ` Okash Khawaja
0 siblings, 0 replies; 45+ messages in thread
From: Okash Khawaja @ 2018-06-21 10:31 UTC (permalink / raw)
To: Song Liu
Cc: Daniel Borkmann, Martin Lau, Alexei Starovoitov, Yonghong Song,
Quentin Monnet, Jakub Kicinski, David S. Miller, netdev,
Kernel Team, linux-kernel
On Thu, Jun 21, 2018 at 12:14:33AM +0100, Song Liu wrote:
>
>
> > On Jun 20, 2018, at 1:30 PM, Okash Khawaja <osk@fb.com> wrote:
> >
> > 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 -p 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": 0x7ffff6f70568,
> > "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 <osk@fb.com>
> > Acked-by: Martin KaFai Lau <kafai@fb.com>
> >
> > ---
> > tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
> > tools/bpf/bpftool/btf_dumper.h | 18 ++
> > 2 files changed, 265 insertions(+)
> >
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/btf_dumper.c
> > @@ -0,0 +1,247 @@
> > +/* 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"
> > +
> > +#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 *btf, uint32_t type_id,
> > + uint8_t bit_offset, const void *data, json_writer_t *jw);
> > +
> > +static void btf_dumper_ptr(const void *data, json_writer_t *jw)
> > +{
> > + jsonw_printf(jw, "%p", *((uintptr_t *)data));
> > +}
> > +
> > +static int btf_dumper_modifier(const struct btf *btf, uint32_t type_id,
> > + const void *data, json_writer_t *jw)
> > +{
> > + int32_t actual_type_id = btf__resolve_type(btf, type_id);
> > + int ret;
> > +
> > + if (actual_type_id < 0)
> > + return actual_type_id;
> > +
> > + ret = btf_dumper_do_type(btf, actual_type_id, 0, data, jw);
> > +
> > + 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 *btf, uint32_t type_id,
> > + const void *data, json_writer_t *jw)
> > +{
> > + const struct btf_type *t = btf__type_by_id(btf, type_id);
> > + struct btf_array *arr = (struct btf_array *)(t + 1);
> > + int64_t elem_size;
> > + uint32_t i;
> > + int ret;
> > +
> > + elem_size = btf__resolve_size(btf, arr->type);
> > + if (elem_size < 0)
> > + return elem_size;
> > +
> > + jsonw_start_array(jw);
> > + for (i = 0; i < arr->nelems; i++) {
> > + ret = btf_dumper_do_type(btf, arr->type, 0,
> > + data + (i * elem_size), jw);
> > + if (ret)
> > + return ret;
>
> Shall we call jsonw_end_array() before return here?
Thanks. I'll update the patch.
>
> > + }
> > +
> > + jsonw_end_array(jw);
> > +
> > + return 0;
> > +}
> > +
> > +static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
> > + const void *data, json_writer_t *jw)
> > +{
> > + uint16_t total_bits_offset;
> > + uint32_t bits = BTF_INT_BITS(int_type);
> > + 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;
> > +
> > + jsonw_printf(jw, "0x%llx", 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)
> > +{
> > + 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);
> > + 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);
> > + 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);
> > + break;
> > + case BTF_INT_CHAR:
> > + if (*((char *)data) == '\0')
> > + jsonw_null(jw);
> > + else if (isprint(*((char *)data)))
> > + jsonw_printf(jw, "\"%c\"", *((char *)data));
> > + else
> > + jsonw_printf(jw, "%hhx", *((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 *btf, uint32_t type_id,
> > + const void *data, json_writer_t *jw)
> > +{
> > + const struct btf_type *t = btf__type_by_id(btf, type_id);
> > + struct btf_member *m;
> > + int ret = 0;
> > + int i, vlen;
> > +
> > + if (t == NULL)
> > + return -EINVAL;
> > +
> > + vlen = BTF_INFO_VLEN(t->info);
> > + jsonw_start_object(jw);
> > + m = (struct btf_member *)(t + 1);
> > +
> > + for (i = 0; i < vlen; i++) {
> > + jsonw_name(jw, btf__name_by_offset(btf, m[i].name_off));
> > + ret = btf_dumper_do_type(btf, m[i].type,
> > + BITS_PER_BYTE_MASKED(m[i].offset),
> > + data + BITS_ROUNDDOWN_BYTES(m[i].offset), jw);
> > + if (ret)
> > + return ret;
>
> Shall we call jsonw_end_object() before return here?
>
> > + }
> > +
> > + jsonw_end_object(jw);
> > +
> > + return 0;
> > +}
> > +
> > +static int btf_dumper_do_type(const struct btf *btf, uint32_t type_id,
> > + uint8_t bit_offset, const void *data, json_writer_t *jw)
> > +{
> > + const struct btf_type *t = btf__type_by_id(btf, type_id);
> > + int ret = 0;
> > +
> > + switch (BTF_INFO_KIND(t->info)) {
> > + case BTF_KIND_INT:
> > + ret = btf_dumper_int(t, bit_offset, data, jw);
> > + break;
> > + case BTF_KIND_STRUCT:
> > + case BTF_KIND_UNION:
> > + ret = btf_dumper_struct(btf, type_id, data, jw);
> > + break;
> > + case BTF_KIND_ARRAY:
> > + ret = btf_dumper_array(btf, type_id, data, jw);
> > + break;
> > + case BTF_KIND_ENUM:
> > + btf_dumper_enum(data, jw);
> > + break;
> > + case BTF_KIND_PTR:
> > + btf_dumper_ptr(data, jw);
> > + break;
> > + case BTF_KIND_UNKN:
> > + jsonw_printf(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(btf, type_id, data, jw);
> > + break;
> > + default:
> > + jsonw_printf(jw, "(unsupported-kind");
>
> "(unsupported-kind)" (missing ')').
Thank you, will update the patch.
>
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +int32_t btf_dumper_type(const struct btf *btf, json_writer_t *jw,
> > + uint32_t type_id, const void *data)
> > +{
> > + if (!jw)
> > + return -EINVAL;
> > +
> > + return btf_dumper_do_type(btf, type_id, 0, data, jw);
> nit: btf_dumper_do_type() returns int, while btf_dumper_type() returns int32_t.
> This should not matter though.
Probably an oversight from me. I'll make them consistent. Thank you.
>
> > +}
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/btf_dumper.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (c) 2018 Facebook */
> > +
> > +#ifndef BTF_DUMPER_H
> > +#define BTF_DUMPER_H
> > +
> > +/* btf_dumper_type - json print data along with type information
> > + * @btf: btf instance initialised via btf__new()
> > + * @jw: json writer used for printing
> > + * @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 *btf, json_writer_t *jw,
> > + uint32_t type_id, void *data);
> > +
> > +#endif
> >
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-20 20:30 ` [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality Okash Khawaja
2018-06-20 23:14 ` Song Liu
@ 2018-06-21 10:42 ` Quentin Monnet
2018-06-22 10:24 ` Okash Khawaja
2018-06-21 21:59 ` Jakub Kicinski
2 siblings, 1 reply; 45+ messages in thread
From: Quentin Monnet @ 2018-06-21 10:42 UTC (permalink / raw)
To: Okash Khawaja, Daniel Borkmann, Martin KaFai Lau,
Alexei Starovoitov, Yonghong Song, Jakub Kicinski,
David S. Miller
Cc: netdev, kernel-team, linux-kernel
Hi Okash,
2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@fb.com>
> 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 -p 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": 0x7ffff6f70568,
Hexadecimal values, without quotes, are not valid JSON. Please stick to
decimal values.
> "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 <osk@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> ---
> tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
> tools/bpf/bpftool/btf_dumper.h | 18 ++
> 2 files changed, 265 insertions(+)
>
> --- /dev/null
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -0,0 +1,247 @@
> +/* 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"
> +
> +#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 *btf, uint32_t type_id,
> + uint8_t bit_offset, const void *data, json_writer_t *jw);
> +
> +static void btf_dumper_ptr(const void *data, json_writer_t *jw)
> +{
> + jsonw_printf(jw, "%p", *((uintptr_t *)data));
> +}
> +
> +static int btf_dumper_modifier(const struct btf *btf, uint32_t type_id,
> + const void *data, json_writer_t *jw)
> +{
> + int32_t actual_type_id = btf__resolve_type(btf, type_id);
> + int ret;
> +
> + if (actual_type_id < 0)
> + return actual_type_id;
> +
> + ret = btf_dumper_do_type(btf, actual_type_id, 0, data, jw);
> +
> + 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 *btf, uint32_t type_id,
> + const void *data, json_writer_t *jw)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, type_id);
> + struct btf_array *arr = (struct btf_array *)(t + 1);
> + int64_t elem_size;
> + uint32_t i;
> + int ret;
> +
> + elem_size = btf__resolve_size(btf, arr->type);
> + if (elem_size < 0)
> + return elem_size;
> +
> + jsonw_start_array(jw);
> + for (i = 0; i < arr->nelems; i++) {
> + ret = btf_dumper_do_type(btf, arr->type, 0,
> + data + (i * elem_size), jw);
> + if (ret)
> + return ret;
> + }
> +
> + jsonw_end_array(jw);
> +
> + return 0;
> +}
> +
> +static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
> + const void *data, json_writer_t *jw)
> +{
> + uint16_t total_bits_offset;
> + uint32_t bits = BTF_INT_BITS(int_type);
Nit: Please use reverse-Christmas-tree here.
As for patch 3 you also have a number of continuation lines not aligned
on the opening parenthesis from the first line, throughout the patch
(please consider running checkpatch in "--strict" mode for the list).
> + 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;
> +
> + jsonw_printf(jw, "0x%llx", 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)
> +{
> + 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);
> + 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);
> + 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);
> + break;
> + case BTF_INT_CHAR:
> + if (*((char *)data) == '\0')
> + jsonw_null(jw);
> + else if (isprint(*((char *)data)))
> + jsonw_printf(jw, "\"%c\"", *((char *)data));
> + else
> + jsonw_printf(jw, "%hhx", *((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 *btf, uint32_t type_id,
> + const void *data, json_writer_t *jw)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, type_id);
> + struct btf_member *m;
> + int ret = 0;
> + int i, vlen;
> +
> + if (t == NULL)
> + return -EINVAL;
> +
> + vlen = BTF_INFO_VLEN(t->info);
> + jsonw_start_object(jw);
> + m = (struct btf_member *)(t + 1);
> +
> + for (i = 0; i < vlen; i++) {
> + jsonw_name(jw, btf__name_by_offset(btf, m[i].name_off));
> + ret = btf_dumper_do_type(btf, m[i].type,
> + BITS_PER_BYTE_MASKED(m[i].offset),
> + data + BITS_ROUNDDOWN_BYTES(m[i].offset), jw);
> + if (ret)
> + return ret;
> + }
> +
> + jsonw_end_object(jw);
> +
> + return 0;
> +}
> +
> +static int btf_dumper_do_type(const struct btf *btf, uint32_t type_id,
> + uint8_t bit_offset, const void *data, json_writer_t *jw)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, type_id);
> + int ret = 0;
> +
> + switch (BTF_INFO_KIND(t->info)) {
> + case BTF_KIND_INT:
> + ret = btf_dumper_int(t, bit_offset, data, jw);
> + break;
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION:
> + ret = btf_dumper_struct(btf, type_id, data, jw);
> + break;
> + case BTF_KIND_ARRAY:
> + ret = btf_dumper_array(btf, type_id, data, jw);
> + break;
> + case BTF_KIND_ENUM:
> + btf_dumper_enum(data, jw);
> + break;
> + case BTF_KIND_PTR:
> + btf_dumper_ptr(data, jw);
> + break;
> + case BTF_KIND_UNKN:
> + jsonw_printf(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(btf, type_id, data, jw);
> + break;
> + default:
> + jsonw_printf(jw, "(unsupported-kind");
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +int32_t btf_dumper_type(const struct btf *btf, json_writer_t *jw,
> + uint32_t type_id, const void *data)
> +{
> + if (!jw)
> + return -EINVAL;
> +
> + return btf_dumper_do_type(btf, type_id, 0, data, jw);
> +}
> --- /dev/null
> +++ b/tools/bpf/bpftool/btf_dumper.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
I believe SPDX tag in header files should use C++ style comments (//)?
> +/* Copyright (c) 2018 Facebook */
> +
> +#ifndef BTF_DUMPER_H
> +#define BTF_DUMPER_H
> +
> +/* btf_dumper_type - json print data along with type information
> + * @btf: btf instance initialised via btf__new()
> + * @jw: json writer used for printing
> + * @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 *btf, json_writer_t *jw,
> + uint32_t type_id, void *data);
> +
> +#endif
>
Thanks,
Quentin
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-21 10:42 ` Quentin Monnet
@ 2018-06-22 10:24 ` Okash Khawaja
2018-06-22 10:39 ` Quentin Monnet
0 siblings, 1 reply; 45+ messages in thread
From: Okash Khawaja @ 2018-06-22 10:24 UTC (permalink / raw)
To: Quentin Monnet
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Jakub Kicinski, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, Jun 21, 2018 at 11:42:59AM +0100, Quentin Monnet wrote:
> Hi Okash,
hi and sorry about delay in responding. the email got routed to
incorrect folder.
>
> 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@fb.com>
> > 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 -p 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": 0x7ffff6f70568,
>
> Hexadecimal values, without quotes, are not valid JSON. Please stick to
> decimal values.
ah sorry, i used a buggy json validator. this should be a quick fix.
which would be better: pointers be output hex strings or integers?
>
> > "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 <osk@fb.com>
> > Acked-by: Martin KaFai Lau <kafai@fb.com>
> >
> > ---
> > tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
> > tools/bpf/bpftool/btf_dumper.h | 18 ++
> > 2 files changed, 265 insertions(+)
> >
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/btf_dumper.c
> > @@ -0,0 +1,247 @@
> > +/* 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"
> > +
> > +#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 *btf, uint32_t type_id,
> > + uint8_t bit_offset, const void *data, json_writer_t *jw);
> > +
> > +static void btf_dumper_ptr(const void *data, json_writer_t *jw)
> > +{
> > + jsonw_printf(jw, "%p", *((uintptr_t *)data));
> > +}
> > +
> > +static int btf_dumper_modifier(const struct btf *btf, uint32_t type_id,
> > + const void *data, json_writer_t *jw)
> > +{
> > + int32_t actual_type_id = btf__resolve_type(btf, type_id);
> > + int ret;
> > +
> > + if (actual_type_id < 0)
> > + return actual_type_id;
> > +
> > + ret = btf_dumper_do_type(btf, actual_type_id, 0, data, jw);
> > +
> > + 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 *btf, uint32_t type_id,
> > + const void *data, json_writer_t *jw)
> > +{
> > + const struct btf_type *t = btf__type_by_id(btf, type_id);
> > + struct btf_array *arr = (struct btf_array *)(t + 1);
> > + int64_t elem_size;
> > + uint32_t i;
> > + int ret;
> > +
> > + elem_size = btf__resolve_size(btf, arr->type);
> > + if (elem_size < 0)
> > + return elem_size;
> > +
> > + jsonw_start_array(jw);
> > + for (i = 0; i < arr->nelems; i++) {
> > + ret = btf_dumper_do_type(btf, arr->type, 0,
> > + data + (i * elem_size), jw);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + jsonw_end_array(jw);
> > +
> > + return 0;
> > +}
> > +
> > +static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
> > + const void *data, json_writer_t *jw)
> > +{
> > + uint16_t total_bits_offset;
> > + uint32_t bits = BTF_INT_BITS(int_type);
>
> Nit: Please use reverse-Christmas-tree here.
>
> As for patch 3 you also have a number of continuation lines not aligned
> on the opening parenthesis from the first line, throughout the patch
> (please consider running checkpatch in "--strict" mode for the list).
i didn't use "--strict". that would explain style mismatch. will fix
that in v2.
>
> > + 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;
> > +
> > + jsonw_printf(jw, "0x%llx", 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)
> > +{
> > + 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);
> > + 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);
> > + 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);
> > + break;
> > + case BTF_INT_CHAR:
> > + if (*((char *)data) == '\0')
> > + jsonw_null(jw);
> > + else if (isprint(*((char *)data)))
> > + jsonw_printf(jw, "\"%c\"", *((char *)data));
> > + else
> > + jsonw_printf(jw, "%hhx", *((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 *btf, uint32_t type_id,
> > + const void *data, json_writer_t *jw)
> > +{
> > + const struct btf_type *t = btf__type_by_id(btf, type_id);
> > + struct btf_member *m;
> > + int ret = 0;
> > + int i, vlen;
> > +
> > + if (t == NULL)
> > + return -EINVAL;
> > +
> > + vlen = BTF_INFO_VLEN(t->info);
> > + jsonw_start_object(jw);
> > + m = (struct btf_member *)(t + 1);
> > +
> > + for (i = 0; i < vlen; i++) {
> > + jsonw_name(jw, btf__name_by_offset(btf, m[i].name_off));
> > + ret = btf_dumper_do_type(btf, m[i].type,
> > + BITS_PER_BYTE_MASKED(m[i].offset),
> > + data + BITS_ROUNDDOWN_BYTES(m[i].offset), jw);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + jsonw_end_object(jw);
> > +
> > + return 0;
> > +}
> > +
> > +static int btf_dumper_do_type(const struct btf *btf, uint32_t type_id,
> > + uint8_t bit_offset, const void *data, json_writer_t *jw)
> > +{
> > + const struct btf_type *t = btf__type_by_id(btf, type_id);
> > + int ret = 0;
> > +
> > + switch (BTF_INFO_KIND(t->info)) {
> > + case BTF_KIND_INT:
> > + ret = btf_dumper_int(t, bit_offset, data, jw);
> > + break;
> > + case BTF_KIND_STRUCT:
> > + case BTF_KIND_UNION:
> > + ret = btf_dumper_struct(btf, type_id, data, jw);
> > + break;
> > + case BTF_KIND_ARRAY:
> > + ret = btf_dumper_array(btf, type_id, data, jw);
> > + break;
> > + case BTF_KIND_ENUM:
> > + btf_dumper_enum(data, jw);
> > + break;
> > + case BTF_KIND_PTR:
> > + btf_dumper_ptr(data, jw);
> > + break;
> > + case BTF_KIND_UNKN:
> > + jsonw_printf(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(btf, type_id, data, jw);
> > + break;
> > + default:
> > + jsonw_printf(jw, "(unsupported-kind");
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +int32_t btf_dumper_type(const struct btf *btf, json_writer_t *jw,
> > + uint32_t type_id, const void *data)
> > +{
> > + if (!jw)
> > + return -EINVAL;
> > +
> > + return btf_dumper_do_type(btf, type_id, 0, data, jw);
> > +}
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/btf_dumper.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> I believe SPDX tag in header files should use C++ style comments (//)?
okay i will verify and fix that.
>
> > +/* Copyright (c) 2018 Facebook */
> > +
> > +#ifndef BTF_DUMPER_H
> > +#define BTF_DUMPER_H
> > +
> > +/* btf_dumper_type - json print data along with type information
> > + * @btf: btf instance initialised via btf__new()
> > + * @jw: json writer used for printing
> > + * @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 *btf, json_writer_t *jw,
> > + uint32_t type_id, void *data);
> > +
> > +#endif
> >
>
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 10:24 ` Okash Khawaja
@ 2018-06-22 10:39 ` Quentin Monnet
2018-06-22 18:44 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Quentin Monnet @ 2018-06-22 10:39 UTC (permalink / raw)
To: Okash Khawaja
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Jakub Kicinski, David S. Miller, netdev,
kernel-team, linux-kernel
2018-06-22 11:24 UTC+0100 ~ Okash Khawaja <osk@fb.com>
> On Thu, Jun 21, 2018 at 11:42:59AM +0100, Quentin Monnet wrote:
>> Hi Okash,
> hi and sorry about delay in responding. the email got routed to
> incorrect folder.
>>
>> 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@fb.com>
>>> 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 -p 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": 0x7ffff6f70568,
>>
>> Hexadecimal values, without quotes, are not valid JSON. Please stick to
>> decimal values.
> ah sorry, i used a buggy json validator. this should be a quick fix.
> which would be better: pointers be output hex strings or integers?
I would go for integers. Although this is harder to read for humans, it
is easier to process for machines, which remain the primary targets for
JSON output.
Quentin
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 10:39 ` Quentin Monnet
@ 2018-06-22 18:44 ` Jakub Kicinski
0 siblings, 0 replies; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 18:44 UTC (permalink / raw)
To: Quentin Monnet
Cc: Okash Khawaja, Daniel Borkmann, Martin KaFai Lau,
Alexei Starovoitov, Yonghong Song, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, 22 Jun 2018 11:39:13 +0100, Quentin Monnet wrote:
> 2018-06-22 11:24 UTC+0100 ~ Okash Khawaja <osk@fb.com>
> > On Thu, Jun 21, 2018 at 11:42:59AM +0100, Quentin Monnet wrote:
> >> Hi Okash,
> > hi and sorry about delay in responding. the email got routed to
> > incorrect folder.
> >>
> >> 2018-06-20 13:30 UTC-0700 ~ Okash Khawaja <osk@fb.com>
> [...]
> >>
> >> Hexadecimal values, without quotes, are not valid JSON. Please stick to
> >> decimal values.
> > ah sorry, i used a buggy json validator. this should be a quick fix.
> > which would be better: pointers be output hex strings or integers?
>
> I would go for integers. Although this is harder to read for humans, it
> is easier to process for machines, which remain the primary targets for
> JSON output.
+1
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-20 20:30 ` [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality Okash Khawaja
2018-06-20 23:14 ` Song Liu
2018-06-21 10:42 ` Quentin Monnet
@ 2018-06-21 21:59 ` Jakub Kicinski
2018-06-21 22:51 ` Martin KaFai Lau
2 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-21 21:59 UTC (permalink / raw)
To: Okash Khawaja
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> "t": {
> "x": 5,
> "y": 10
> },
> "u": 100,
> "v": 20,
> "w1": 0x7,
> "w2": 0x3
> }
> }
> ]
I don't think this format is okay, JSON output is an API you shouldn't
break. You can change the non-JSON output whatever way you like, but
JSON must remain backwards compatible.
The dump today has object per entry, e.g.:
{
"key":["0x00","0x00","0x00","0x00",
],
"value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
]
}
This format must remain, you may only augment it with new fields. E.g.:
{
"key":["0x00","0x00","0x00","0x00",
],
"key_struct":{
"index":0
},
"value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
],
"value_struct":{
"src_ip":2,
"dst_ip:0
}
}
The name XYZ_struct may not be the best, perhaps you can come up with a
better one?
Does that make sense? Am I missing what you're doing here?
One process note - please make sure you run checkpatch.pl --strict on
bpftool patches before posting.
Thanks for working on this!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-21 21:59 ` Jakub Kicinski
@ 2018-06-21 22:51 ` Martin KaFai Lau
2018-06-21 23:07 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-21 22:51 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > "t": {
> > "x": 5,
> > "y": 10
> > },
> > "u": 100,
> > "v": 20,
> > "w1": 0x7,
> > "w2": 0x3
> > }
> > }
> > ]
>
> I don't think this format is okay, JSON output is an API you shouldn't
> break. You can change the non-JSON output whatever way you like, but
> JSON must remain backwards compatible.
>
> The dump today has object per entry, e.g.:
>
> {
> "key":["0x00","0x00","0x00","0x00",
> ],
> "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> ]
> }
>
> This format must remain, you may only augment it with new fields. E.g.:
>
> {
> "key":["0x00","0x00","0x00","0x00",
> ],
> "key_struct":{
> "index":0
> },
> "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> ],
> "value_struct":{
> "src_ip":2,
> "dst_ip:0
> }
> }
I am not sure how useful to have both "key|value" and "(key|value)_struct"
while most people would prefer "key_struct"/"value_struct" if it is
available.
How about introducing a new option, like "-b", to print the
map with BTF (if available) such that it won't break the existing
one (-j or -p) while the "-b" output can keep using the "key"
and "value".
The existing json can be kept as is.
>
> The name XYZ_struct may not be the best, perhaps you can come up with a
> better one?
>
> Does that make sense? Am I missing what you're doing here?
>
> One process note - please make sure you run checkpatch.pl --strict on
> bpftool patches before posting.
>
> Thanks for working on this!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-21 22:51 ` Martin KaFai Lau
@ 2018-06-21 23:07 ` Jakub Kicinski
2018-06-21 23:58 ` Martin KaFai Lau
0 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-21 23:07 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > "t": {
> > > "x": 5,
> > > "y": 10
> > > },
> > > "u": 100,
> > > "v": 20,
> > > "w1": 0x7,
> > > "w2": 0x3
> > > }
> > > }
> > > ]
> >
> > I don't think this format is okay, JSON output is an API you shouldn't
> > break. You can change the non-JSON output whatever way you like, but
> > JSON must remain backwards compatible.
> >
> > The dump today has object per entry, e.g.:
> >
> > {
> > "key":["0x00","0x00","0x00","0x00",
> > ],
> > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > ]
> > }
> >
> > This format must remain, you may only augment it with new fields. E.g.:
> >
> > {
> > "key":["0x00","0x00","0x00","0x00",
> > ],
> > "key_struct":{
> > "index":0
> > },
> > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > ],
> > "value_struct":{
> > "src_ip":2,
> > "dst_ip:0
> > }
> > }
> I am not sure how useful to have both "key|value" and "(key|value)_struct"
> while most people would prefer "key_struct"/"value_struct" if it is
> available.
Agreed, it's not that useful, especially with the string-hex debacle :(
It's just about the backwards compat.
> How about introducing a new option, like "-b", to print the
> map with BTF (if available) such that it won't break the existing
> one (-j or -p) while the "-b" output can keep using the "key"
> and "value".
>
> The existing json can be kept as is.
That was my knee jerk reaction too, but on reflection it doesn't sound
that great. We expect people with new-enough bpftool to use btf, so it
should be available in the default output, without hiding it behind a
switch. We could add a switch to hide the old output, but that doesn't
give us back the names... What about Key and Value or k and v? Or
key_fields and value_fields?
> > The name XYZ_struct may not be the best, perhaps you can come up with a
> > better one?
> >
> > Does that make sense? Am I missing what you're doing here?
> >
> > One process note - please make sure you run checkpatch.pl --strict on
> > bpftool patches before posting.
> >
> > Thanks for working on this!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-21 23:07 ` Jakub Kicinski
@ 2018-06-21 23:58 ` Martin KaFai Lau
2018-06-22 0:25 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-21 23:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > "t": {
> > > > "x": 5,
> > > > "y": 10
> > > > },
> > > > "u": 100,
> > > > "v": 20,
> > > > "w1": 0x7,
> > > > "w2": 0x3
> > > > }
> > > > }
> > > > ]
> > >
> > > I don't think this format is okay, JSON output is an API you shouldn't
> > > break. You can change the non-JSON output whatever way you like, but
> > > JSON must remain backwards compatible.
> > >
> > > The dump today has object per entry, e.g.:
> > >
> > > {
> > > "key":["0x00","0x00","0x00","0x00",
> > > ],
> > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > ]
> > > }
> > >
> > > This format must remain, you may only augment it with new fields. E.g.:
> > >
> > > {
> > > "key":["0x00","0x00","0x00","0x00",
> > > ],
> > > "key_struct":{
> > > "index":0
> > > },
> > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > ],
> > > "value_struct":{
> > > "src_ip":2,
> > > "dst_ip:0
> > > }
> > > }
> > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > while most people would prefer "key_struct"/"value_struct" if it is
> > available.
>
> Agreed, it's not that useful, especially with the string-hex debacle :(
> It's just about the backwards compat.
>
> > How about introducing a new option, like "-b", to print the
> > map with BTF (if available) such that it won't break the existing
> > one (-j or -p) while the "-b" output can keep using the "key"
> > and "value".
> >
> > The existing json can be kept as is.
>
> That was my knee jerk reaction too, but on reflection it doesn't sound
> that great. We expect people with new-enough bpftool to use btf, so it
> should be available in the default output, without hiding it behind a
> switch. We could add a switch to hide the old output, but that doesn't
> give us back the names... What about Key and Value or k and v? Or
> key_fields and value_fields?
I thought the current default output is "plain" ;)
Having said that, yes, the btf is currently printed in json.
Ideally, the default json output should do what most people want:
print btf and btf only (if it is available).
but I don't see a way out without new option if we need to
be backward compat :(
Agree that showing the btf in the existing json output will be useful (e.g.
to hint people that BTF is available). If btf is showing in old json,
also agree that the names should be the same with the new json.
key_fields and value_fields may hint it has >1 fields though.
May be "formatted_key" and "formatted_value"?
>
> > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > better one?
> > >
> > > Does that make sense? Am I missing what you're doing here?
> > >
> > > One process note - please make sure you run checkpatch.pl --strict on
> > > bpftool patches before posting.
> > >
> > > Thanks for working on this!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-21 23:58 ` Martin KaFai Lau
@ 2018-06-22 0:25 ` Jakub Kicinski
2018-06-22 1:20 ` Martin KaFai Lau
0 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 0:25 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > "t": {
> > > > > "x": 5,
> > > > > "y": 10
> > > > > },
> > > > > "u": 100,
> > > > > "v": 20,
> > > > > "w1": 0x7,
> > > > > "w2": 0x3
> > > > > }
> > > > > }
> > > > > ]
> > > >
> > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > break. You can change the non-JSON output whatever way you like, but
> > > > JSON must remain backwards compatible.
> > > >
> > > > The dump today has object per entry, e.g.:
> > > >
> > > > {
> > > > "key":["0x00","0x00","0x00","0x00",
> > > > ],
> > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > ]
> > > > }
> > > >
> > > > This format must remain, you may only augment it with new fields. E.g.:
> > > >
> > > > {
> > > > "key":["0x00","0x00","0x00","0x00",
> > > > ],
> > > > "key_struct":{
> > > > "index":0
> > > > },
> > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > ],
> > > > "value_struct":{
> > > > "src_ip":2,
> > > > "dst_ip:0
> > > > }
> > > > }
> > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > while most people would prefer "key_struct"/"value_struct" if it is
> > > available.
> >
> > Agreed, it's not that useful, especially with the string-hex debacle :(
> > It's just about the backwards compat.
> >
> > > How about introducing a new option, like "-b", to print the
> > > map with BTF (if available) such that it won't break the existing
> > > one (-j or -p) while the "-b" output can keep using the "key"
> > > and "value".
> > >
> > > The existing json can be kept as is.
> >
> > That was my knee jerk reaction too, but on reflection it doesn't sound
> > that great. We expect people with new-enough bpftool to use btf, so it
> > should be available in the default output, without hiding it behind a
> > switch. We could add a switch to hide the old output, but that doesn't
> > give us back the names... What about Key and Value or k and v? Or
> > key_fields and value_fields?
> I thought the current default output is "plain" ;)
> Having said that, yes, the btf is currently printed in json.
>
> Ideally, the default json output should do what most people want:
> print btf and btf only (if it is available).
> but I don't see a way out without new option if we need to
> be backward compat :(
>
> Agree that showing the btf in the existing json output will be useful (e.g.
> to hint people that BTF is available). If btf is showing in old json,
> also agree that the names should be the same with the new json.
> key_fields and value_fields may hint it has >1 fields though.
> May be "formatted_key" and "formatted_value"?
SGTM! Or even maybe as a "formatted" object?:
{
"key":["0x00","0x00","0x00","0x00",
],
"value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
],
"formatted":{
"key":{
"index":0
},
"value":{
"src_ip":2,
"dst_ip:0
}
}
}
> > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > better one?
> > > >
> > > > Does that make sense? Am I missing what you're doing here?
> > > >
> > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > bpftool patches before posting.
> > > >
> > > > Thanks for working on this!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 0:25 ` Jakub Kicinski
@ 2018-06-22 1:20 ` Martin KaFai Lau
2018-06-22 11:17 ` Okash Khawaja
2018-06-22 18:40 ` Jakub Kicinski
0 siblings, 2 replies; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-22 1:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > > "t": {
> > > > > > "x": 5,
> > > > > > "y": 10
> > > > > > },
> > > > > > "u": 100,
> > > > > > "v": 20,
> > > > > > "w1": 0x7,
> > > > > > "w2": 0x3
> > > > > > }
> > > > > > }
> > > > > > ]
> > > > >
> > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > JSON must remain backwards compatible.
> > > > >
> > > > > The dump today has object per entry, e.g.:
> > > > >
> > > > > {
> > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > ],
> > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > ]
> > > > > }
> > > > >
> > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > >
> > > > > {
> > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > ],
> > > > > "key_struct":{
> > > > > "index":0
> > > > > },
Got a few questions.
When we support hashtab later, the key could be int
but reusing the name as "index" is weird.
The key could also be a struct (e.g. a struct to describe ip:port).
Can you suggest how the "key_struct" will look like?
> > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > ],
> > > > > "value_struct":{
> > > > > "src_ip":2,
If for the same map the user changes the "src_ip" to an array of int[4]
later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
Is it breaking backward compat?
i.e.
struct five_tuples {
- int src_ip;
+ int src_ip[4];
/* ... */
};
> > > > > "dst_ip:0
> > > > > }
> > > > > }
> > > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > > while most people would prefer "key_struct"/"value_struct" if it is
> > > > available.
> > >
> > > Agreed, it's not that useful, especially with the string-hex debacle :(
> > > It's just about the backwards compat.
> > >
> > > > How about introducing a new option, like "-b", to print the
> > > > map with BTF (if available) such that it won't break the existing
> > > > one (-j or -p) while the "-b" output can keep using the "key"
> > > > and "value".
> > > >
> > > > The existing json can be kept as is.
> > >
> > > That was my knee jerk reaction too, but on reflection it doesn't sound
> > > that great. We expect people with new-enough bpftool to use btf, so it
> > > should be available in the default output, without hiding it behind a
> > > switch. We could add a switch to hide the old output, but that doesn't
> > > give us back the names... What about Key and Value or k and v? Or
> > > key_fields and value_fields?
> > I thought the current default output is "plain" ;)
> > Having said that, yes, the btf is currently printed in json.
> >
> > Ideally, the default json output should do what most people want:
> > print btf and btf only (if it is available).
> > but I don't see a way out without new option if we need to
> > be backward compat :(
> >
> > Agree that showing the btf in the existing json output will be useful (e.g.
> > to hint people that BTF is available). If btf is showing in old json,
> > also agree that the names should be the same with the new json.
> > key_fields and value_fields may hint it has >1 fields though.
> > May be "formatted_key" and "formatted_value"?
>
> SGTM! Or even maybe as a "formatted" object?:
>
> {
> "key":["0x00","0x00","0x00","0x00",
> ],
> "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> ],
> "formatted":{
> "key":{
> "index":0
> },
> "value":{
> "src_ip":2,
> "dst_ip:0
> }
> }
hmm... that is an extra indentation (keep in mind that the "value" could
already have a few nested structs which itself consumes a few indentations)
but I guess adding another one may be ok-ish.
> }
>
> > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > better one?
> > > > >
> > > > > Does that make sense? Am I missing what you're doing here?
> > > > >
> > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > bpftool patches before posting.
> > > > >
> > > > > Thanks for working on this!
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 1:20 ` Martin KaFai Lau
@ 2018-06-22 11:17 ` Okash Khawaja
2018-06-22 18:43 ` Jakub Kicinski
2018-06-22 18:40 ` Jakub Kicinski
1 sibling, 1 reply; 45+ messages in thread
From: Okash Khawaja @ 2018-06-22 11:17 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Jakub Kicinski, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, Jun 21, 2018 at 06:20:52PM -0700, Martin KaFai Lau wrote:
> On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > > > "t": {
> > > > > > > "x": 5,
> > > > > > > "y": 10
> > > > > > > },
> > > > > > > "u": 100,
> > > > > > > "v": 20,
> > > > > > > "w1": 0x7,
> > > > > > > "w2": 0x3
> > > > > > > }
> > > > > > > }
> > > > > > > ]
> > > > > >
> > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > JSON must remain backwards compatible.
> > > > > >
> > > > > > The dump today has object per entry, e.g.:
> > > > > >
> > > > > > {
> > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > ],
> > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > ]
> > > > > > }
> > > > > >
> > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > >
> > > > > > {
> > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > ],
> > > > > > "key_struct":{
> > > > > > "index":0
> > > > > > },
> Got a few questions.
>
> When we support hashtab later, the key could be int
> but reusing the name as "index" is weird.
> The key could also be a struct (e.g. a struct to describe ip:port).
> Can you suggest how the "key_struct" will look like?
>
> > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > ],
> > > > > > "value_struct":{
> > > > > > "src_ip":2,
> If for the same map the user changes the "src_ip" to an array of int[4]
> later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> Is it breaking backward compat?
> i.e.
> struct five_tuples {
> - int src_ip;
> + int src_ip[4];
> /* ... */
> };
>
> > > > > > "dst_ip:0
> > > > > > }
> > > > > > }
> > > > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > > > while most people would prefer "key_struct"/"value_struct" if it is
> > > > > available.
> > > >
> > > > Agreed, it's not that useful, especially with the string-hex debacle :(
> > > > It's just about the backwards compat.
> > > >
> > > > > How about introducing a new option, like "-b", to print the
> > > > > map with BTF (if available) such that it won't break the existing
> > > > > one (-j or -p) while the "-b" output can keep using the "key"
> > > > > and "value".
> > > > >
> > > > > The existing json can be kept as is.
> > > >
> > > > That was my knee jerk reaction too, but on reflection it doesn't sound
> > > > that great. We expect people with new-enough bpftool to use btf, so it
> > > > should be available in the default output, without hiding it behind a
> > > > switch. We could add a switch to hide the old output, but that doesn't
> > > > give us back the names... What about Key and Value or k and v? Or
> > > > key_fields and value_fields?
> > > I thought the current default output is "plain" ;)
> > > Having said that, yes, the btf is currently printed in json.
> > >
> > > Ideally, the default json output should do what most people want:
> > > print btf and btf only (if it is available).
> > > but I don't see a way out without new option if we need to
> > > be backward compat :(
> > >
> > > Agree that showing the btf in the existing json output will be useful (e.g.
> > > to hint people that BTF is available). If btf is showing in old json,
> > > also agree that the names should be the same with the new json.
> > > key_fields and value_fields may hint it has >1 fields though.
> > > May be "formatted_key" and "formatted_value"?
> >
> > SGTM! Or even maybe as a "formatted" object?:
> >
> > {
> > "key":["0x00","0x00","0x00","0x00",
> > ],
> > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > ],
> > "formatted":{
> > "key":{
> > "index":0
> > },
> > "value":{
> > "src_ip":2,
> > "dst_ip:0
> > }
> > }
> hmm... that is an extra indentation (keep in mind that the "value" could
> already have a few nested structs which itself consumes a few indentations)
> but I guess adding another one may be ok-ish.
>
> > }
> >
> > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > better one?
> > > > > >
> > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > >
> > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > bpftool patches before posting.
> > > > > >
> > > > > > Thanks for working on this!
> >
Hi,
While I agree on the point of backward compatibility, I think printing
two overlapping pieces of information side-by-side will make the
interface less clear. Having separate outputs for the two will keep the
interface clear and readable.
Is there a major downside to adding a new flag for BTF output?
Thanks,
Okash
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 11:17 ` Okash Khawaja
@ 2018-06-22 18:43 ` Jakub Kicinski
0 siblings, 0 replies; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 18:43 UTC (permalink / raw)
To: Okash Khawaja
Cc: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, 22 Jun 2018 12:17:52 +0100, Okash Khawaja wrote:
> > > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > > better one?
> > > > > > >
> > > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > > >
> > > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > > bpftool patches before posting.
> > > > > > >
> > > > > > > Thanks for working on this!
> > >
>
> Hi,
>
> While I agree on the point of backward compatibility, I think printing
> two overlapping pieces of information side-by-side will make the
> interface less clear. Having separate outputs for the two will keep the
> interface clear and readable.
>
> Is there a major downside to adding a new flag for BTF output?
plain output and JSON should be more or less equivalent, just formatted
differently. If we hide the BTF-ed JSON under some flag most users
will just run bpftool -jp map dump id X, see BTF is not there and move
on, without ever looking into the man page to discover there is a magic
switch...
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 1:20 ` Martin KaFai Lau
2018-06-22 11:17 ` Okash Khawaja
@ 2018-06-22 18:40 ` Jakub Kicinski
2018-06-22 20:58 ` Martin KaFai Lau
1 sibling, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 18:40 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > > > "t": {
> > > > > > > "x": 5,
> > > > > > > "y": 10
> > > > > > > },
> > > > > > > "u": 100,
> > > > > > > "v": 20,
> > > > > > > "w1": 0x7,
> > > > > > > "w2": 0x3
> > > > > > > }
> > > > > > > }
> > > > > > > ]
> > > > > >
> > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > JSON must remain backwards compatible.
> > > > > >
> > > > > > The dump today has object per entry, e.g.:
> > > > > >
> > > > > > {
> > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > ],
> > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > ]
> > > > > > }
> > > > > >
> > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > >
> > > > > > {
> > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > ],
> > > > > > "key_struct":{
> > > > > > "index":0
> > > > > > },
> Got a few questions.
>
> When we support hashtab later, the key could be int
> but reusing the name as "index" is weird.
Ugh, yes, naturally. I just typed that out without thinking, so for
array maps there is usually no BTF info?... For hashes obviously we
should just use the BTF, I'm not sure we should format indexes for
arrays nicely or not :S
> The key could also be a struct (e.g. a struct to describe ip:port).
> Can you suggest how the "key_struct" will look like?
Hm. I think in my mind it has only been a struct but that's not true :S
So the struct in the name makes very limited sense now.
Should we do:
"formatted" : {
"value" : XXX
}
Where
XXX == plain int for integers, e.g. "value":0
XXX == array for arrays, e.g. "value":[1,2,3,4]
XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
> > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > ],
> > > > > > "value_struct":{
> > > > > > "src_ip":2,
> If for the same map the user changes the "src_ip" to an array of int[4]
> later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> Is it breaking backward compat?
> i.e.
> struct five_tuples {
> - int src_ip;
> + int src_ip[4];
> /* ... */
> };
Well, it is breaking backward compat, but it's the program doing it,
not bpftool :) BTF changes so does the output.
> > > > > > "dst_ip:0
> > > > > > }
> > > > > > }
> > > > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > > > while most people would prefer "key_struct"/"value_struct" if it is
> > > > > available.
> > > >
> > > > Agreed, it's not that useful, especially with the string-hex debacle :(
> > > > It's just about the backwards compat.
> > > >
> > > > > How about introducing a new option, like "-b", to print the
> > > > > map with BTF (if available) such that it won't break the existing
> > > > > one (-j or -p) while the "-b" output can keep using the "key"
> > > > > and "value".
> > > > >
> > > > > The existing json can be kept as is.
> > > >
> > > > That was my knee jerk reaction too, but on reflection it doesn't sound
> > > > that great. We expect people with new-enough bpftool to use btf, so it
> > > > should be available in the default output, without hiding it behind a
> > > > switch. We could add a switch to hide the old output, but that doesn't
> > > > give us back the names... What about Key and Value or k and v? Or
> > > > key_fields and value_fields?
> > > I thought the current default output is "plain" ;)
> > > Having said that, yes, the btf is currently printed in json.
> > >
> > > Ideally, the default json output should do what most people want:
> > > print btf and btf only (if it is available).
> > > but I don't see a way out without new option if we need to
> > > be backward compat :(
> > >
> > > Agree that showing the btf in the existing json output will be useful (e.g.
> > > to hint people that BTF is available). If btf is showing in old json,
> > > also agree that the names should be the same with the new json.
> > > key_fields and value_fields may hint it has >1 fields though.
> > > May be "formatted_key" and "formatted_value"?
> >
> > SGTM! Or even maybe as a "formatted" object?:
> >
> > {
> > "key":["0x00","0x00","0x00","0x00",
> > ],
> > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > ],
> > "formatted":{
> > "key":{
> > "index":0
> > },
> > "value":{
> > "src_ip":2,
> > "dst_ip:0
> > }
> > }
> hmm... that is an extra indentation (keep in mind that the "value" could
> already have a few nested structs which itself consumes a few indentations)
> but I guess adding another one may be ok-ish.
I'm not fussed about this, whatever JSON writer does by default is fine
with me, really.
> > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > better one?
> > > > > >
> > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > >
> > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > bpftool patches before posting.
> > > > > >
> > > > > > Thanks for working on this!
> >
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 18:40 ` Jakub Kicinski
@ 2018-06-22 20:58 ` Martin KaFai Lau
2018-06-22 21:27 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-22 20:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 11:40:32AM -0700, Jakub Kicinski wrote:
> On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> > On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > > > > "t": {
> > > > > > > > "x": 5,
> > > > > > > > "y": 10
> > > > > > > > },
> > > > > > > > "u": 100,
> > > > > > > > "v": 20,
> > > > > > > > "w1": 0x7,
> > > > > > > > "w2": 0x3
> > > > > > > > }
> > > > > > > > }
> > > > > > > > ]
> > > > > > >
> > > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > > JSON must remain backwards compatible.
> > > > > > >
> > > > > > > The dump today has object per entry, e.g.:
> > > > > > >
> > > > > > > {
> > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > ],
> > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > ]
> > > > > > > }
> > > > > > >
> > > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > > >
> > > > > > > {
> > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > ],
> > > > > > > "key_struct":{
> > > > > > > "index":0
> > > > > > > },
> > Got a few questions.
> >
> > When we support hashtab later, the key could be int
> > but reusing the name as "index" is weird.
>
> Ugh, yes, naturally. I just typed that out without thinking, so for
> array maps there is usually no BTF info?... For hashes obviously we
The key of array map also has BTF info which must be an int.
> should just use the BTF, I'm not sure we should format indexes for
> arrays nicely or not :S
>
> > The key could also be a struct (e.g. a struct to describe ip:port).
> > Can you suggest how the "key_struct" will look like?
>
> Hm. I think in my mind it has only been a struct but that's not true :S
> So the struct in the name makes very limited sense now.
>
> Should we do:
> "formatted" : {
> "value" : XXX
> }
>
> Where
> XXX == plain int for integers, e.g. "value":0
> XXX == array for arrays, e.g. "value":[1,2,3,4]
> XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
It is exactly how this patch is using json's {}, [] and int. ;)
but other than that, it does not have to be json.
In the next spin, lets stop calling this output json to avoid wrong
user's expection and I also don't want the future readability
improvements to be limited by that. Lets call it something
like plain text output with BTF.
How about:
When "bpftool map dump id 1" is used, it will print the BTF plaintext output
if a map has BTF available. If not, it will print the existing
plaintext output. That should solve the concern about the user may not
know BTF is available.
This ascii output is for human. The script should not parse the ascii output
because it is silly not to use the binary ABI (like what this patch is using)
which does not suffer backward compat issue.
The existing "-j" can be used to dump the map's binary data
for remote debugging purpose. The BTF is in one of the elf section of
the bpf_prog.o.
>
> > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > ],
> > > > > > > "value_struct":{
> > > > > > > "src_ip":2,
> > If for the same map the user changes the "src_ip" to an array of int[4]
> > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > Is it breaking backward compat?
> > i.e.
> > struct five_tuples {
> > - int src_ip;
> > + int src_ip[4];
> > /* ... */
> > };
>
> Well, it is breaking backward compat, but it's the program doing it,
> not bpftool :) BTF changes so does the output.
As we see, the key/value's btf-output is inherently not backward compat.
Hence, "-j" and "-p" will stay as is. The whole existing json will
be backward compat instead of only partly backward compat.
>
> > > > > > > "dst_ip:0
> > > > > > > }
> > > > > > > }
> > > > > > I am not sure how useful to have both "key|value" and "(key|value)_struct"
> > > > > > while most people would prefer "key_struct"/"value_struct" if it is
> > > > > > available.
> > > > >
> > > > > Agreed, it's not that useful, especially with the string-hex debacle :(
> > > > > It's just about the backwards compat.
> > > > >
> > > > > > How about introducing a new option, like "-b", to print the
> > > > > > map with BTF (if available) such that it won't break the existing
> > > > > > one (-j or -p) while the "-b" output can keep using the "key"
> > > > > > and "value".
> > > > > >
> > > > > > The existing json can be kept as is.
> > > > >
> > > > > That was my knee jerk reaction too, but on reflection it doesn't sound
> > > > > that great. We expect people with new-enough bpftool to use btf, so it
> > > > > should be available in the default output, without hiding it behind a
> > > > > switch. We could add a switch to hide the old output, but that doesn't
> > > > > give us back the names... What about Key and Value or k and v? Or
> > > > > key_fields and value_fields?
> > > > I thought the current default output is "plain" ;)
> > > > Having said that, yes, the btf is currently printed in json.
> > > >
> > > > Ideally, the default json output should do what most people want:
> > > > print btf and btf only (if it is available).
> > > > but I don't see a way out without new option if we need to
> > > > be backward compat :(
> > > >
> > > > Agree that showing the btf in the existing json output will be useful (e.g.
> > > > to hint people that BTF is available). If btf is showing in old json,
> > > > also agree that the names should be the same with the new json.
> > > > key_fields and value_fields may hint it has >1 fields though.
> > > > May be "formatted_key" and "formatted_value"?
> > >
> > > SGTM! Or even maybe as a "formatted" object?:
> > >
> > > {
> > > "key":["0x00","0x00","0x00","0x00",
> > > ],
> > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > ],
> > > "formatted":{
> > > "key":{
> > > "index":0
> > > },
> > > "value":{
> > > "src_ip":2,
> > > "dst_ip:0
> > > }
> > > }
> > hmm... that is an extra indentation (keep in mind that the "value" could
> > already have a few nested structs which itself consumes a few indentations)
> > but I guess adding another one may be ok-ish.
>
> I'm not fussed about this, whatever JSON writer does by default is fine
> with me, really.
>
> > > > > > > The name XYZ_struct may not be the best, perhaps you can come up with a
> > > > > > > better one?
> > > > > > >
> > > > > > > Does that make sense? Am I missing what you're doing here?
> > > > > > >
> > > > > > > One process note - please make sure you run checkpatch.pl --strict on
> > > > > > > bpftool patches before posting.
> > > > > > >
> > > > > > > Thanks for working on this!
> > >
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 20:58 ` Martin KaFai Lau
@ 2018-06-22 21:27 ` Jakub Kicinski
2018-06-22 21:49 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 21:27 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, 22 Jun 2018 13:58:52 -0700, Martin KaFai Lau wrote:
> On Fri, Jun 22, 2018 at 11:40:32AM -0700, Jakub Kicinski wrote:
> > On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> > > On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > > > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > > > > > "t": {
> > > > > > > > > "x": 5,
> > > > > > > > > "y": 10
> > > > > > > > > },
> > > > > > > > > "u": 100,
> > > > > > > > > "v": 20,
> > > > > > > > > "w1": 0x7,
> > > > > > > > > "w2": 0x3
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > ]
> > > > > > > >
> > > > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > > > JSON must remain backwards compatible.
> > > > > > > >
> > > > > > > > The dump today has object per entry, e.g.:
> > > > > > > >
> > > > > > > > {
> > > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > > ],
> > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > ]
> > > > > > > > }
> > > > > > > >
> > > > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > > > >
> > > > > > > > {
> > > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > > ],
> > > > > > > > "key_struct":{
> > > > > > > > "index":0
> > > > > > > > },
> > > Got a few questions.
> > >
> > > When we support hashtab later, the key could be int
> > > but reusing the name as "index" is weird.
> >
> > Ugh, yes, naturally. I just typed that out without thinking, so for
> > array maps there is usually no BTF info?... For hashes obviously we
> The key of array map also has BTF info which must be an int.
Perfect.
> > should just use the BTF, I'm not sure we should format indexes for
> > arrays nicely or not :S
> >
> > > The key could also be a struct (e.g. a struct to describe ip:port).
> > > Can you suggest how the "key_struct" will look like?
> >
> > Hm. I think in my mind it has only been a struct but that's not true :S
> > So the struct in the name makes very limited sense now.
> >
> > Should we do:
> > "formatted" : {
> > "value" : XXX
> > }
> >
> > Where
> > XXX == plain int for integers, e.g. "value":0
> > XXX == array for arrays, e.g. "value":[1,2,3,4]
> > XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
> It is exactly how this patch is using json's {}, [] and int. ;)
Great, then just wrap that in a "formatted" object instead of
redefining fields and we're good?
> but other than that, it does not have to be json.
> In the next spin, lets stop calling this output json to avoid wrong
> user's expection and I also don't want the future readability
> improvements to be limited by that. Lets call it something
> like plain text output with BTF.
I don't understand. We are discussing JSON output here. The example we
are commenting on is output of:
$ sudo bpftool map dump -p id 14
That -p means JSON. Nobody objects to plain test output changes. I
actually didn't realize you haven't implemented plain text in this
series, we should have both.
> How about:
> When "bpftool map dump id 1" is used, it will print the BTF plaintext output
> if a map has BTF available. If not, it will print the existing
> plaintext output. That should solve the concern about the user may not
> know BTF is available.
>
> This ascii output is for human. The script should not parse the ascii output
> because it is silly not to use the binary ABI (like what this patch is using)
> which does not suffer backward compat issue.
What binary ABI? I'm also not 100% sure what this patch is doing as it
adds bunch of code in new files that never gets called:
tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
tools/bpf/bpftool/btf_dumper.h | 18 ++
2 files changed, 265 insertions(+)
> The existing "-j" can be used to dump the map's binary data
> for remote debugging purpose. The BTF is in one of the elf section of
> the bpf_prog.o.
> > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > ],
> > > > > > > > "value_struct":{
> > > > > > > > "src_ip":2,
> > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > Is it breaking backward compat?
> > > i.e.
> > > struct five_tuples {
> > > - int src_ip;
> > > + int src_ip[4];
> > > /* ... */
> > > };
> >
> > Well, it is breaking backward compat, but it's the program doing it,
> > not bpftool :) BTF changes so does the output.
> As we see, the key/value's btf-output is inherently not backward compat.
> Hence, "-j" and "-p" will stay as is. The whole existing json will
> be backward compat instead of only partly backward compat.
No. There is a difference between user of a facility changing their
input and kernel/libraries providing different output in response to
that, and the libraries suddenly changing the output on their own.
Your example is like saying if user started using IPv6 addresses
instead of IPv4 the netlink attributes in dumps will be different so
kernel didn't keep backwards compat. While what you're doing is more
equivalent to dropping support for old ioctl interfaces because there
is a better mechanism now.
BTF in JSON is very useful, and will help people who writes simple
orchestration/scripts based on bpftool *a* *lot*. I really appreciate
this addition to bpftool and will start using it myself as soon as it
lands. I'm not sure why the reluctance to slightly change the output
format?
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 21:27 ` Jakub Kicinski
@ 2018-06-22 21:49 ` Jakub Kicinski
2018-06-22 23:19 ` Martin KaFai Lau
2018-06-22 22:48 ` Okash Khawaja
2018-06-22 22:54 ` Martin KaFai Lau
2 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 21:49 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, 22 Jun 2018 14:27:43 -0700, Jakub Kicinski wrote:
> BTF in JSON is very useful, and will help people who writes simple
> orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> this addition to bpftool and will start using it myself as soon as it
> lands. I'm not sure why the reluctance to slightly change the output
> format?
Ohh, maybe that's the misunderstanding, you only implemented JSON so
you wanted it to be as readable and clean as possible. Hence the hex
output and cutting out the old cruft! That perspective makes sense!
But I think we should keep JSON for machines (but including BTF
formatted values) and let's make the plain text output nice and clean,
agreed.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 21:49 ` Jakub Kicinski
@ 2018-06-22 23:19 ` Martin KaFai Lau
2018-06-22 23:40 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-22 23:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 02:49:52PM -0700, Jakub Kicinski wrote:
> On Fri, 22 Jun 2018 14:27:43 -0700, Jakub Kicinski wrote:
> > BTF in JSON is very useful, and will help people who writes simple
> > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > this addition to bpftool and will start using it myself as soon as it
> > lands. I'm not sure why the reluctance to slightly change the output
> > format?
>
> Ohh, maybe that's the misunderstanding, you only implemented JSON so
> you wanted it to be as readable and clean as possible. Hence the hex
> output and cutting out the old cruft! That perspective makes sense!
> But I think we should keep JSON for machines (but including BTF
> formatted values) and let's make the plain text output nice and clean,
> agreed.
Right, it is what my earlier comment meant on "this ascii output is
for human". We merely call it json because we are reusing
the json's meaning on {}, [] and int since it fits nicely
on what we want to achieve, readability. Other than that,
it does not have to follow other json's requirements.
We can call it whatever except json to avoid wrong
user expectation. Putting it under "-j"/"-p" was a mistake.
Hence, I said this patch belongs to the 'plaintext" output.
If we really needed a similar output under "-j" or "-p", things
had to be changed and it belongs to a separate patch.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 23:19 ` Martin KaFai Lau
@ 2018-06-22 23:40 ` Jakub Kicinski
2018-06-22 23:58 ` Martin KaFai Lau
0 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 23:40 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, 22 Jun 2018 16:19:40 -0700, Martin KaFai Lau wrote:
> On Fri, Jun 22, 2018 at 02:49:52PM -0700, Jakub Kicinski wrote:
> > On Fri, 22 Jun 2018 14:27:43 -0700, Jakub Kicinski wrote:
> > > BTF in JSON is very useful, and will help people who writes simple
> > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > > this addition to bpftool and will start using it myself as soon as it
> > > lands. I'm not sure why the reluctance to slightly change the output
> > > format?
> >
> > Ohh, maybe that's the misunderstanding, you only implemented JSON so
> > you wanted it to be as readable and clean as possible. Hence the hex
> > output and cutting out the old cruft! That perspective makes sense!
> > But I think we should keep JSON for machines (but including BTF
> > formatted values) and let's make the plain text output nice and clean,
> > agreed.
> Right, it is what my earlier comment meant on "this ascii output is
> for human". We merely call it json because we are reusing
> the json's meaning on {}, [] and int since it fits nicely
> on what we want to achieve, readability. Other than that,
> it does not have to follow other json's requirements.
> We can call it whatever except json to avoid wrong
> user expectation. Putting it under "-j"/"-p" was a mistake.
> Hence, I said this patch belongs to the 'plaintext" output.
Yes, that were the confusion came from, right. I'm personally not sold
on JSON as "human readable" but I'm very far from a UI guru so up to
you :)
I think it may be good to intentionally do non-JSON things, like use
hex integers, don't put quotes around strings, or always add a comma
after value, so people can't use it as JSON even if they try.
Basic printer is trivial to write, I'm concerned that the reuse of JSON
writer to create a user-readable output will bite us on the posterior
later on...
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 23:40 ` Jakub Kicinski
@ 2018-06-22 23:58 ` Martin KaFai Lau
0 siblings, 0 replies; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-22 23:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 04:40:48PM -0700, Jakub Kicinski wrote:
> On Fri, 22 Jun 2018 16:19:40 -0700, Martin KaFai Lau wrote:
> > On Fri, Jun 22, 2018 at 02:49:52PM -0700, Jakub Kicinski wrote:
> > > On Fri, 22 Jun 2018 14:27:43 -0700, Jakub Kicinski wrote:
> > > > BTF in JSON is very useful, and will help people who writes simple
> > > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > > > this addition to bpftool and will start using it myself as soon as it
> > > > lands. I'm not sure why the reluctance to slightly change the output
> > > > format?
> > >
> > > Ohh, maybe that's the misunderstanding, you only implemented JSON so
> > > you wanted it to be as readable and clean as possible. Hence the hex
> > > output and cutting out the old cruft! That perspective makes sense!
> > > But I think we should keep JSON for machines (but including BTF
> > > formatted values) and let's make the plain text output nice and clean,
> > > agreed.
> > Right, it is what my earlier comment meant on "this ascii output is
> > for human". We merely call it json because we are reusing
> > the json's meaning on {}, [] and int since it fits nicely
> > on what we want to achieve, readability. Other than that,
> > it does not have to follow other json's requirements.
> > We can call it whatever except json to avoid wrong
> > user expectation. Putting it under "-j"/"-p" was a mistake.
> > Hence, I said this patch belongs to the 'plaintext" output.
>
> Yes, that were the confusion came from, right. I'm personally not sold
> on JSON as "human readable" but I'm very far from a UI guru so up to
> you :)
We have thought of a few options but nothing else shine in term
of saving the verbosity on spelling out the word like "it is a struct"
, "it is an array"... and at the same time trying to use some short form
that people are already familiar with.
>
> I think it may be good to intentionally do non-JSON things, like use
> hex integers, don't put quotes around strings, or always add a comma
> after value, so people can't use it as JSON even if they try.
Good point.
>
> Basic printer is trivial to write, I'm concerned that the reuse of JSON
> writer to create a user-readable output will bite us on the posterior
> later on...
If we can think of some better way later, we can also provide another
plaintext output. I don't want to over design it at this point. Let
see how it goes.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 21:27 ` Jakub Kicinski
2018-06-22 21:49 ` Jakub Kicinski
@ 2018-06-22 22:48 ` Okash Khawaja
2018-06-22 22:54 ` Martin KaFai Lau
2 siblings, 0 replies; 45+ messages in thread
From: Okash Khawaja @ 2018-06-22 22:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 02:27:43PM -0700, Jakub Kicinski wrote:
[...]
> > > should just use the BTF, I'm not sure we should format indexes for
> > > arrays nicely or not :S
> > >
> > > > The key could also be a struct (e.g. a struct to describe ip:port).
> > > > Can you suggest how the "key_struct" will look like?
> > >
> > > Hm. I think in my mind it has only been a struct but that's not true :S
> > > So the struct in the name makes very limited sense now.
> > >
> > > Should we do:
> > > "formatted" : {
> > > "value" : XXX
> > > }
> > >
> > > Where
> > > XXX == plain int for integers, e.g. "value":0
> > > XXX == array for arrays, e.g. "value":[1,2,3,4]
> > > XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
> > It is exactly how this patch is using json's {}, [] and int. ;)
>
> Great, then just wrap that in a "formatted" object instead of
> redefining fields and we're good?
Please don't let two formats of same data in a single ouput. Overloading
same output with multple formats suggests a confused design, IMO. It
will confuse users too.
>
> > but other than that, it does not have to be json.
> > In the next spin, lets stop calling this output json to avoid wrong
> > user's expection and I also don't want the future readability
> > improvements to be limited by that. Lets call it something
> > like plain text output with BTF.
>
> I don't understand. We are discussing JSON output here. The example we
> are commenting on is output of:
>
> $ sudo bpftool map dump -p id 14
>
> That -p means JSON. Nobody objects to plain test output changes. I
> actually didn't realize you haven't implemented plain text in this
> series, we should have both.
>
> > How about:
> > When "bpftool map dump id 1" is used, it will print the BTF plaintext output
> > if a map has BTF available. If not, it will print the existing
> > plaintext output. That should solve the concern about the user may not
> > know BTF is available.
> >
> > This ascii output is for human. The script should not parse the ascii output
> > because it is silly not to use the binary ABI (like what this patch is using)
> > which does not suffer backward compat issue.
>
> What binary ABI?
Reading binary data and linking it with BTF information.
> I'm also not 100% sure what this patch is doing as it
> adds bunch of code in new files that never gets called:
Please take a look at the patch then :) Code in these files does get
called.
We seem to be conflating two different - and conflicting - intents here.
First is progam-readability of the output and second is human
readability of the output. I believe both are important. Let's leave
existing output for programs to read. That way we can try to keep it
backward compatible as well as JSON compatible. Let's dedicate the new
BTF format for humans to read. That way, we don't have to worry about
backward compatibility or JSON compatibility.
Let me know if this is not clear. If we agree on above then I think we
can move towards a solution.
Thanks,
Okash
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 21:27 ` Jakub Kicinski
2018-06-22 21:49 ` Jakub Kicinski
2018-06-22 22:48 ` Okash Khawaja
@ 2018-06-22 22:54 ` Martin KaFai Lau
2018-06-22 23:32 ` Jakub Kicinski
2 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-22 22:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 02:27:43PM -0700, Jakub Kicinski wrote:
> On Fri, 22 Jun 2018 13:58:52 -0700, Martin KaFai Lau wrote:
> > On Fri, Jun 22, 2018 at 11:40:32AM -0700, Jakub Kicinski wrote:
> > > On Thu, 21 Jun 2018 18:20:52 -0700, Martin KaFai Lau wrote:
> > > > On Thu, Jun 21, 2018 at 05:25:23PM -0700, Jakub Kicinski wrote:
> > > > > On Thu, 21 Jun 2018 16:58:15 -0700, Martin KaFai Lau wrote:
> > > > > > On Thu, Jun 21, 2018 at 04:07:19PM -0700, Jakub Kicinski wrote:
> > > > > > > On Thu, 21 Jun 2018 15:51:17 -0700, Martin KaFai Lau wrote:
> > > > > > > > On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> > > > > > > > > On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > > > > > > > > > $ sudo bpftool map dump -p 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": 0x7ffff6f70568,
> > > > > > > > > > "t": {
> > > > > > > > > > "x": 5,
> > > > > > > > > > "y": 10
> > > > > > > > > > },
> > > > > > > > > > "u": 100,
> > > > > > > > > > "v": 20,
> > > > > > > > > > "w1": 0x7,
> > > > > > > > > > "w2": 0x3
> > > > > > > > > > }
> > > > > > > > > > }
> > > > > > > > > > ]
> > > > > > > > >
> > > > > > > > > I don't think this format is okay, JSON output is an API you shouldn't
> > > > > > > > > break. You can change the non-JSON output whatever way you like, but
> > > > > > > > > JSON must remain backwards compatible.
> > > > > > > > >
> > > > > > > > > The dump today has object per entry, e.g.:
> > > > > > > > >
> > > > > > > > > {
> > > > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > > > ],
> > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > ]
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > This format must remain, you may only augment it with new fields. E.g.:
> > > > > > > > >
> > > > > > > > > {
> > > > > > > > > "key":["0x00","0x00","0x00","0x00",
> > > > > > > > > ],
> > > > > > > > > "key_struct":{
> > > > > > > > > "index":0
> > > > > > > > > },
> > > > Got a few questions.
> > > >
> > > > When we support hashtab later, the key could be int
> > > > but reusing the name as "index" is weird.
> > >
> > > Ugh, yes, naturally. I just typed that out without thinking, so for
> > > array maps there is usually no BTF info?... For hashes obviously we
> > The key of array map also has BTF info which must be an int.
>
> Perfect.
>
> > > should just use the BTF, I'm not sure we should format indexes for
> > > arrays nicely or not :S
> > >
> > > > The key could also be a struct (e.g. a struct to describe ip:port).
> > > > Can you suggest how the "key_struct" will look like?
> > >
> > > Hm. I think in my mind it has only been a struct but that's not true :S
> > > So the struct in the name makes very limited sense now.
> > >
> > > Should we do:
> > > "formatted" : {
> > > "value" : XXX
> > > }
> > >
> > > Where
> > > XXX == plain int for integers, e.g. "value":0
> > > XXX == array for arrays, e.g. "value":[1,2,3,4]
> > > XXX == object for objects, e.g. "value":{"field":XXX, "field2":XXX}
> > It is exactly how this patch is using json's {}, [] and int. ;)
>
> Great, then just wrap that in a "formatted" object instead of
> redefining fields and we're good?
>
> > but other than that, it does not have to be json.
> > In the next spin, lets stop calling this output json to avoid wrong
> > user's expection and I also don't want the future readability
> > improvements to be limited by that. Lets call it something
> > like plain text output with BTF.
>
> I don't understand. We are discussing JSON output here. The example we
> are commenting on is output of:
>
> $ sudo bpftool map dump -p id 14
>
> That -p means JSON. Nobody objects to plain test output changes. I
> actually didn't realize you haven't implemented plain text in this
> series, we should have both.
>
> > How about:
> > When "bpftool map dump id 1" is used, it will print the BTF plaintext output
> > if a map has BTF available. If not, it will print the existing
> > plaintext output. That should solve the concern about the user may not
> > know BTF is available.
> >
> > This ascii output is for human. The script should not parse the ascii output
> > because it is silly not to use the binary ABI (like what this patch is using)
> > which does not suffer backward compat issue.
>
> What binary ABI? I'm also not 100% sure what this patch is doing as it
> adds bunch of code in new files that never gets called:
I meant the BTF format, the new kernel API to get BTF and how it is used
on map data are stable.
Yes, there is new codes to get and consume the new BTF format but so does
any new kernel API. and it should not drive everybody to parse ascii.
>
> tools/bpf/bpftool/btf_dumper.c | 247 +++++++++++++++++++++++++++++++++++++++++
> tools/bpf/bpftool/btf_dumper.h | 18 ++
> 2 files changed, 265 insertions(+)
>
> > The existing "-j" can be used to dump the map's binary data
> > for remote debugging purpose. The BTF is in one of the elf section of
> > the bpf_prog.o.
>
> > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > ],
> > > > > > > > > "value_struct":{
> > > > > > > > > "src_ip":2,
> > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > Is it breaking backward compat?
> > > > i.e.
> > > > struct five_tuples {
> > > > - int src_ip;
> > > > + int src_ip[4];
> > > > /* ... */
> > > > };
> > >
> > > Well, it is breaking backward compat, but it's the program doing it,
> > > not bpftool :) BTF changes so does the output.
> > As we see, the key/value's btf-output is inherently not backward compat.
> > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > be backward compat instead of only partly backward compat.
>
> No. There is a difference between user of a facility changing their
> input and kernel/libraries providing different output in response to
> that, and the libraries suddenly changing the output on their own.
>
> Your example is like saying if user started using IPv6 addresses
> instead of IPv4 the netlink attributes in dumps will be different so
> kernel didn't keep backwards compat. While what you're doing is more
> equivalent to dropping support for old ioctl interfaces because there
> is a better mechanism now.
Sorry, I don't follow this. I don't see netlink suffer json issue like
the one on "key" and "value".
All I can grasp is, the json should normally be backward compat but now
we are saying anything added by btf-output is an exception because
the script parsing it will treat it differently than "key" and "value"
>
> BTF in JSON is very useful, and will help people who writes simple
> orchestration/scripts based on bpftool *a* *lot*. I really appreciate
Can you share what the script will do? I want to understand why
it cannot directly use the BTF format and the map data.
> this addition to bpftool and will start using it myself as soon as it
> lands. I'm not sure why the reluctance to slightly change the output
> format?
The initial change argument is because the json has to be backward compat.
Then we show that btf-output is inherently not backward compat, so
printing it in json does not make sense at all.
However, now it is saying part of it does not have to be backward compat.
I am fine putting it under "formatted" for "-j" or "-p" if that is the
case, other than the double output is still confusing. Lets wait for
Okash's input.
At the same time, the same output will be used as the default plaintext
output when BTF is available. Then the plaintext BTF output
will not be limited by the json restrictions when we want
to improve human readability later. Apparently, the
improvements on plaintext will not be always applicable
to json output.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 22:54 ` Martin KaFai Lau
@ 2018-06-22 23:32 ` Jakub Kicinski
2018-06-23 0:26 ` Martin KaFai Lau
0 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-22 23:32 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > > ],
> > > > > > > > > > "value_struct":{
> > > > > > > > > > "src_ip":2,
> > > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > > Is it breaking backward compat?
> > > > > i.e.
> > > > > struct five_tuples {
> > > > > - int src_ip;
> > > > > + int src_ip[4];
> > > > > /* ... */
> > > > > };
> > > >
> > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > not bpftool :) BTF changes so does the output.
> > > As we see, the key/value's btf-output is inherently not backward compat.
> > > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > > be backward compat instead of only partly backward compat.
> >
> > No. There is a difference between user of a facility changing their
> > input and kernel/libraries providing different output in response to
> > that, and the libraries suddenly changing the output on their own.
> >
> > Your example is like saying if user started using IPv6 addresses
> > instead of IPv4 the netlink attributes in dumps will be different so
> > kernel didn't keep backwards compat. While what you're doing is more
> > equivalent to dropping support for old ioctl interfaces because there
> > is a better mechanism now.
> Sorry, I don't follow this. I don't see netlink suffer json issue like
> the one on "key" and "value".
>
> All I can grasp is, the json should normally be backward compat but now
> we are saying anything added by btf-output is an exception because
> the script parsing it will treat it differently than "key" and "value"
Backward compatibility means that if I run *the same* program against
different kernels/libraries it continues to work. If someone decides
to upgrade their program to work with IPv6 (which was your example)
obviously there is no way system as a whole will look 1:1 the same.
> > BTF in JSON is very useful, and will help people who writes simple
> > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> Can you share what the script will do? I want to understand why
> it cannot directly use the BTF format and the map data.
Think about a python script which wants to read a counter in a map.
Right now it would have to get the BTF, find out which bytes are the
counter, then convert the bytes into a larger int. With JSON BTF if
just does entry["formatted"]["value"]["counter"].
Real life example from my test code (conversion of 3 element counter
array):
def str2int(strtab):
inttab = []
for i in strtab:
inttab.append(int(i, 16))
ba = bytearray(inttab)
if len(strtab) == 4:
fmt = "I"
elif len(strtab) == 8:
fmt = "Q"
else:
raise Exception("String array of len %d can't be unpacked to an int" %
(len(strtab)))
return struct.unpack(fmt, ba)[0]
def convert(elems, idx):
val = []
for i in range(3):
part = elems[idx]["value"][i * length:(i + 1) * length]
val.append(str2int(part))
return val
With BTF it would be:
elems[idx]["formatted"]["value"]
Which is fairly awesome.
> > this addition to bpftool and will start using it myself as soon as it
> > lands. I'm not sure why the reluctance to slightly change the output
> > format?
> The initial change argument is because the json has to be backward compat.
>
> Then we show that btf-output is inherently not backward compat, so
> printing it in json does not make sense at all.
>
> However, now it is saying part of it does not have to be backward compat.
Compatibility of "formatted" member is defined as -> fields broken out
according to BTF. So it is backward compatible. The definition of
"value" member is -> an array of unfortunately formatted array of
ugly hex strings :(
> I am fine putting it under "formatted" for "-j" or "-p" if that is the
> case, other than the double output is still confusing. Lets wait for
> Okash's input.
>
> At the same time, the same output will be used as the default plaintext
> output when BTF is available. Then the plaintext BTF output
> will not be limited by the json restrictions when we want
> to improve human readability later. Apparently, the
> improvements on plaintext will not be always applicable
> to json output.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-22 23:32 ` Jakub Kicinski
@ 2018-06-23 0:26 ` Martin KaFai Lau
2018-06-26 16:48 ` Okash Khawaja
0 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-23 0:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 04:32:00PM -0700, Jakub Kicinski wrote:
> On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > > > ],
> > > > > > > > > > > "value_struct":{
> > > > > > > > > > > "src_ip":2,
> > > > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > > > Is it breaking backward compat?
> > > > > > i.e.
> > > > > > struct five_tuples {
> > > > > > - int src_ip;
> > > > > > + int src_ip[4];
> > > > > > /* ... */
> > > > > > };
> > > > >
> > > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > > not bpftool :) BTF changes so does the output.
> > > > As we see, the key/value's btf-output is inherently not backward compat.
> > > > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > > > be backward compat instead of only partly backward compat.
> > >
> > > No. There is a difference between user of a facility changing their
> > > input and kernel/libraries providing different output in response to
> > > that, and the libraries suddenly changing the output on their own.
> > >
> > > Your example is like saying if user started using IPv6 addresses
> > > instead of IPv4 the netlink attributes in dumps will be different so
> > > kernel didn't keep backwards compat. While what you're doing is more
> > > equivalent to dropping support for old ioctl interfaces because there
> > > is a better mechanism now.
> > Sorry, I don't follow this. I don't see netlink suffer json issue like
> > the one on "key" and "value".
> >
> > All I can grasp is, the json should normally be backward compat but now
> > we are saying anything added by btf-output is an exception because
> > the script parsing it will treat it differently than "key" and "value"
>
> Backward compatibility means that if I run *the same* program against
> different kernels/libraries it continues to work. If someone decides
> to upgrade their program to work with IPv6 (which was your example)
> obviously there is no way system as a whole will look 1:1 the same.
>
> > > BTF in JSON is very useful, and will help people who writes simple
> > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > Can you share what the script will do? I want to understand why
> > it cannot directly use the BTF format and the map data.
>
> Think about a python script which wants to read a counter in a map.
> Right now it would have to get the BTF, find out which bytes are the
> counter, then convert the bytes into a larger int. With JSON BTF if
> just does entry["formatted"]["value"]["counter"].
>
> Real life example from my test code (conversion of 3 element counter
> array):
>
> def str2int(strtab):
> inttab = []
> for i in strtab:
> inttab.append(int(i, 16))
> ba = bytearray(inttab)
> if len(strtab) == 4:
> fmt = "I"
> elif len(strtab) == 8:
> fmt = "Q"
> else:
> raise Exception("String array of len %d can't be unpacked to an int" %
> (len(strtab)))
> return struct.unpack(fmt, ba)[0]
>
> def convert(elems, idx):
> val = []
> for i in range(3):
> part = elems[idx]["value"][i * length:(i + 1) * length]
> val.append(str2int(part))
> return val
>
> With BTF it would be:
>
> elems[idx]["formatted"]["value"]
>
> Which is fairly awesome.
Thanks for the example. Agree that with BTF, things are easier in general.
btw, what more awesome is,
#> bpftool map find id 100 key 1
{
"counter_x": 1,
"counter_y": 10
}
>
> > > this addition to bpftool and will start using it myself as soon as it
> > > lands. I'm not sure why the reluctance to slightly change the output
> > > format?
> > The initial change argument is because the json has to be backward compat.
> >
> > Then we show that btf-output is inherently not backward compat, so
> > printing it in json does not make sense at all.
> >
> > However, now it is saying part of it does not have to be backward compat.
>
> Compatibility of "formatted" member is defined as -> fields broken out
> according to BTF. So it is backward compatible. The definition of
> "value" member is -> an array of unfortunately formatted array of
> ugly hex strings :(
>
> > I am fine putting it under "formatted" for "-j" or "-p" if that is the
> > case, other than the double output is still confusing. Lets wait for
> > Okash's input.
> >
> > At the same time, the same output will be used as the default plaintext
> > output when BTF is available. Then the plaintext BTF output
> > will not be limited by the json restrictions when we want
> > to improve human readability later. Apparently, the
> > improvements on plaintext will not be always applicable
> > to json output.
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-23 0:26 ` Martin KaFai Lau
@ 2018-06-26 16:48 ` Okash Khawaja
2018-06-26 20:31 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Okash Khawaja @ 2018-06-26 16:48 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Jakub Kicinski, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Fri, Jun 22, 2018 at 05:26:39PM -0700, Martin KaFai Lau wrote:
> On Fri, Jun 22, 2018 at 04:32:00PM -0700, Jakub Kicinski wrote:
> > On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > > > > ],
> > > > > > > > > > > > "value_struct":{
> > > > > > > > > > > > "src_ip":2,
> > > > > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > > > > Is it breaking backward compat?
> > > > > > > i.e.
> > > > > > > struct five_tuples {
> > > > > > > - int src_ip;
> > > > > > > + int src_ip[4];
> > > > > > > /* ... */
> > > > > > > };
> > > > > >
> > > > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > > > not bpftool :) BTF changes so does the output.
> > > > > As we see, the key/value's btf-output is inherently not backward compat.
> > > > > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > > > > be backward compat instead of only partly backward compat.
> > > >
> > > > No. There is a difference between user of a facility changing their
> > > > input and kernel/libraries providing different output in response to
> > > > that, and the libraries suddenly changing the output on their own.
> > > >
> > > > Your example is like saying if user started using IPv6 addresses
> > > > instead of IPv4 the netlink attributes in dumps will be different so
> > > > kernel didn't keep backwards compat. While what you're doing is more
> > > > equivalent to dropping support for old ioctl interfaces because there
> > > > is a better mechanism now.
> > > Sorry, I don't follow this. I don't see netlink suffer json issue like
> > > the one on "key" and "value".
> > >
> > > All I can grasp is, the json should normally be backward compat but now
> > > we are saying anything added by btf-output is an exception because
> > > the script parsing it will treat it differently than "key" and "value"
> >
> > Backward compatibility means that if I run *the same* program against
> > different kernels/libraries it continues to work. If someone decides
> > to upgrade their program to work with IPv6 (which was your example)
> > obviously there is no way system as a whole will look 1:1 the same.
> >
> > > > BTF in JSON is very useful, and will help people who writes simple
> > > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > > Can you share what the script will do? I want to understand why
> > > it cannot directly use the BTF format and the map data.
> >
> > Think about a python script which wants to read a counter in a map.
> > Right now it would have to get the BTF, find out which bytes are the
> > counter, then convert the bytes into a larger int. With JSON BTF if
> > just does entry["formatted"]["value"]["counter"].
> >
> > Real life example from my test code (conversion of 3 element counter
> > array):
> >
> > def str2int(strtab):
> > inttab = []
> > for i in strtab:
> > inttab.append(int(i, 16))
> > ba = bytearray(inttab)
> > if len(strtab) == 4:
> > fmt = "I"
> > elif len(strtab) == 8:
> > fmt = "Q"
> > else:
> > raise Exception("String array of len %d can't be unpacked to an int" %
> > (len(strtab)))
> > return struct.unpack(fmt, ba)[0]
> >
> > def convert(elems, idx):
> > val = []
> > for i in range(3):
> > part = elems[idx]["value"][i * length:(i + 1) * length]
> > val.append(str2int(part))
> > return val
> >
> > With BTF it would be:
> >
> > elems[idx]["formatted"]["value"]
> >
> > Which is fairly awesome.
> Thanks for the example. Agree that with BTF, things are easier in general.
>
> btw, what more awesome is,
> #> bpftool map find id 100 key 1
> {
> "counter_x": 1,
> "counter_y": 10
> }
>
> >
> > > > this addition to bpftool and will start using it myself as soon as it
> > > > lands. I'm not sure why the reluctance to slightly change the output
> > > > format?
> > > The initial change argument is because the json has to be backward compat.
> > >
> > > Then we show that btf-output is inherently not backward compat, so
> > > printing it in json does not make sense at all.
> > >
> > > However, now it is saying part of it does not have to be backward compat.
> >
> > Compatibility of "formatted" member is defined as -> fields broken out
> > according to BTF. So it is backward compatible. The definition of
> > "value" member is -> an array of unfortunately formatted array of
> > ugly hex strings :(
> >
> > > I am fine putting it under "formatted" for "-j" or "-p" if that is the
> > > case, other than the double output is still confusing. Lets wait for
> > > Okash's input.
> > >
> > > At the same time, the same output will be used as the default plaintext
> > > output when BTF is available. Then the plaintext BTF output
> > > will not be limited by the json restrictions when we want
> > > to improve human readability later. Apparently, the
> > > improvements on plaintext will not be always applicable
> > > to json output.
> >
hi,
so i guess following is what we want:
1. a "formatted" object nested inside -p and -j switches for bpf map
dump. this will be JSON and backward compatible
2. an output for humans - which is like the current output. this will
not be JSON. this won't have to be backward compatible. this output will
be shown when neither of -j and -p are supplied and btf info is
available.
i can update the patches to v2 which covers 2 above + all other comments
on the patchset. later we can follow up with a patch for 1.
thanks for valuable feedback :)
okash
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-26 16:48 ` Okash Khawaja
@ 2018-06-26 20:31 ` Jakub Kicinski
2018-06-26 22:27 ` Martin KaFai Lau
0 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-26 20:31 UTC (permalink / raw)
To: Okash Khawaja
Cc: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Tue, 26 Jun 2018 17:48:22 +0100, Okash Khawaja wrote:
> On Fri, Jun 22, 2018 at 05:26:39PM -0700, Martin KaFai Lau wrote:
> > On Fri, Jun 22, 2018 at 04:32:00PM -0700, Jakub Kicinski wrote:
> > > On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > > > > > ],
> > > > > > > > > > > > > "value_struct":{
> > > > > > > > > > > > > "src_ip":2,
> > > > > > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > > > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > > > > > Is it breaking backward compat?
> > > > > > > > i.e.
> > > > > > > > struct five_tuples {
> > > > > > > > - int src_ip;
> > > > > > > > + int src_ip[4];
> > > > > > > > /* ... */
> > > > > > > > };
> > > > > > >
> > > > > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > > > > not bpftool :) BTF changes so does the output.
> > > > > > As we see, the key/value's btf-output is inherently not backward compat.
> > > > > > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > > > > > be backward compat instead of only partly backward compat.
> > > > >
> > > > > No. There is a difference between user of a facility changing their
> > > > > input and kernel/libraries providing different output in response to
> > > > > that, and the libraries suddenly changing the output on their own.
> > > > >
> > > > > Your example is like saying if user started using IPv6 addresses
> > > > > instead of IPv4 the netlink attributes in dumps will be different so
> > > > > kernel didn't keep backwards compat. While what you're doing is more
> > > > > equivalent to dropping support for old ioctl interfaces because there
> > > > > is a better mechanism now.
> > > > Sorry, I don't follow this. I don't see netlink suffer json issue like
> > > > the one on "key" and "value".
> > > >
> > > > All I can grasp is, the json should normally be backward compat but now
> > > > we are saying anything added by btf-output is an exception because
> > > > the script parsing it will treat it differently than "key" and "value"
> > >
> > > Backward compatibility means that if I run *the same* program against
> > > different kernels/libraries it continues to work. If someone decides
> > > to upgrade their program to work with IPv6 (which was your example)
> > > obviously there is no way system as a whole will look 1:1 the same.
> > >
> > > > > BTF in JSON is very useful, and will help people who writes simple
> > > > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > > > Can you share what the script will do? I want to understand why
> > > > it cannot directly use the BTF format and the map data.
> > >
> > > Think about a python script which wants to read a counter in a map.
> > > Right now it would have to get the BTF, find out which bytes are the
> > > counter, then convert the bytes into a larger int. With JSON BTF if
> > > just does entry["formatted"]["value"]["counter"].
> > >
> > > Real life example from my test code (conversion of 3 element counter
> > > array):
> > >
> > > def str2int(strtab):
> > > inttab = []
> > > for i in strtab:
> > > inttab.append(int(i, 16))
> > > ba = bytearray(inttab)
> > > if len(strtab) == 4:
> > > fmt = "I"
> > > elif len(strtab) == 8:
> > > fmt = "Q"
> > > else:
> > > raise Exception("String array of len %d can't be unpacked to an int" %
> > > (len(strtab)))
> > > return struct.unpack(fmt, ba)[0]
> > >
> > > def convert(elems, idx):
> > > val = []
> > > for i in range(3):
> > > part = elems[idx]["value"][i * length:(i + 1) * length]
> > > val.append(str2int(part))
> > > return val
> > >
> > > With BTF it would be:
> > >
> > > elems[idx]["formatted"]["value"]
> > >
> > > Which is fairly awesome.
> > Thanks for the example. Agree that with BTF, things are easier in general.
> >
> > btw, what more awesome is,
> > #> bpftool map find id 100 key 1
> > {
> > "counter_x": 1,
> > "counter_y": 10
> > }
> >
> > >
> > > > > this addition to bpftool and will start using it myself as soon as it
> > > > > lands. I'm not sure why the reluctance to slightly change the output
> > > > > format?
> > > > The initial change argument is because the json has to be backward compat.
> > > >
> > > > Then we show that btf-output is inherently not backward compat, so
> > > > printing it in json does not make sense at all.
> > > >
> > > > However, now it is saying part of it does not have to be backward compat.
> > >
> > > Compatibility of "formatted" member is defined as -> fields broken out
> > > according to BTF. So it is backward compatible. The definition of
> > > "value" member is -> an array of unfortunately formatted array of
> > > ugly hex strings :(
> > >
> > > > I am fine putting it under "formatted" for "-j" or "-p" if that is the
> > > > case, other than the double output is still confusing. Lets wait for
> > > > Okash's input.
> > > >
> > > > At the same time, the same output will be used as the default plaintext
> > > > output when BTF is available. Then the plaintext BTF output
> > > > will not be limited by the json restrictions when we want
> > > > to improve human readability later. Apparently, the
> > > > improvements on plaintext will not be always applicable
> > > > to json output.
> > >
>
> hi,
>
> so i guess following is what we want:
>
> 1. a "formatted" object nested inside -p and -j switches for bpf map
> dump. this will be JSON and backward compatible
> 2. an output for humans - which is like the current output. this will
> not be JSON. this won't have to be backward compatible. this output will
> be shown when neither of -j and -p are supplied and btf info is
> available.
>
> i can update the patches to v2 which covers 2 above + all other comments
> on the patchset. later we can follow up with a patch for 1.
Please do both at the same time. I've learnt not to trust people when
they say things like "we can follow up with xyz" :( In my experience it
_always_ backfires.
Implementing both outputs in one series will help you structure your
code to best suit both of the formats up front.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-26 20:31 ` Jakub Kicinski
@ 2018-06-26 22:27 ` Martin KaFai Lau
2018-06-26 22:35 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Martin KaFai Lau @ 2018-06-26 22:27 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
> On Tue, 26 Jun 2018 17:48:22 +0100, Okash Khawaja wrote:
> > On Fri, Jun 22, 2018 at 05:26:39PM -0700, Martin KaFai Lau wrote:
> > > On Fri, Jun 22, 2018 at 04:32:00PM -0700, Jakub Kicinski wrote:
> > > > On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > > > > > > ],
> > > > > > > > > > > > > > "value_struct":{
> > > > > > > > > > > > > > "src_ip":2,
> > > > > > > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > > > > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > > > > > > Is it breaking backward compat?
> > > > > > > > > i.e.
> > > > > > > > > struct five_tuples {
> > > > > > > > > - int src_ip;
> > > > > > > > > + int src_ip[4];
> > > > > > > > > /* ... */
> > > > > > > > > };
> > > > > > > >
> > > > > > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > > > > > not bpftool :) BTF changes so does the output.
> > > > > > > As we see, the key/value's btf-output is inherently not backward compat.
> > > > > > > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > > > > > > be backward compat instead of only partly backward compat.
> > > > > >
> > > > > > No. There is a difference between user of a facility changing their
> > > > > > input and kernel/libraries providing different output in response to
> > > > > > that, and the libraries suddenly changing the output on their own.
> > > > > >
> > > > > > Your example is like saying if user started using IPv6 addresses
> > > > > > instead of IPv4 the netlink attributes in dumps will be different so
> > > > > > kernel didn't keep backwards compat. While what you're doing is more
> > > > > > equivalent to dropping support for old ioctl interfaces because there
> > > > > > is a better mechanism now.
> > > > > Sorry, I don't follow this. I don't see netlink suffer json issue like
> > > > > the one on "key" and "value".
> > > > >
> > > > > All I can grasp is, the json should normally be backward compat but now
> > > > > we are saying anything added by btf-output is an exception because
> > > > > the script parsing it will treat it differently than "key" and "value"
> > > >
> > > > Backward compatibility means that if I run *the same* program against
> > > > different kernels/libraries it continues to work. If someone decides
> > > > to upgrade their program to work with IPv6 (which was your example)
> > > > obviously there is no way system as a whole will look 1:1 the same.
> > > >
> > > > > > BTF in JSON is very useful, and will help people who writes simple
> > > > > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > > > > Can you share what the script will do? I want to understand why
> > > > > it cannot directly use the BTF format and the map data.
> > > >
> > > > Think about a python script which wants to read a counter in a map.
> > > > Right now it would have to get the BTF, find out which bytes are the
> > > > counter, then convert the bytes into a larger int. With JSON BTF if
> > > > just does entry["formatted"]["value"]["counter"].
> > > >
> > > > Real life example from my test code (conversion of 3 element counter
> > > > array):
> > > >
> > > > def str2int(strtab):
> > > > inttab = []
> > > > for i in strtab:
> > > > inttab.append(int(i, 16))
> > > > ba = bytearray(inttab)
> > > > if len(strtab) == 4:
> > > > fmt = "I"
> > > > elif len(strtab) == 8:
> > > > fmt = "Q"
> > > > else:
> > > > raise Exception("String array of len %d can't be unpacked to an int" %
> > > > (len(strtab)))
> > > > return struct.unpack(fmt, ba)[0]
> > > >
> > > > def convert(elems, idx):
> > > > val = []
> > > > for i in range(3):
> > > > part = elems[idx]["value"][i * length:(i + 1) * length]
> > > > val.append(str2int(part))
> > > > return val
> > > >
> > > > With BTF it would be:
> > > >
> > > > elems[idx]["formatted"]["value"]
> > > >
> > > > Which is fairly awesome.
> > > Thanks for the example. Agree that with BTF, things are easier in general.
> > >
> > > btw, what more awesome is,
> > > #> bpftool map find id 100 key 1
> > > {
> > > "counter_x": 1,
> > > "counter_y": 10
> > > }
> > >
> > > >
> > > > > > this addition to bpftool and will start using it myself as soon as it
> > > > > > lands. I'm not sure why the reluctance to slightly change the output
> > > > > > format?
> > > > > The initial change argument is because the json has to be backward compat.
> > > > >
> > > > > Then we show that btf-output is inherently not backward compat, so
> > > > > printing it in json does not make sense at all.
> > > > >
> > > > > However, now it is saying part of it does not have to be backward compat.
> > > >
> > > > Compatibility of "formatted" member is defined as -> fields broken out
> > > > according to BTF. So it is backward compatible. The definition of
> > > > "value" member is -> an array of unfortunately formatted array of
> > > > ugly hex strings :(
> > > >
> > > > > I am fine putting it under "formatted" for "-j" or "-p" if that is the
> > > > > case, other than the double output is still confusing. Lets wait for
> > > > > Okash's input.
> > > > >
> > > > > At the same time, the same output will be used as the default plaintext
> > > > > output when BTF is available. Then the plaintext BTF output
> > > > > will not be limited by the json restrictions when we want
> > > > > to improve human readability later. Apparently, the
> > > > > improvements on plaintext will not be always applicable
> > > > > to json output.
> > > >
> >
> > hi,
> >
> > so i guess following is what we want:
> >
> > 1. a "formatted" object nested inside -p and -j switches for bpf map
> > dump. this will be JSON and backward compatible
> > 2. an output for humans - which is like the current output. this will
> > not be JSON. this won't have to be backward compatible. this output will
> > be shown when neither of -j and -p are supplied and btf info is
> > available.
> >
> > i can update the patches to v2 which covers 2 above + all other comments
> > on the patchset. later we can follow up with a patch for 1.
>
> Please do both at the same time. I've learnt not to trust people when
> they say things like "we can follow up with xyz" :( In my experience it
> _always_ backfires.
>
> Implementing both outputs in one series will help you structure your
> code to best suit both of the formats up front.
hex and "formatted" are the only things missing? As always, things
can be refactored when new use case comes up. Lets wait for
Okash input.
Regardless, plaintext is our current use case. Having the current
patchset in does not stop us or others from contributing other use
cases (json, "bpftool map find"...etc), and IMO it is actually
the opposite. Others may help us get there faster than us alone.
We should not stop making forward progress and take this patch
as hostage because "abc" and "xyz" are not done together.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-26 22:27 ` Martin KaFai Lau
@ 2018-06-26 22:35 ` Jakub Kicinski
2018-06-27 10:34 ` Daniel Borkmann
0 siblings, 1 reply; 45+ messages in thread
From: Jakub Kicinski @ 2018-06-26 22:35 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
> > On Tue, 26 Jun 2018 17:48:22 +0100, Okash Khawaja wrote:
> > > On Fri, Jun 22, 2018 at 05:26:39PM -0700, Martin KaFai Lau wrote:
> > > > On Fri, Jun 22, 2018 at 04:32:00PM -0700, Jakub Kicinski wrote:
> > > > > On Fri, 22 Jun 2018 15:54:08 -0700, Martin KaFai Lau wrote:
> > > > > > > > > > > > > > > "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
> > > > > > > > > > > > > > > ],
> > > > > > > > > > > > > > > "value_struct":{
> > > > > > > > > > > > > > > "src_ip":2,
> > > > > > > > > > If for the same map the user changes the "src_ip" to an array of int[4]
> > > > > > > > > > later (e.g. to support ipv6), it will become "src_ip": [1, 2, 3, 4].
> > > > > > > > > > Is it breaking backward compat?
> > > > > > > > > > i.e.
> > > > > > > > > > struct five_tuples {
> > > > > > > > > > - int src_ip;
> > > > > > > > > > + int src_ip[4];
> > > > > > > > > > /* ... */
> > > > > > > > > > };
> > > > > > > > >
> > > > > > > > > Well, it is breaking backward compat, but it's the program doing it,
> > > > > > > > > not bpftool :) BTF changes so does the output.
> > > > > > > > As we see, the key/value's btf-output is inherently not backward compat.
> > > > > > > > Hence, "-j" and "-p" will stay as is. The whole existing json will
> > > > > > > > be backward compat instead of only partly backward compat.
> > > > > > >
> > > > > > > No. There is a difference between user of a facility changing their
> > > > > > > input and kernel/libraries providing different output in response to
> > > > > > > that, and the libraries suddenly changing the output on their own.
> > > > > > >
> > > > > > > Your example is like saying if user started using IPv6 addresses
> > > > > > > instead of IPv4 the netlink attributes in dumps will be different so
> > > > > > > kernel didn't keep backwards compat. While what you're doing is more
> > > > > > > equivalent to dropping support for old ioctl interfaces because there
> > > > > > > is a better mechanism now.
> > > > > > Sorry, I don't follow this. I don't see netlink suffer json issue like
> > > > > > the one on "key" and "value".
> > > > > >
> > > > > > All I can grasp is, the json should normally be backward compat but now
> > > > > > we are saying anything added by btf-output is an exception because
> > > > > > the script parsing it will treat it differently than "key" and "value"
> > > > >
> > > > > Backward compatibility means that if I run *the same* program against
> > > > > different kernels/libraries it continues to work. If someone decides
> > > > > to upgrade their program to work with IPv6 (which was your example)
> > > > > obviously there is no way system as a whole will look 1:1 the same.
> > > > >
> > > > > > > BTF in JSON is very useful, and will help people who writes simple
> > > > > > > orchestration/scripts based on bpftool *a* *lot*. I really appreciate
> > > > > > Can you share what the script will do? I want to understand why
> > > > > > it cannot directly use the BTF format and the map data.
> > > > >
> > > > > Think about a python script which wants to read a counter in a map.
> > > > > Right now it would have to get the BTF, find out which bytes are the
> > > > > counter, then convert the bytes into a larger int. With JSON BTF if
> > > > > just does entry["formatted"]["value"]["counter"].
> > > > >
> > > > > Real life example from my test code (conversion of 3 element counter
> > > > > array):
> > > > >
> > > > > def str2int(strtab):
> > > > > inttab = []
> > > > > for i in strtab:
> > > > > inttab.append(int(i, 16))
> > > > > ba = bytearray(inttab)
> > > > > if len(strtab) == 4:
> > > > > fmt = "I"
> > > > > elif len(strtab) == 8:
> > > > > fmt = "Q"
> > > > > else:
> > > > > raise Exception("String array of len %d can't be unpacked to an int" %
> > > > > (len(strtab)))
> > > > > return struct.unpack(fmt, ba)[0]
> > > > >
> > > > > def convert(elems, idx):
> > > > > val = []
> > > > > for i in range(3):
> > > > > part = elems[idx]["value"][i * length:(i + 1) * length]
> > > > > val.append(str2int(part))
> > > > > return val
> > > > >
> > > > > With BTF it would be:
> > > > >
> > > > > elems[idx]["formatted"]["value"]
> > > > >
> > > > > Which is fairly awesome.
> > > > Thanks for the example. Agree that with BTF, things are easier in general.
> > > >
> > > > btw, what more awesome is,
> > > > #> bpftool map find id 100 key 1
> > > > {
> > > > "counter_x": 1,
> > > > "counter_y": 10
> > > > }
> > > >
> > > > >
> > > > > > > this addition to bpftool and will start using it myself as soon as it
> > > > > > > lands. I'm not sure why the reluctance to slightly change the output
> > > > > > > format?
> > > > > > The initial change argument is because the json has to be backward compat.
> > > > > >
> > > > > > Then we show that btf-output is inherently not backward compat, so
> > > > > > printing it in json does not make sense at all.
> > > > > >
> > > > > > However, now it is saying part of it does not have to be backward compat.
> > > > >
> > > > > Compatibility of "formatted" member is defined as -> fields broken out
> > > > > according to BTF. So it is backward compatible. The definition of
> > > > > "value" member is -> an array of unfortunately formatted array of
> > > > > ugly hex strings :(
> > > > >
> > > > > > I am fine putting it under "formatted" for "-j" or "-p" if that is the
> > > > > > case, other than the double output is still confusing. Lets wait for
> > > > > > Okash's input.
> > > > > >
> > > > > > At the same time, the same output will be used as the default plaintext
> > > > > > output when BTF is available. Then the plaintext BTF output
> > > > > > will not be limited by the json restrictions when we want
> > > > > > to improve human readability later. Apparently, the
> > > > > > improvements on plaintext will not be always applicable
> > > > > > to json output.
> > > > >
> > >
> > > hi,
> > >
> > > so i guess following is what we want:
> > >
> > > 1. a "formatted" object nested inside -p and -j switches for bpf map
> > > dump. this will be JSON and backward compatible
> > > 2. an output for humans - which is like the current output. this will
> > > not be JSON. this won't have to be backward compatible. this output will
> > > be shown when neither of -j and -p are supplied and btf info is
> > > available.
> > >
> > > i can update the patches to v2 which covers 2 above + all other comments
> > > on the patchset. later we can follow up with a patch for 1.
> >
> > Please do both at the same time. I've learnt not to trust people when
> > they say things like "we can follow up with xyz" :( In my experience it
> > _always_ backfires.
> >
> > Implementing both outputs in one series will help you structure your
> > code to best suit both of the formats up front.
> hex and "formatted" are the only things missing? As always, things
> can be refactored when new use case comes up. Lets wait for
> Okash input.
>
> Regardless, plaintext is our current use case. Having the current
> patchset in does not stop us or others from contributing other use
> cases (json, "bpftool map find"...etc), and IMO it is actually
> the opposite. Others may help us get there faster than us alone.
> We should not stop making forward progress and take this patch
> as hostage because "abc" and "xyz" are not done together.
Parity between JSON and plain text output is non negotiable.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-26 22:35 ` Jakub Kicinski
@ 2018-06-27 10:34 ` Daniel Borkmann
2018-06-27 11:47 ` Okash Khawaja
0 siblings, 1 reply; 45+ messages in thread
From: Daniel Borkmann @ 2018-06-27 10:34 UTC (permalink / raw)
To: Jakub Kicinski, Martin KaFai Lau
Cc: Okash Khawaja, Alexei Starovoitov, Yonghong Song, Quentin Monnet,
David S. Miller, netdev, kernel-team, linux-kernel
On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
> On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
>> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
[...]
>>> Implementing both outputs in one series will help you structure your
>>> code to best suit both of the formats up front.
>> hex and "formatted" are the only things missing? As always, things
>> can be refactored when new use case comes up. Lets wait for
>> Okash input.
>>
>> Regardless, plaintext is our current use case. Having the current
>> patchset in does not stop us or others from contributing other use
>> cases (json, "bpftool map find"...etc), and IMO it is actually
>> the opposite. Others may help us get there faster than us alone.
>> We should not stop making forward progress and take this patch
>> as hostage because "abc" and "xyz" are not done together.
>
> Parity between JSON and plain text output is non negotiable.
Longish discussion and some confusion in this thread. :-) First of all
thanks a lot for working on it, very useful! My $0.02 on it is that so far
great care has been taken in bpftool to indeed have feature parity between
JSON and plain text, so it would be highly desirable to keep continuing
this practice if the consensus is that it indeed is feasible and makes
sense wrt BTF data. There has been mentioned that given BTF data can be
dynamic depending on what the user loads via bpf(2) so a potential JSON
output may look different/break each time anyway. This however could all be
embedded under a container object that has a fixed key like 'formatted'
where tools like jq(1) can query into it. I think this would be fine since
the rest of the (non-dynamic) output is still retained as-is and then
wouldn't confuse or collide with existing users, and anyone programmatically
parsing deeper into the BTF data under such JSON container object needs
to have awareness of what specific data it wants to query from it; so
there's no conflict wrt breaking anything here. Imho, both outputs would
be very valuable.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-27 10:34 ` Daniel Borkmann
@ 2018-06-27 11:47 ` Okash Khawaja
2018-06-27 12:56 ` Daniel Borkmann
0 siblings, 1 reply; 45+ messages in thread
From: Okash Khawaja @ 2018-06-27 11:47 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jakub Kicinski, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:
> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
> > On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
> >> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
> [...]
> >>> Implementing both outputs in one series will help you structure your
> >>> code to best suit both of the formats up front.
> >> hex and "formatted" are the only things missing? As always, things
> >> can be refactored when new use case comes up. Lets wait for
> >> Okash input.
> >>
> >> Regardless, plaintext is our current use case. Having the current
> >> patchset in does not stop us or others from contributing other use
> >> cases (json, "bpftool map find"...etc), and IMO it is actually
> >> the opposite. Others may help us get there faster than us alone.
> >> We should not stop making forward progress and take this patch
> >> as hostage because "abc" and "xyz" are not done together.
> >
> > Parity between JSON and plain text output is non negotiable.
>
> Longish discussion and some confusion in this thread. :-) First of all
> thanks a lot for working on it, very useful!
Thanks :)
> My $0.02 on it is that so far
> great care has been taken in bpftool to indeed have feature parity between
> JSON and plain text, so it would be highly desirable to keep continuing
> this practice if the consensus is that it indeed is feasible and makes
> sense wrt BTF data. There has been mentioned that given BTF data can be
> dynamic depending on what the user loads via bpf(2) so a potential JSON
> output may look different/break each time anyway. This however could all be
> embedded under a container object that has a fixed key like 'formatted'
> where tools like jq(1) can query into it. I think this would be fine since
> the rest of the (non-dynamic) output is still retained as-is and then
> wouldn't confuse or collide with existing users, and anyone programmatically
> parsing deeper into the BTF data under such JSON container object needs
> to have awareness of what specific data it wants to query from it; so
> there's no conflict wrt breaking anything here. Imho, both outputs would
> be very valuable.
Okay I can add "formatted" object under json output.
One thing to note here is that the fixed output will change if the map
itself changes. So someone writing a program that consumes that fixed
output will have to account for his program breaking in future, thus
breaking backward compatibility anyway as far as the developer is
concerned :)
I will go ahead with work on "formatted" object.
Thanks,
Okash
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-27 11:47 ` Okash Khawaja
@ 2018-06-27 12:56 ` Daniel Borkmann
2018-07-01 10:31 ` Okash Khawaja
0 siblings, 1 reply; 45+ messages in thread
From: Daniel Borkmann @ 2018-06-27 12:56 UTC (permalink / raw)
To: Okash Khawaja
Cc: Jakub Kicinski, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On 06/27/2018 01:47 PM, Okash Khawaja wrote:
> On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:
>> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
>>> On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
>>>> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
>> [...]
>>>>> Implementing both outputs in one series will help you structure your
>>>>> code to best suit both of the formats up front.
>>>> hex and "formatted" are the only things missing? As always, things
>>>> can be refactored when new use case comes up. Lets wait for
>>>> Okash input.
>>>>
>>>> Regardless, plaintext is our current use case. Having the current
>>>> patchset in does not stop us or others from contributing other use
>>>> cases (json, "bpftool map find"...etc), and IMO it is actually
>>>> the opposite. Others may help us get there faster than us alone.
>>>> We should not stop making forward progress and take this patch
>>>> as hostage because "abc" and "xyz" are not done together.
>>>
>>> Parity between JSON and plain text output is non negotiable.
>>
>> Longish discussion and some confusion in this thread. :-) First of all
>> thanks a lot for working on it, very useful!
> Thanks :)
>
>> My $0.02 on it is that so far
>> great care has been taken in bpftool to indeed have feature parity between
>> JSON and plain text, so it would be highly desirable to keep continuing
>> this practice if the consensus is that it indeed is feasible and makes
>> sense wrt BTF data. There has been mentioned that given BTF data can be
>> dynamic depending on what the user loads via bpf(2) so a potential JSON
>> output may look different/break each time anyway. This however could all be
>> embedded under a container object that has a fixed key like 'formatted'
>> where tools like jq(1) can query into it. I think this would be fine since
>> the rest of the (non-dynamic) output is still retained as-is and then
>> wouldn't confuse or collide with existing users, and anyone programmatically
>> parsing deeper into the BTF data under such JSON container object needs
>> to have awareness of what specific data it wants to query from it; so
>> there's no conflict wrt breaking anything here. Imho, both outputs would
>> be very valuable.
> Okay I can add "formatted" object under json output.
>
> One thing to note here is that the fixed output will change if the map
> itself changes. So someone writing a program that consumes that fixed
> output will have to account for his program breaking in future, thus
Yes, that aspect is fine though, any program/script parsing this would need
to be aware of the underlying map type to make sense of it (e.g. per-cpu vs
non per-cpu maps to name one). But that info it could query/verify already
beforehand via bpftool as well (via normal map info dump for a given id).
> breaking backward compatibility anyway as far as the developer is
> concerned :)
>
> I will go ahead with work on "formatted" object.
Cool, thanks,
Daniel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-06-27 12:56 ` Daniel Borkmann
@ 2018-07-01 10:31 ` Okash Khawaja
2018-07-02 17:19 ` Jakub Kicinski
0 siblings, 1 reply; 45+ messages in thread
From: Okash Khawaja @ 2018-07-01 10:31 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Wed, Jun 27, 2018 at 02:56:49PM +0200, Daniel Borkmann wrote:
> On 06/27/2018 01:47 PM, Okash Khawaja wrote:
> > On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:
> >> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
> >>> On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
> >>>> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
> >> [...]
> >>>>> Implementing both outputs in one series will help you structure your
> >>>>> code to best suit both of the formats up front.
> >>>> hex and "formatted" are the only things missing? As always, things
> >>>> can be refactored when new use case comes up. Lets wait for
> >>>> Okash input.
> >>>>
> >>>> Regardless, plaintext is our current use case. Having the current
> >>>> patchset in does not stop us or others from contributing other use
> >>>> cases (json, "bpftool map find"...etc), and IMO it is actually
> >>>> the opposite. Others may help us get there faster than us alone.
> >>>> We should not stop making forward progress and take this patch
> >>>> as hostage because "abc" and "xyz" are not done together.
> >>>
> >>> Parity between JSON and plain text output is non negotiable.
> >>
> >> Longish discussion and some confusion in this thread. :-) First of all
> >> thanks a lot for working on it, very useful!
> > Thanks :)
> >
> >> My $0.02 on it is that so far
> >> great care has been taken in bpftool to indeed have feature parity between
> >> JSON and plain text, so it would be highly desirable to keep continuing
> >> this practice if the consensus is that it indeed is feasible and makes
> >> sense wrt BTF data. There has been mentioned that given BTF data can be
> >> dynamic depending on what the user loads via bpf(2) so a potential JSON
> >> output may look different/break each time anyway. This however could all be
> >> embedded under a container object that has a fixed key like 'formatted'
> >> where tools like jq(1) can query into it. I think this would be fine since
> >> the rest of the (non-dynamic) output is still retained as-is and then
> >> wouldn't confuse or collide with existing users, and anyone programmatically
> >> parsing deeper into the BTF data under such JSON container object needs
> >> to have awareness of what specific data it wants to query from it; so
> >> there's no conflict wrt breaking anything here. Imho, both outputs would
> >> be very valuable.
> > Okay I can add "formatted" object under json output.
> >
> > One thing to note here is that the fixed output will change if the map
> > itself changes. So someone writing a program that consumes that fixed
> > output will have to account for his program breaking in future, thus
>
> Yes, that aspect is fine though, any program/script parsing this would need
> to be aware of the underlying map type to make sense of it (e.g. per-cpu vs
> non per-cpu maps to name one). But that info it could query/verify already
> beforehand via bpftool as well (via normal map info dump for a given id).
>
> > breaking backward compatibility anyway as far as the developer is
> > concerned :)
> >
> > I will go ahead with work on "formatted" object.
>
> Cool, thanks,
> Daniel
hi,
couple of questions:
1. just to be sure, formatted section will be on the same level as "key"
and "value"? so something like following:
$ bpftool map dump -p id 8
[{
"key": ["0x00","0x00","0x00","0x00"
],
"value": [...
],
"formatted": {
"key": 0,
"value": {
"int_field": 3,
"pointerfield": 2152930552,
...
}
}
}]
2. i noticed that the ouput in v1 has all the keys and values on the
same level. in v2, i'll change them so that each key-value pair is a
separate object. let me know what you think.
finally, i noticed there is a map lookup command which also prints map
entries. do want that to also be btf-printed in this patchset?
thanks,
okash
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
2018-07-01 10:31 ` Okash Khawaja
@ 2018-07-02 17:19 ` Jakub Kicinski
0 siblings, 0 replies; 45+ messages in thread
From: Jakub Kicinski @ 2018-07-02 17:19 UTC (permalink / raw)
To: Okash Khawaja
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
On Sun, 1 Jul 2018 11:31:47 +0100, Okash Khawaja wrote:
> On Wed, Jun 27, 2018 at 02:56:49PM +0200, Daniel Borkmann wrote:
> > On 06/27/2018 01:47 PM, Okash Khawaja wrote:
> > > On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:
> > >> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
> > >>> On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
> > >>>> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
> > >> [...]
> > >>>>> Implementing both outputs in one series will help you structure your
> > >>>>> code to best suit both of the formats up front.
> > >>>> hex and "formatted" are the only things missing? As always, things
> > >>>> can be refactored when new use case comes up. Lets wait for
> > >>>> Okash input.
> > >>>>
> > >>>> Regardless, plaintext is our current use case. Having the current
> > >>>> patchset in does not stop us or others from contributing other use
> > >>>> cases (json, "bpftool map find"...etc), and IMO it is actually
> > >>>> the opposite. Others may help us get there faster than us alone.
> > >>>> We should not stop making forward progress and take this patch
> > >>>> as hostage because "abc" and "xyz" are not done together.
> > >>>
> > >>> Parity between JSON and plain text output is non negotiable.
> > >>
> > >> Longish discussion and some confusion in this thread. :-) First of all
> > >> thanks a lot for working on it, very useful!
> > > Thanks :)
> > >
> > >> My $0.02 on it is that so far
> > >> great care has been taken in bpftool to indeed have feature parity between
> > >> JSON and plain text, so it would be highly desirable to keep continuing
> > >> this practice if the consensus is that it indeed is feasible and makes
> > >> sense wrt BTF data. There has been mentioned that given BTF data can be
> > >> dynamic depending on what the user loads via bpf(2) so a potential JSON
> > >> output may look different/break each time anyway. This however could all be
> > >> embedded under a container object that has a fixed key like 'formatted'
> > >> where tools like jq(1) can query into it. I think this would be fine since
> > >> the rest of the (non-dynamic) output is still retained as-is and then
> > >> wouldn't confuse or collide with existing users, and anyone programmatically
> > >> parsing deeper into the BTF data under such JSON container object needs
> > >> to have awareness of what specific data it wants to query from it; so
> > >> there's no conflict wrt breaking anything here. Imho, both outputs would
> > >> be very valuable.
> > > Okay I can add "formatted" object under json output.
> > >
> > > One thing to note here is that the fixed output will change if the map
> > > itself changes. So someone writing a program that consumes that fixed
> > > output will have to account for his program breaking in future, thus
> >
> > Yes, that aspect is fine though, any program/script parsing this would need
> > to be aware of the underlying map type to make sense of it (e.g. per-cpu vs
> > non per-cpu maps to name one). But that info it could query/verify already
> > beforehand via bpftool as well (via normal map info dump for a given id).
> >
> > > breaking backward compatibility anyway as far as the developer is
> > > concerned :)
> > >
> > > I will go ahead with work on "formatted" object.
> >
> > Cool, thanks,
> > Daniel
>
>
> hi,
>
> couple of questions:
>
> 1. just to be sure, formatted section will be on the same level as "key"
> and "value"? so something like following:
>
>
> $ bpftool map dump -p id 8
> [{
> "key": ["0x00","0x00","0x00","0x00"
> ],
> "value": [...
> ],
> "formatted": {
> "key": 0,
> "value": {
> "int_field": 3,
> "pointerfield": 2152930552,
> ...
> }
> }
> }]
Looks good, yes!
> 2. i noticed that the ouput in v1 has all the keys and values on the
> same level. in v2, i'll change them so that each key-value pair is a
> separate object. let me know what you think.
For non-JSON output? No preference, whatever looks better :) Empty
line between key/value pairs to visually separate them could also
work. But up to you.
> finally, i noticed there is a map lookup command which also prints map
> entries. do want that to also be btf-printed in this patchset?
It would be nice to share the printing code for the two, yes.
^ permalink raw reply [flat|nested] 45+ messages in thread