[PATCH v4] tools/bpf:Fix the wrong format specifier

[PATCH v4] tools/bpf:Fix the wrong format specifier

[Zhu Jun]

The format specifier of "unsigned int" in printf() should be "%u", not
"%d".

Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
Changes:
v2:
modify commit info
v3:
fix compile warning
v4:
Thanks! But unsigned seems relevant here, and it doesn't make much sense
to change the type of the int just because we don't have the right
specifier in the printf(), does it? Sorry, I should have been more
explicit: the warning on v1 and v2 can be addressed by simply removing
the "space flag" from the format string, in other words:

 tools/bpf/bpftool/xlated_dumper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
index 567f56dfd9f1..d0094345fb2b 100644
--- a/tools/bpf/bpftool/xlated_dumper.c
+++ b/tools/bpf/bpftool/xlated_dumper.c
@@ -349,7 +349,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
 
 		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
 
-		printf("% 4d: ", i);
+		printf("%4u: ", i);
 		print_bpf_insn(&cbs, insn + i, true);
 
 		if (opcodes) {
@@ -415,7 +415,7 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
 			}
 		}
 
-		printf("%d: ", insn_off);
+		printf("%u: ", insn_off);
 		print_bpf_insn(&cbs, cur, true);
 
 		if (opcodes) {
-- 
2.17.1

Re: [PATCH v4] tools/bpf:Fix the wrong format specifier

[Quentin Monnet]

2024-07-24 04:11 UTC-0700 ~ Zhu Jun <zhujun2@cmss.chinamobile.com>
> The format specifier of "unsigned int" in printf() should be "%u", not
> "%d".
> 
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>

Great, thank you!

Acked-by: Quentin Monnet <qmo@kernel.org>

Re: [PATCH v4] tools/bpf: Fix the wrong format specifier

[Markus Elfring]

> The format specifier of "unsigned int" in printf() should be "%u", not
> "%d".

* Please improve the change description with imperative wordings.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n94

* Would you like to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n145


…
> ---
> Changes:
…
v4:
Thanks! But unsigned seems relevant here, …

Please adjust the representation of information from a patch review by Quentin Monnet.
https://lore.kernel.org/linux-kernel/2d6875dd-6050-4f57-9a6d-9168634aa6c4@kernel.org/
https://lkml.org/lkml/2024/7/24/378


…
> +++ b/tools/bpf/bpftool/xlated_dumper.c
> @@ -349,7 +349,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
>
>  		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
>
> -		printf("% 4d: ", i);
> +		printf("%4u: ", i);
>  		print_bpf_insn(&cbs, insn + i, true);
…

How do you think about to care more also for the return value from such a function call?
https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors

Regards,
Markus

Re: [PATCH v4] tools/bpf: Fix the wrong format specifier

[Quentin Monnet]

2024-07-24 17:43 UTC+0200 ~ Markus Elfring <Markus.Elfring@web.de>
>> The format specifier of "unsigned int" in printf() should be "%u", not
>> "%d".
> 
> * Please improve the change description with imperative wordings.
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n94
> 


The wording is fine. The commit subject does use imperative. If
anything, the subsystem prefix should be "bpftool" rather than
"tools/bpf", something that can be addressed when applying, perhaps.


> * Would you like to add any tags (like “Fixes” and “Cc”) accordingly?
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n145


"Fixes:" arguably, although there's no bug being fixed here, it's just a
clean-up. No need to respin the patch for that. Also there's no need to
Cc the author here, Jiong no longer works on this and the email address
you'll find in the logs is no longer valid.


> 
> 
> …
>> ---
>> Changes:
> …
> v4:
> Thanks! But unsigned seems relevant here, …
> 
> Please adjust the representation of information from a patch review by Quentin Monnet.
> https://lore.kernel.org/linux-kernel/2d6875dd-6050-4f57-9a6d-9168634aa6c4@kernel.org/
> https://lkml.org/lkml/2024/7/24/378


I'm not sure what you mean here. This part won't be kept in the commit
description anyway.

Zhu, for future patches I'd recommend keeping the history above the
comment delimiter (so that it makes it into the final patch
description), and writing a real description rather than copy/pasting
the feedback, which I believe is what Markus is commenting about?


> 
> 
> …
>> +++ b/tools/bpf/bpftool/xlated_dumper.c
>> @@ -349,7 +349,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
>>
>>  		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
>>
>> -		printf("% 4d: ", i);
>> +		printf("%4u: ", i);
>>  		print_bpf_insn(&cbs, insn + i, true);
> …
> 
> How do you think about to care more also for the return value from such a function call?
> https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors

Apologies, I'm afraid I don't understand what you're asking here, can
you please rephrase?

As far as I'm concerned I'm good with the current version of the patch.
Quentin

Re: [v4] tools/bpf: Fix the wrong format specifier

[Markus Elfring]

>>> The format specifier of "unsigned int" in printf() should be "%u", not
>>> "%d".
>>
>> * Please improve the change description with imperative wordings.
>>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10#n94
>
> The wording is fine.

I find it improvable.


> The commit subject does use imperative.

Yes.

The requirement for “imperative mood” affects mostly the commit message,
doesn't it?


>> …
>>> ---
>>> Changes:
>> …
>>> v4:
>>> Thanks! But unsigned seems relevant here, …
>>
>> Please adjust the representation of information from a patch review by Quentin Monnet.
>> https://lore.kernel.org/linux-kernel/2d6875dd-6050-4f57-9a6d-9168634aa6c4@kernel.org/
>> https://lkml.org/lkml/2024/7/24/378
>
>
> I'm not sure what you mean here.

Should quoted information be marked better anyhow in version descriptions?



> I'm not sure what you mean here. This part won't be kept in the commit
> description anyway.
>
> Zhu, for future patches I'd recommend keeping the history above the
> comment delimiter (so that it makes it into the final patch description),
…

Please reconsider such a suggestion once more.


>> …
>>> +++ b/tools/bpf/bpftool/xlated_dumper.c
>>> @@ -349,7 +349,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
>>>
>>>  		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
>>>
>>> -		printf("% 4d: ", i);
>>> +		printf("%4u: ", i);
>>>  		print_bpf_insn(&cbs, insn + i, true);
>> …
>>
>> How do you think about to care more also for the return value from such a function call?
>> https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors
>
> Apologies, I'm afraid I don't understand what you're asking here, can
> you please rephrase?

Various source code analysis tools can point further programming concerns out
for some implementation details.
https://cwe.mitre.org/data/definitions/252.html

How will development interests evolve further?

Regards,
Markus

Re: [PATCH v4] tools/bpf:Fix the wrong format specifier

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 24 Jul 2024 04:11:20 -0700 you wrote:
> The format specifier of "unsigned int" in printf() should be "%u", not
> "%d".
> 
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
> ---
> Changes:
> v2:
> modify commit info
> v3:
> fix compile warning
> v4:
> Thanks! But unsigned seems relevant here, and it doesn't make much sense
> to change the type of the int just because we don't have the right
> specifier in the printf(), does it? Sorry, I should have been more
> explicit: the warning on v1 and v2 can be addressed by simply removing
> the "space flag" from the format string, in other words:
> 
> [...]

Here is the summary with links:
  - [v4] tools/bpf:Fix the wrong format specifier
    https://git.kernel.org/bpf/bpf-next/c/781f0bbbdade

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html