lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <db58f8bd-1ac6-45fc-a402-065d234d5161@kernel.org>
Date: Wed, 24 Jul 2024 16:59:06 +0100
From: Quentin Monnet <qmo@...nel.org>
To: Markus Elfring <Markus.Elfring@....de>,
 Zhu Jun <zhujun2@...s.chinamobile.com>, bpf@...r.kernel.org
Cc: LKML <linux-kernel@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Eduard Zingerman
 <eddyz87@...il.com>, Hao Luo <haoluo@...gle.com>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Yonghong Song <yonghong.song@...ux.dev>
Subject: Re: [PATCH v4] tools/bpf: Fix the wrong format specifier

2024-07-24 17:43 UTC+0200 ~ Markus Elfring <Markus.Elfring@....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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ