| Message ID | 20230424090935.52707-1-kuro@kuroa.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpftool: Dump map id instead of value for map_of_maps types | expand |
On 4/24/23 02:09, 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 Is it possible that arg_size is bigger than sizeof(val)? > + > + 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' : ' '); > + print_uint(value, info->value_size); > + } else { > + printf("value:%c", break_names ? '\n' : ' '); > + fprint_hex(stdout, value, info->value_size, " "); > + } > } > > printf("\n");
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' : ' '); > + print_uint(value, info->value_size); 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); > + } else { > + printf("value:%c", break_names ? '\n' : ' '); > + fprint_hex(stdout, value, info->value_size, " "); > + } > } > > printf("\n");
> On 4/24/23 02:09, 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 On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote: > Is it possible that arg_size is bigger than sizeof(val)? Yes it is possible, I had the thought of adding a check. But as I mentioned before the diff section, previous review https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that I should leave it to the caller function to behave. If I were to add a check, what action do you recommend if the check fails? Print a '-1', do nothing, or just use the first sizeof(val) bytes? >> + >> + 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' : ' '); >> + print_uint(value, info->value_size); >> + } else { >> + printf("value:%c", break_names ? '\n' : ' '); >> + fprint_hex(stdout, value, info->value_size, " "); >> + } >> } >> >> printf("\n");
> 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? >> + } else { >> + printf("value:%c", break_names ? '\n' : ' '); >> + fprint_hex(stdout, value, info->value_size, " "); >> + } >> } >> >> printf("\n");
On 4/24/23 20:58, Xueming Feng wrote: >> On 4/24/23 02:09, 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 > > On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote: >> Is it possible that arg_size is bigger than sizeof(val)? > > Yes it is possible, I had the thought of adding a check. But as I mentioned > before the diff section, previous review > https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that > I should leave it to the caller function to behave. If I were to add a check, > what action do you recommend if the check fails? Print a '-1', do nothing, > or just use the first sizeof(val) bytes? In the previous patch, it may have integer overflow, but it is never buffer underrun. This version uses memcpy and may cause buffer underrun if arg_size is bigger than sizeof(val). I would say that at least prevent buffer underrun from happening. > >>> + >>> + 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' : ' '); >>> + print_uint(value, info->value_size); >>> + } else { >>> + printf("value:%c", break_names ? '\n' : ' '); >>> + fprint_hex(stdout, value, info->value_size, " "); >>> + } >>> } >>> >>> printf("\n");
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");
>On 4/24/23 20:58, Xueming Feng wrote: >>> On 4/24/23 02:09, 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 >> >> On Mon, 24 Apr 2023 09:44:18 -0700, Kui-Feng Lee wrote: >>> Is it possible that arg_size is bigger than sizeof(val)? >> >> Yes it is possible, I had the thought of adding a check. But as I mentioned >> before the diff section, previous review >> https://lore.kernel.org/bpf/20230421101154.23690-1-kuro@kuroa.me/ suggested that >> I should leave it to the caller function to behave. If I were to add a check, >> what action do you recommend if the check fails? Print a '-1', do nothing, >> or just use the first sizeof(val) bytes? On Mon, 24 Apr 2023 22:19:52 -0700, Kui-Feng Lee wrote: > In the previous patch, it may have integer overflow, but it is never > buffer underrun. This version uses memcpy and may cause buffer underrun > if arg_size is bigger than sizeof(val). I would say that at least > prevent buffer underrun from happening. Thanks for the advice! This function is pending remove in the next version of this patch. But I will remember this for future patches. >>> + >>> + 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' : ' '); >>> + print_uint(value, info->value_size); >>> + } else { >>> + printf("value:%c", break_names ? '\n' : ' '); >>> + fprint_hex(stdout, value, info->value_size, " "); >>> + } >>> } >>> >>> printf("\n");
>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? On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote: > 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. Will reflect this in v3. > > Please add the command line to dump map values triggering the above > change, also the actual dumps with and without this patch. $ bpftool map dump id 138 Without patch: ``` key: fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05 27 16 06 00 value: 8b 00 00 00 Found 1 element ``` With patch: ``` key: fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05 27 16 06 00 inner_map_id: 139 Found 1 element ``` >> >>>> + } else { >>>> + printf("value:%c", break_names ? '\n' : ' '); >>>> + fprint_hex(stdout, value, info->value_size, " "); >>>> + } >>>> } >>>> >>>> printf("\n");
2023-04-25 14:37 UTC+0800 ~ Xueming Feng <kuro@kuroa.me> >> 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? This makes me think we could also have something similar for prog_array maps (but not necessarily as part of your patchset). > > On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote: >> 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. > > Will reflect this in v3. > >> >> Please add the command line to dump map values triggering the above >> change, also the actual dumps with and without this patch. > > $ bpftool map dump id 138 > Without patch: > ``` > key: > fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05 > 27 16 06 00 > value: > 8b 00 00 00 > Found 1 element > ``` > With patch: > ``` > key: > fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05 > 27 16 06 00 > inner_map_id: > 139 > Found 1 element > ``` Thanks! Please add those sample outputs to the commit description for v3. Can you please also add an example with JSON ("-p")? Quentin
>2023-04-25 14:37 UTC+0800 ~ Xueming Feng <kuro@kuroa.me> >>> 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? On Tue, 25 Apr 2023 09:57:17 +0100, Quentin Monnet wrote: > This makes me think we could also have something similar for prog_array > maps (but not necessarily as part of your patchset). Good idea, will take a look at that after finishing v3 patch, and possibly make a separate patch for it. (This is my first time contributing, better keep it simple). >> On Mon, 24 Apr 2023 22:58:10 -0700, Yonghong Song wrote: >>> 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. >> >> Will reflect this in v3. >> >>> >>> Please add the command line to dump map values triggering the above >>> change, also the actual dumps with and without this patch. >> >> $ bpftool map dump id 138 >> Without patch: >> ``` >> key: >> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05 >> 27 16 06 00 >> value: >> 8b 00 00 00 >> Found 1 element >> ``` >> With patch: >> ``` >> key: >> fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05 >> 27 16 06 00 >> inner_map_id: >> 139 >> Found 1 element >> ``` > Thanks! Please add those sample outputs to the commit description for > v3. Can you please also add an example with JSON ("-p")? Sure, will add them in v3! About json output, they are currently not touched to avoid breaking scripts. I will add inner_map_id as a new field in v3 like the following patch. https://lore.kernel.org/bpf/20230419003651.988865-1-kuifeng@meta.com/ > Quentin
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' : ' '); + print_uint(value, info->value_size); + } else { + printf("value:%c", break_names ? '\n' : ' '); + fprint_hex(stdout, value, info->value_size, " "); + } } printf("\n");
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(-)