Messages in this thread | | | Date | Mon, 24 Apr 2023 22:58:10 -0700 | Subject | Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types | From | Yonghong Song <> |
| |
On 4/24/23 9:10 PM, Xueming Feng wrote: >> On 4/24/23 2:09 AM, Xueming Feng wrote: >>> When using `bpftool map dump` in plain format, it is usually >>> more convenient to show the inner map id instead of raw value. >>> Changing this behavior would help with quick debugging with >>> `bpftool`, without disrupting scripted behavior. Since user >>> could dump the inner map with id, and need to convert value. >>> >>> Signed-off-by: Xueming Feng <kuro@kuroa.me> >>> --- >>> Changes in v2: >>> - Fix commit message grammar. >>> - Change `print_uint` to only print to stdout, make `arg` const, and rename >>> `n` to `arg_size`. >>> - Make `print_uint` able to take any size of argument up to `unsigned long`, >>> and print it as unsigned decimal. >>> >>> Thanks for the review and suggestions! I have changed my patch accordingly. >>> There is a possibility that `arg_size` is larger than `unsigned long`, >>> but previous review suggested that it should be up to the caller function to >>> set `arg_size` correctly. So I didn't add check for that, should I? >>> >>> tools/bpf/bpftool/main.c | 15 +++++++++++++++ >>> tools/bpf/bpftool/main.h | 1 + >>> tools/bpf/bpftool/map.c | 9 +++++++-- >>> 3 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c >>> index 08d0ac543c67..810c0dc10ecb 100644 >>> --- a/tools/bpf/bpftool/main.c >>> +++ b/tools/bpf/bpftool/main.c >>> @@ -251,6 +251,21 @@ int detect_common_prefix(const char *arg, ...) >>> return 0; >>> } >>> >>> +void print_uint(const void *arg, unsigned int arg_size) >>> +{ >>> + const unsigned char *data = arg; >>> + unsigned long val = 0ul; >>> + >>> + #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >>> + memcpy(&val, data, arg_size); >>> + #else >>> + memcpy((unsigned char *)&val + sizeof(val) - arg_size, >>> + data, arg_size); >>> + #endif >>> + >>> + fprintf(stdout, "%lu", val); >>> +} >>> + >>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep) >>> { >>> unsigned char *data = arg; >>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h >>> index 0ef373cef4c7..0de671423431 100644 >>> --- a/tools/bpf/bpftool/main.h >>> +++ b/tools/bpf/bpftool/main.h >>> @@ -90,6 +90,7 @@ void __printf(1, 2) p_info(const char *fmt, ...); >>> >>> bool is_prefix(const char *pfx, const char *str); >>> int detect_common_prefix(const char *arg, ...); >>> +void print_uint(const void *arg, unsigned int arg_size); >>> void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep); >>> void usage(void) __noreturn; >>> >>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c >>> index aaeb8939e137..f5be4c0564cf 100644 >>> --- a/tools/bpf/bpftool/map.c >>> +++ b/tools/bpf/bpftool/map.c >>> @@ -259,8 +259,13 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key, >>> } >>> >>> if (info->value_size) { >>> - printf("value:%c", break_names ? '\n' : ' '); >>> - fprint_hex(stdout, value, info->value_size, " "); >>> + if (map_is_map_of_maps(info->type)) { >>> + printf("id:%c", break_names ? '\n' : ' '); >> 1> + print_uint(value, info->value_size); > > On Mon, 24 Apr 2023 18:07:27 -0700, Yonghong Song wrote: >> For all map_in_map types, the inner map value size is 32bit int which >> represents a fd (for map creation) and a id (for map info), e.g., in >> show_prog_maps() in prog.c. So maybe we can simplify the code as below: >> printf("id: %u", *(unsigned int *)value); > > That is true, maybe the "id" could also be changed to "map_id" to follow the > convention. Do you think that `print_uint` could be useful in the future? > If that is the case, should I keep using it here as an example usage, and to > avoid dead code? Or should I just remove it?
Maybe, "inner_map_id" is a better choice. For array of maps, some array element value could be 0, implying "inner_map_id 0", but I think it is okay, people should know a real inner_map_id (or any map_id) should never be 0.
Function "print_uint" is not needed any more. Please remove it.
Please add the command line to dump map values triggering the above change, also the actual dumps with and without this patch.
> >>> + } else { >>> + printf("value:%c", break_names ? '\n' : ' '); >>> + fprint_hex(stdout, value, info->value_size, " "); >>> + } >>> } >>> >>> printf("\n");
| |