[PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Xueming Feng]

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);
+			} else {
+				printf("value:%c", break_names ? '\n' : ' ');
+				fprint_hex(stdout, value, info->value_size, " ");
+			}
 		}
 
 		printf("\n");
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Kui-Feng Lee]

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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Xueming Feng]

> 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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Kui-Feng Lee]

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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Xueming Feng]

>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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Yonghong Song]

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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Xueming Feng]

> 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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Xueming Feng]

>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");

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Quentin Monnet]

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

Re: [PATCH bpf-next v2] bpftool: Dump map id instead of value for map_of_maps types

[Xueming Feng]

>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