diff mbox series

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

Message ID 20240724111120.11625-1-zhujun2@cmss.chinamobile.com (mailing list archive)
State Accepted
Commit 781f0bbbdade00f91490a3f64212e8cc8d75905c
Delegated to: BPF
Headers show
Series [v4] tools/bpf:Fix the wrong format specifier | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc

Commit Message

Zhu Jun July 24, 2024, 11:11 a.m. UTC 
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(-)

Comments

Quentin Monnet July 24, 2024, 1:44 p.m. UTC | #1 
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>
Markus Elfring July 24, 2024, 3:43 p.m. UTC | #2 
> 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
Quentin Monnet July 24, 2024, 3:59 p.m. UTC | #3 
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
Markus Elfring July 24, 2024, 4:26 p.m. UTC | #4 
>>> 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
patchwork-bot+netdevbpf@kernel.org July 30, 2024, 8:50 p.m. UTC | #5 
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!
diff mbox series

Patch

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) {