Re: [PATCH v2 bpf-next 7/9] bpftool: add BTF dump "format meta" to dump header/metadata

[Quentin Monnet <quentin@isovalent.com>]

Thanks Alan, and apologies for the late review!

2023-06-16 18:17 UTC+0100 ~ Alan Maguire <alan.maguire@oracle.com>
> Provide a way to dump BTF metadata info via bpftool; this
> consists of BTF size, header fields and kind layout info
> (if available); for example
> 
> $ bpftool btf dump file vmlinux format meta
> size 4966513   
> magic 0xeb9f      
> version 1         
> flags 0x0         
> hdr_len 24        
> type_len 2929900   
> type_off 0         
> str_len 2036589   
> str_off 2929900   
> 
> ...or for vmlinux with kind layout, crc:
> 
> $ bpftool btf dump file vmlinux format meta
> size 5034496   
> magic 0xeb9f      
> version 1         
> flags 0x1         
> hdr_len 40        
> type_len 2973628   
> type_off 0         
> str_len 2060745   
> str_off 2973628   
> kind_layout_len 80        
> kind_layout_off 5034376   
> crc 0xb6a5171f  
> base_crc 0x0         
> kind 0    flags 0x0    info_sz 0    elem_sz 0   
> kind 1    flags 0x0    info_sz 4    elem_sz 0   
> kind 2    flags 0x0    info_sz 0    elem_sz 0   
> kind 3    flags 0x0    info_sz 12   elem_sz 0   
> kind 4    flags 0x0    info_sz 0    elem_sz 12
> ...
> 
> JSON output is also supported:
> 
> $ bpftool -j btf dump file vmlinux format meta
> {"size":4904369,{"header":"magic":60319,"version":1,"flags":0,"hdr_len":24,"type_len":2893508,"type_off":0,"str_len":2010837,"str_off":2893508}}

This is not valid JSON. I suspect that instead of:

	{"size":4904369,{"header":"magic":60319, ...

you meant the following?:

	{"size":4904369,"header":{"magic":60319,

Could you please also provide a JSON example with the kind_layouts?

> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/bpf/bpftool/bash-completion/bpftool |  2 +-
>  tools/bpf/bpftool/btf.c                   | 93 ++++++++++++++++++++++-
>  2 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 085bf18f3659..4c186d4efb35 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -937,7 +937,7 @@ _bpftool()
>                              return 0
>                              ;;
>                          format)
> -                            COMPREPLY=( $( compgen -W "c raw" -- "$cur" ) )
> +                            COMPREPLY=( $( compgen -W "c raw meta" -- "$cur" ) )
>                              ;;
>                          *)
>                              # emit extra options
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 91fcb75babe3..56f40adcc161 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -504,6 +504,90 @@ static int dump_btf_c(const struct btf *btf,
>  	return err;
>  }
>  
> +static int dump_btf_meta(const struct btf *btf)
> +{
> +	const struct btf_header *hdr;
> +	const struct btf_kind_layout *k;
> +	const void *data;
> +	__u32 data_sz;
> +	__u8 i, nr_kinds;
> +
> +	data = btf__raw_data(btf, &data_sz);
> +	if (!data)
> +		return -ENOMEM;
> +	hdr = data;
> +	if (json_output) {
> +		jsonw_start_object(json_wtr);   /* btf metadata object */
> +		jsonw_uint_field(json_wtr, "size", data_sz);
> +		jsonw_start_object(json_wtr);

... /* header object */

> +		jsonw_name(json_wtr, "header");

I think we need to swap the above two lines to fix the JSON.

> +		jsonw_uint_field(json_wtr, "magic", hdr->magic);
> +		jsonw_uint_field(json_wtr, "version", hdr->version);
> +		jsonw_uint_field(json_wtr, "flags", hdr->flags);
> +		jsonw_uint_field(json_wtr, "hdr_len", hdr->hdr_len);
> +		jsonw_uint_field(json_wtr, "type_len", hdr->type_len);
> +		jsonw_uint_field(json_wtr, "type_off", hdr->type_off);
> +		jsonw_uint_field(json_wtr, "str_len", hdr->str_len);
> +		jsonw_uint_field(json_wtr, "str_off", hdr->str_off);
> +	} else {
> +		printf("size %-10d\n", data_sz);
> +		printf("magic 0x%-10x\nversion %-10d\nflags 0x%-10x\nhdr_len %-10d\n",
> +		       hdr->magic, hdr->version, hdr->flags, hdr->hdr_len);
> +		printf("type_len %-10d\ntype_off %-10d\n", hdr->type_len, hdr->type_off);
> +		printf("str_len %-10d\nstr_off %-10d\n", hdr->str_len, hdr->str_off);
> +	}
> +
> +	if (hdr->hdr_len < sizeof(struct btf_header) ||
> +	    hdr->kind_layout_len == 0 || hdr->kind_layout_len == 0) {
> +		if (json_output) {
> +			jsonw_end_object(json_wtr); /* header object */
> +			jsonw_end_object(json_wtr); /* metadata object */
> +		}
> +		return 0;
> +	}
> +
> +	if (json_output) {
> +		jsonw_uint_field(json_wtr, "kind_layout_len", hdr->kind_layout_len);
> +		jsonw_uint_field(json_wtr, "kind_layout_offset", hdr->kind_layout_off);
> +		jsonw_uint_field(json_wtr, "crc", hdr->crc);
> +		jsonw_uint_field(json_wtr, "base_crc", hdr->base_crc);
> +		jsonw_end_object(json_wtr); /* end header object */
> +
> +		jsonw_start_object(json_wtr);
> +		jsonw_name(json_wtr, "kind_layouts");

It seems to me we have the same JSON error here as for the header.

> +		jsonw_start_array(json_wtr);
> +	} else {
> +		printf("kind_layout_len %-10d\nkind_layout_off %-10d\n",
> +		       hdr->kind_layout_len, hdr->kind_layout_off);
> +		printf("crc 0x%-10x\nbase_crc 0x%-10x\n",
> +		       hdr->crc, hdr->base_crc);
> +	}
> +
> +	k = (void *)hdr + hdr->hdr_len + hdr->kind_layout_off;
> +	nr_kinds = hdr->kind_layout_len / sizeof(*k);
> +	for (i = 0; i < nr_kinds; i++) {
> +		if (json_output) {
> +			jsonw_start_object(json_wtr);
> +			jsonw_name(json_wtr, "kind_layout");

And here?

> +			jsonw_uint_field(json_wtr, "kind", i);
> +			jsonw_uint_field(json_wtr, "flags", k[i].flags);
> +			jsonw_uint_field(json_wtr, "info_sz", k[i].info_sz);
> +			jsonw_uint_field(json_wtr, "elem_sz", k[i].elem_sz);
> +			jsonw_end_object(json_wtr);
> +		} else {
> +			printf("kind %-4d flags 0x%-4x info_sz %-4d elem_sz %-4d\n",
> +			       i, k[i].flags, k[i].info_sz, k[i].elem_sz);
> +		}
> +	}
> +	if (json_output) {
> +		jsonw_end_array(json_wtr);
> +		jsonw_end_object(json_wtr); /* end kind layout */
> +		jsonw_end_object(json_wtr); /* end metadata object */
> +	}
> +
> +	return 0;
> +}
> +

There's a number of locations where we split between two functions for
JSON and plain output, for better clarity. So we could have
dump_btf_meta_plain() and dump_btf_meta_json(). I think it would be
easier to read, but don't feel strongly so also OK if you prefer to keep
the current form to avoid duplicating the logics.

Thanks for adding all those items I asked on v1!